CSS problem finder - code review, please :-)

[Hallvord R. M. Steen]

Hey, I'd appreciate some code review of this method - just two methods in Python:

https://github.com/seiflotfy/mozcompat/blob/master/browser.py#L44 onwards to line 98

I've added some special-casing for flexbox rules - the idea is that we want to warn if a site is doing
.foo{display:box}
but NOT warn if the site is doing
.foo{display:box; display: flex}
because it's not an issue when the standards-compliant CSS is included as well.

This is tested with test cases listed in
http://hallvord.com/temp/moz/compatTesterTesting/testurls.txt
but the bigger question is: are there other CSS problems that have variations like the flexbox ones? I.e. where finding the standards-compliant version is harder than just removing a "-webkit-" prefix? Or are there other non-prefixed but non-standard properties we should warn against?

Review, comments and input welcome.
-HR

[Daniel Holbert]

On 01/07/2014 01:26 AM, Hallvord R. M. Steen wrote:
> Hey, I'd appreciate some code review of this method - just two methods in Python:
>
> https://github.com/seiflotfy/mozcompat/blob/master/browser.py#L44 onwards to line 98
>
> I've added some special-casing for flexbox rules - the idea is that we want to warn if a site is doing
> .foo{display:box}
> but NOT warn if the site is doing
> .foo{display:box; display: flex}
> because it's not an issue when the standards-compliant CSS is included as well.

Nice!

Unfortunately, I think you'll need similar special-cases for a bunch of
other properties related to -webkit-box / flexbox, too, or else you'll
incorrectly flag pages as being incompatible.

For example, if you have styles like the following:

.foo { display:-webkit-box; display: flex }
.foo > * { -webkit-box-flex: 1; flex: 1 }
.foo > .childToReorder { -webkit-box-ordinal-group: 2; order: 2 }

...I suspect your tool would warn about the use of "-webkit-box-flex"
and "-webkit-box-ordinal-group" without unprefixed variants. But
really, it shouldn't warn, since those properties' new versions (from
new flexbox) are included - they just have different names.

I'm not sure if there's a comprehensive list of these flexbox
property-mappings anywhere (partly because they aren't direct mappings),
but [1] has some of them, and [2] has more in a less readable form.

A few more comments, with links to spots in the code:

***
* https://github.com/seiflotfy/mozcompat/blob/master/browser.py#L61-62

You probably want something like this for "display:-ms-flexbox", too.
(IE10 uses that name because "display:flex" was going to be named
"display:flexbox" for a period of time, and IE10 was feature-frozen (as
part of Windows 8) with prefixed support for -ms-flexbox during that
time [3])

Similarly, you'll want to check for -ms-prefixed flexbox accessory
properties from IE10, listed here: [4]

***
* https://github.com/seiflotfy/mozcompat/blob/master/browser.py#L65

I might be mistaken, but it looks like you're checking for (unprefixed)
"display: box" and "display: flexbox" here, correct? If so: I'm not sure
that's useful, because (as far as I know), no browser supports
unprefixed versions of either of those display values. WebKit/blink have
"display:-webkit-box" and IE has "display:-ms-flexbox", as noted above,
but neither have unprefixed support (because those aren't standard
property-names).

So this check might not actually be useful in practice, since any
"display:box"/"display:flexbox" style that it flags will be rejected by
*all* browsers (and hence isn't really a "compatibility" issue :) )

***
* https://github.com/seiflotfy/mozcompat/blob/master/browser.py#L52

With the "at_keyword" check here, it looks like you're discarding
everything inside of @ rules. You probably don't want to do that -- a
lot of interesting mobile-specific (i.e. likely-webkit-prefixed) code
could easily be inside of an @media query based on the screen size. You
probably want to refactor this to have a "process_rule" function, which
would look something like:
def process_rule(rule):
if (rule.at_keyword is None):
process_concrete_rule(rule)
elif (rule.at_keyword == 'media'):
# Make recursive call on each subrule
# (which themselves could be @rules)
for subrule in rule.rules:
process_rule(rule);
elif (rule.at_keyword == 'page'):
# @page is like a special selector; treat it
# like a normal rule.
process_concrete_rule(rule)
else
# Maybe report an unsupported at_keyword here?

Footnotes:
[1] http://css-tricks.com/using-flexbox/
[2] https://gist.github.com/cimmanon/4461470#file-flexbox-scss-L46
[3] http://msdn.microsoft.com/en-us/library/ie/hh673531%28v=vs.85%29.aspx
[4]
http://msdn.microsoft.com/en-us/library/ie/dn265027%28v=vs.85%29.aspx

~Daniel

[Daniel Holbert]

On 01/07/2014 11:38 PM, Daniel Holbert wrote:
> With the "at_keyword" check here, it looks like you're discarding
> everything inside of @ rules. You probably don't want to do that -- a
> lot of interesting mobile-specific (i.e. likely-webkit-prefixed) code
> could easily be inside of an @media query based on the screen size. You
> probably want to refactor this to have a "process_rule" function, which
> would look something like:
> def process_rule(rule):
> if (rule.at_keyword is None):
> process_concrete_rule(rule)
> elif (rule.at_keyword == 'media'):
> # Make recursive call on each subrule
> # (which themselves could be @rules)
> for subrule in rule.rules:
> process_rule(rule);
> elif (rule.at_keyword == 'page'):
> # @page is like a special selector; treat it
> # like a normal rule.
> process_concrete_rule(rule)
> else
> # Maybe report an unsupported at_keyword here?

BTW: my suggested code's if/else tree only checks for 'media' and 'page'
because those are the only at-rules that the CSS-parsing library you're
using (tinycss) claims to support, according to
http://pythonhosted.org/tinycss/parsing.html#parsing-a-stylesheet

(Sorry if my python is broken; I haven't written python for a while.)

Tangent: "@supports" would be useful to be able to descend into, too,
if/when tinycss has support for that. However, that gets a bit more
complicated, since an author could reasonably use @supports to split
things up like so:
@supports (display:-webkit-box) {
.container {
display: -webkit-box;
}
/* more old-flexbox stuff */
}

@supports (display:flex) {
.container {
display: flex;
}
/* more new-flexbox-stuff */
}

...and that would confuse your script, since AFAICT you're assuming that
the prefixed and unprefixed version of a property (or value) will be
specified in the same rule. Maybe that's not a common enough pattern to
worry about, though.

[Daniel Holbert]

On 01/07/2014 11:38 PM, Daniel Holbert wrote:
> elif (rule.at_keyword == 'media'):
> # Make recursive call on each subrule
> # (which themselves could be @rules)
> for subrule in rule.rules:
> process_rule(rule);

...and of course I meant "process_rule(subrule);" there.

(making the recursive call with "subrule", not "rule")

[Hallvord R. M. Steen]

>> I've added some special-casing for flexbox rules - the idea is that we want to warn if a site is doing
>> .foo{display:box}
>> but NOT warn if the site is doing
>> .foo{display:box; display: flex}
>> because it's not an issue when the standards-compliant CSS is included as well.

> Nice!
>
> Unfortunately, I think you'll need similar special-cases for a bunch of
> other properties related to -webkit-box / flexbox, too, or else you'll
> incorrectly flag pages as being incompatible.

Some false positives are (as of now) unavoidable - one of the limitations I'm currently sort of accepting is to only look for equivalents inside the current rule - so .foo{display: -webkit-flex; display: flex} will be ignored but sadly, .foo{display: -webkit-flex} html .foo{display: flex} would cause a warning even though the second selector most likely will match the same elements. An alternative implementation would be to scan through all elements on the page in WebKit and check the computed styles. If we get an annoying number of false positives with the current approach I'll consider doing that, but that also depends on being able to hook up both a WebKit and a Gecko backend.

> For example, if you have styles like the following:
>
> .foo { display:-webkit-box; display: flex }
> .foo > * { -webkit-box-flex: 1; flex: 1 }
> .foo > .childToReorder { -webkit-box-ordinal-group: 2; order: 2 }
> ...I suspect your tool would warn about the use of "-webkit-box-flex"
> and "-webkit-box-ordinal-group" without unprefixed variants. But
> really, it shouldn't warn, since those properties' new versions (from
> new flexbox) are included - they just have different names.

I don't know flexbox syntax *that* well and it probably shows. Hey, perhaps we should develop some adjacent service (could be as simple as a page with some JS) that will run through a snippet of CSS and rewrite it to add standards-compliant stuff? That would actually be a nice tool to have on webcompat.com :-)

> I'm not sure if there's a comprehensive list of these flexbox
> property-mappings anywhere (partly because they aren't direct mappings),
> but [1] has some of them, and [2] has more in a less readable form.

Thanks, I'll look at those to see if I can add more special-casing.

> A few more comments, with links to spots in the code:
>
> ***
> * https://github.com/seiflotfy/mozcompat/blob/master/browser.py#L61-62
>
> You probably want something like this for "display:-ms-flexbox", too.
> (IE10 uses that name because "display:flex" was going to be named
> "display:flexbox" for a period of time,

I guess we won't miss much content if we're not looking for -ms-flexbox - I'd assume that pretty much all sites that do add -ms-flexbox are already pretty cross-browser aware at this point in time ;-). However, it may be worth adding simply for giving better quality advice to web developers, should we actually come across a site coded especially for mobile IE.

> ***
> * https://github.com/seiflotfy/mozcompat/blob/master/browser.py#L65
> I might be mistaken, but it looks like you're checking for (unprefixed)
> "display: box" and "display: flexbox" here, correct? If so: I'm not sure
> that's useful, because (as far as I know), no browser supports
> unprefixed versions of either of those display values.

Hehe. There's like a million sites out there with display:box in its CSS code, probably added by tools trying to future-proof their code when using -webkit-box. Maybe this is actually one of the reasons the working group renamed it from box to flex? ;-)

Of course, actually detecting and warning about it may be pointless as it's pretty much always used next to -webkit-box which we already warn against. I'll consider dropping it..

> So this check might not actually be useful in practice, since any
> "display:box"/"display:flexbox" style that it flags will be rejected by
> *all* browsers (and hence isn't really a "compatibility" issue :) )

Indeed :)

> ***
> * https://github.com/seiflotfy/mozcompat/blob/master/browser.py#L52
>

> With the "at_keyword" check here, it looks like you're discarding
> everything inside of @ rules. You probably don't want to do that -- a
> lot of interesting mobile-specific (i.e. likely-webkit-prefixed) code
> could easily be inside of an @media query based on the screen size.

Oops. This is indeed not intended. I've reported https://github.com/seiflotfy/mozcompat/issues/16

> You
> probably want to refactor this to have a "process_rule" function, which
> would look something like:

Thanks a lot, Daniel! :-D
-Hallvord

[Daniel Holbert]

On 01/08/2014 12:28 AM, Hallvord R. M. Steen wrote:
>Hey, perhaps we should develop some adjacent service (could be as simple
as a page with some JS) that will run through a snippet of CSS and
rewrite it to add standards-compliant stuff? That would actually be a
nice tool to have on webcompat.com :-)

Unfortunately, any such service would require human intervention (or at
least human sanity-checking) for flexbox conversions, since the old
"-[vendor]-box" implementations have some fundamental differences from
the new stuff.

>> You probably want something like this for "display:-ms-flexbox", too.
>> (IE10 uses that name because "display:flex" was going to be named
>> "display:flexbox" for a period of time,
>
> I guess we won't miss much content if we're not looking for -ms-flexbox - I'd assume that pretty much all sites that do add -ms-flexbox are already pretty cross-browser aware at this point in time ;-). However, it may be worth adding simply for giving better quality advice to web developers, should we actually come across a site coded especially for mobile IE.

Maybe. The reason I bring it up is that I think Win8 supports
HTML/CSS-based Metro apps (and encouraged developers to make apps when
Win8 was released), and -ms-flexbox was one of the tools available for
writing such apps.

However, any such content is likely tied up in a store or something,
rather than being out there on the web for your scanner tool to
discover, so it's probably fine to not worry about it.

(As you can probably tell, I'm not particularly familiar with the
Windows / Metro ecosystem, so I may be off-base with any part of the above.)

~Daniel

[Hallvord R. M. Steen]

On 01/07/2014 11:38 PM, Daniel Holbert wrote:
> elif (rule.at_keyword == 'media'):
> # Make recursive call on each subrule
> # (which themselves could be @rules)
> for subrule in rule.rules:
> process_rule(rule);

And here you go -
https://github.com/seiflotfy/mozcompat/commit/073b3c7f031d057564f7c6c19a70d4c8582cf92b
That should address the most important issues you found - and thank you again! :-D
-Hallvord

[Mike Taylor]

On 07/01/2014 03:26, Hallvord R. M. Steen wrote:
> are there other CSS problems that have variations like the flexbox ones? I.e. where finding the standards-compliant version is harder than just removing a "-webkit-" prefix? Or are there other non-prefixed but non-standard properties we should warn against?

I sent out a tweet[1] asking this same question. Hopefully we get some
useful replies.

[1] https://twitter.com/miketaylr/status/420978040454135808

--
Mike Taylor
Web Compat, Mozilla

[Daniel Holbert]

On 01/08/2014 12:28 AM, Hallvord R. M. Steen wrote:
>> Unfortunately, I think you'll need similar special-cases for a bunch
>> of other properties related to -webkit-box / flexbox
>

> Some false positives are (as of now) unavoidable - one of the
> limitations I'm currently sort of accepting is to only look for
> equivalents inside the current rule - so
> .foo{display: -webkit-flex; display: flex} will be ignored but
> sadly, .foo{display: -webkit-flex} html .foo{display: flex} would
> cause a warning even though the second selector most likely will
> match the same elements.

I think you might've been responding to something slightly different
from what I was saying there.

I was saying that there are additional "box" properties whose modern
flexbox equivalents have different names (meaning that dropping
"-webkit" isn't sufficient, to check if there's a standards-compliant
version).

Here are the old/new property mappings that I'm aware of:
-webkit-box-align is now align-items
-webkit-box-pack is now justify-content
-webkit-box-orient is now flex-direction [1]
-webkit-box-flex is now flex [2]
-webkit-box-ordinal-group is now order
(and there's also "display:-webkit-box" --> "display:flex", but you
already have that.)

Source:
http://wiki.csswg.org/spec/flexbox-2009-2011-spec-property-mapping
(I didn't just link to that, because that doesn't include the final
versions of the new property names. Those are available here:
http://dev.w3.org/csswg/css-flexbox/#property-index )

~Daniel

[1] Authors might also use "flex-flow" instead of "flex-direction".
(flex-flow is a shorthand which includes flex-direction)

[2] Authors might also use "flex-grow" instead of "flex". (flex is a
shorthand, and the spec recommends that people use it instead of its
subcomponents; but authors might just specify its main subcomponent,
"flex-grow". That's the one that maps most closely to -webkit-box-flex,
anyway.)

[Karl Dubost]

Found in the wild today, but not sure yet they are necessary relevant for the Web compat test. Putting them here for not forgetting.


scrolling
http://fioravengi.blogspot.jp/2011/06/implications-of-ios-5-webkit-overflow.html

> .ct-reveal .yui3-sidebar,.ct-reveal-android .yui3-sidebar{
> overflow-y:scroll;
> overflow-x:hidden;
> -webkit-overflow-scrolling:touch;
> display:block}

and text size
http://www.456bereastreet.com/archive/201011/beware_of_-webkit-text-size-adjustnone/

> html{
> -webkit-text-size-adjust:none}


and tap higlight color
https://shyamalmadura.wordpress.com/2014/01/07/getting-rid-of-gray-highlight-when-tapping-objects-in-mobile-browser/

> .yui-sv-btn{
> -webkit-tap-highlight-color:rgba(0,0,0,0)}



--
Karl Dubost, Mozilla
http://www.la-grange.net/karl/moz

[Daniel Holbert]

I'm guessing (?) you were bringing these up as webkit-only experimental
properties that don't have a -moz or an unprefixed equivalent, yes?

If so: this one isn't in that category:

On 01/09/2014 04:34 PM, Karl Dubost wrote:
> and text size
> http://www.456bereastreet.com/archive/201011/beware_of_-webkit-text-size-adjustnone/
>
>> html{
>> -webkit-text-size-adjust:none}

We do have a (prefixed) version of this one -- see bottom of:
http://dbaron.org/log/20111126-font-inflation

Quoting that blog post:
"Web pages can opt out of the inflation for all or part of a page by
specifying -moz-text-size-adjust: none, much like the existing
-webkit-text-size-adjust: none and -ms-text-size-adjust: none. The
-moz-text-size-adjust property exists only to opt opt of this font size
inflation, and doesn't do anything else."

It looks like there's a draft for a standardized version of the property
here, too:
http://dev.w3.org/csswg/css-size-adjust/#adjustment-control
though it's still got some issues open and hasn't been touched in a while.

~Daniel

[Hallvord R. M. Steen]

Daniel, thanks for that extra list. I had -webkit-box-flex from your previous feedback (plus the @media fix which I thought was most urgent), now I've just checked in a fix with mappings for the other properties you listed.
https://github.com/seiflotfy/compatipede/commit/e52f1c2fd34e4053e8d604c9e4ddb290b5eca739
-Hallvord

[Hallvord R. M. Steen]

Update - with https://github.com/seiflotfy/compatipede/commit/8ec91513b00d83d639a91368068b6d715458fcbb (plus the next bugfix commit), I think the logic is quite solid. This tries to make sure we read only the function name (i.e. -webkit-linear-gradient) from the CSS and compare with other function names. It's trivial to further tweak this code by modifying the tables here:

https://github.com/seiflotfy/compatipede/blob/master/browser.py#L90

if/when we come across other -webkit- prefixed properties and functions that have slightly different standardised equivalents.

_______________________________________________
compatibility mailing list
compat...@lists.mozilla.org
https://lists.mozilla.org/listinfo/compatibility

[Mike Taylor]

On 1/17/14, 4:17, Hallvord R. M. Steen wrote:
> if/when we come across other -webkit- prefixed properties and functions that have slightly different standardised equivalents.

Did you see the stuff in https://gist.github.com/miketaylr/8321236?