1107378 - (CSSUnprefixingService) rewrite certain -webkit- prefixed CSS (ultimately for sites on a "fixlist", via bug 1132743)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Hallvord R. M. Steen [:hallvors]]

We need to handle some -webkit- styling, but only certain properties on a limited number of sites.

Some properties/values can simply be aliased. Others that involve more parsing and processing we'll implement by calling into a service implemented in a JavaScript-module.

Properties we should handle by aliasing/value substitution are:

FLEXBOX 

- see mapping table on https://github.com/hallvors/css-fixme/blob/master/cssfixme.htm#L157
* Property 'display' and values 'box', 'flexbox', 'inline-box', 'inline-flexbox'
* Property 'box-align'
* Values of property 'flex-direction'
* Property 'box-pack'
* Property 'box-ordinal-group'
* Property 'box-flex'
* Property 'flex-align'
* Property 'flex-order'

OTHER PROPERTIES suitable for simple aliasing
* Property 'border-image' 
  (note: gotcha here regarding border-style, just aliasing without adding border-style:solid may not give the expected rendering - breaks GMail buttons++)
* Property 'background-size'

GRADIENTS
In addition, this should be handled by calling a JS-based CSS fixup service:
* gradient

(Note: a -webkit- prefix is implied for all those properties / values except display. The script rewrites these properties values both if prefixed with -webkit- or not, thus potentially fixing up some sites that have attempted to add non-prefixed CSS but gotten it wrong, but I know of no sites that are fixed due to specifically handling the non-webkit-prefixed variant.)

Unsure if/how these need to be handled - might be featurecrept in..:
* @-webkit-foo rules
* transition
* animation

The processing model should probably be something along the lines of:
1) After parsing a CSS ruleset (as in a 'block' of property:value declaration - everything from { to }, check if -webkit- properties/values from that list were present.
2) If yes, if the site is fixlisted, and fixup is enabled, check if there are equivalent styles present. For example, if the ruleset already contains both a 

display: box;

and a 

display: flex;

we should avoid doing any further magic or trickery. If we've seen a -webkit-gradient in the background but a gradient will be applied by other code in the ruleset, we likewise do not apply any fixing.

3) Do substitution for simple mapping rules. Call JS service for complex ones.

Note on gradients: these can be part of complex background: shorthand declarations with code before and after. It's good if you can pass only the specific problematic part to the JS service so that the logic over there doesn't have to parse things quite as carefully.

4) If CSS fixes are to be applied, print a warning to the error console.
Suggested wording: "Firefox will rewrite parts of the CSS code on this site because the existing code has errors. Read <link> for details". (Can we have clickable links in console messages?)

5) Apply fixes automagically. Either by inserting equivalent rules in the CSS, or by using some engine-internal mechanism to apply CSS to the page.

Comment 1

[Hallvord R. M. Steen [:hallvors]]

Jet was going to pick the lucky winner of the "who implements this?" lottery

Comment 2

[Jet Villegas (inactive)]

Please add/link the top 10 fixlist sites here. Screenshots would also be good. Please sort by popularity (users want to use these sites but can't due to -webkit) not severity. We'll want to sort our fixes to enable the most popular sites first.

Comment 3

[Peipei Cheng (needinfo if you need my action)]

I will provide the top 10 fixlist sites in China mainland.

Comment 4

[Peipei Cheng (needinfo if you need my action)]

Here is the top 10 fixlist sites for china mainland and they are sorted by the popularity.

1. m.taobao.com
2. map.baidu.com
3. music.baidu.com
4. 3g.163.com
5. hao123.com
6. 3g.qq.com
7. shucheng.baidu.com
8. mogujie.com
9. dianping.com
10. m.qunar.com


Screenshots are available in following document: 
https://docs.google.com/a/mozilla.com/presentation/d/1IiAbIXfEI0-gZXKkZhZR90mN6usjgyHef2Lri2zqnfk/edit?usp=sharing

Please ni? on me if more information is needed.

Comment 5

[Daniel Holbert [:dholbert]]

FWIW, I tried hallvord's "CSS fixme" tool on the top 3 sites from comment 4, with good results.  Steps are:
 0. Configure mobile UA (I'm using "Mozilla/5.0 (Android; Tablet; rv:37.0) Gecko/37.0 Firefox/37.0")
 1. Visit site in responsive design mode.
 2. Copypaste a stylesheet out of Devtools style editor, into the textfield on http://hallvord.com/temp/moz/cssfixme.php , and click button
 3. Copypaste the results back into the Devtools style editor.
 4. Enjoy.

For m.taobao.com, there's only one stylesheet in devtools style editor.
For map.baidu.com, there are several; the second one is the one that needs fixing.
For music.baidu.com, there's only one stylesheet.

I haven't tried interacting with the sites much, but the layout looks mostly-right for taobao and entirely right (as far as I can tell) on the other two.

Comment 6

[Hallvord R. M. Steen [:hallvors]]

I went through all these sites in Portland with some help from Yanfang, Wei Deng and others from the Beijing office, using the GreaseMonkey version of the CSS fixer script ( https://github.com/hallvors/GM_tools/blob/master/CSS-fixer/CSS-fixer.user.js - logic should be the same as CSS:fixme page). Most sites worked better with than without the script, and a few of them also rendered better with the script than in a Chinese Fennec build with their -webkit- aliasing. We came across a couple of problems, hao123 in particular was a bit of a mystery but I think the problem was that the script failed to parse the external CSS file because of encoding issues with the Chinese characters. That won't be a problem when some of this logic is moved into core :)

The issues we know about from that test session are:

dianping.com	menu breaks after load because style.width of element is set very wide	(click city name to see problem)
mogujie.com	may not be able to swipe banners?	
shucheng.baidu.com 	free_books element is not wide enough for contents, leading to squashing and overlap	
3g.qq.com	weekday kanji not on one line on http://info.3g.qq.com/g/s?aid=template&tid=astro_h&iarea=84&i_f=244&sid=AR18i9Zf9knRxA1sy3RotrMK	probably charset problem
hao123.com	fails to set display:flex for <nav> and some <li> elements	probably charset problem
tieba.baidu.com	may not be able to swipe banners?	
m.taobao.com 	UA sniffing. Banner broken on «fancy non-touch» site. Untouchable icons due to event.srcElement	

(sorry if that gets a bit messed up in Bugzilla, copied from a spreadsheet..)

Comment 7

[Daniel Holbert [:dholbert]]

Note: the patches in bug 837211 may come in handy here, either directly or simply as inspiration, depending on the solution that we end up going with here.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #0)
> The processing model should probably be something along the lines of:
> 1) After parsing a CSS ruleset (as in a 'block' of property:value
> declaration - everything from { to }, check if -webkit- properties/values
> from that list were present.
> 2) If yes, if the site is fixlisted, and fixup is enabled, check if there
> are equivalent styles present. For example, if the ruleset already contains
> both a [...]

Discussed this a bit with dbaron just now.  The processing model described in my quoted text here is likely to be more complex than what's actually necessary.  Instead of doing this, it should likely be sufficient to simply treat the -webkit-prefixed keywords/properties as aliases, and let the CSS cascade handle the overriding.

I can imagine contrived cases where this wouldn't be sufficient, but until we actually see something like this in practice, it'll better to keep it simple & just use aliasing (and trust the CSS cascade to do overriding when non-prefixed style is present).  So, I'm going to proceed with something closer to bug 837211 (-webkit aliasing, controlled by an internal origin-dependent fixlist) for the time being.

(We'd need to add a decent amount of complexity to our (already complex) CSS parser code, in order for it to support the processing model described here.  Given that this is just a compatibility hack for a very limited number of websites, it'd be better not to have to add that complexity, unless it's been demonstrated to be necessary.)

Comment 9

[Daniel Holbert [:dholbert]]

Note to self: the site-specific UA override code seems to live here:
 https://mxr.mozilla.org/mozilla-central/source/dom/base/nsISiteSpecificUserAgent.idl
 https://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SiteSpecificUserAgent.js
 https://mxr.mozilla.org/mozilla-central/source/dom/base/SiteSpecificUserAgent.js
 https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/UserAgentOverrides.jsm

...and is invoked here:
  https://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#2566

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Why not just do a simple string replace in the CSS before feeding to the CSS parser if we only process them as an alias. I think it would be easier to maintain and switch as it is an independent module.

Comment 11

[Masatoshi Kimura [:emk]]

I don't think string replace before parsing is easy at all (string literals, urls, escpaes, etc.) Eventually you will have to reinvent the CSS parser.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

For alias things, I think string replace, or at most, regexp replace would be enough. I understand that this won't be perfect, but in my opinion, we only need to make it just work for most actual cases as a quick hack.

The question is, how many string literals in CSS would includes "-webkit-" in practice? What about urls? I don't think a simple replace could break much more things than alias in practice.

Comment 13

[Daniel Holbert [:dholbert]]

Simply stripping "-webkit-" out of the CSS before feeding it to the parser would not work, because many of the replacements don't use the same property-name. (see the cssfixme mapping table linked in comment 0 for details)  So, I don't think that's really a viable option. (And even if it were viable, property-aliases aren't much more complicated.)

Comment 14

[Hallvord R. M. Steen [:hallvors]]

> Discussed this a bit with dbaron just now.  The processing model described
> in my quoted text here is likely to be more complex than what's actually
> necessary. 

OK. Hopefully no problems simplifying it. Theoretically, we'd break stuff that was carefully crafted to be both Moz- and WebKit-compatible by applying their -webkit- code after all, but given the "fixlisting" approach that's hardly a concern. On the other hand, I've seen code where only the -webkit- prefixed parts are really functional and the -moz- prefixed or supposedly standard code is broken, so a simplified implementation may well buy us more compatibility. Thanks for talking through it with dbaron :)

Comment 15

[Daniel Holbert [:dholbert]]

I've got local patches that support "display: -webkit-box" as an alias for "display: flex", and also "-webkit-box-flex: N" as an alias for "flex-grow: N". This seems to be mostly sufficient to fix a lot of the brokenness in the 10 sites listed in step 4.

I've created https://wiki.mozilla.org/Platform/Layout/WebkitPrefixEmulation to track status of the various sites, to avoid too much back-and-forth on this bug page. (That wiki page isn't very populated yet, but I expect to flesh it out over the next day or two.)

A few notable highlights (not all of which I've put on the wiki page):
 (A) on music.baidu.com, I can't repro the brokenness shown in the screenshot. (maybe the site has been fixed?), though I did get a few different UI quirks, and filed bug 1127107 for the most noticable one.
 (B) on m.qunar.com, the site specifies "display:-moz-box" after "display:-webkit-box", so emulating -webkit-box with new-flexbox doesn't help (because the site overrides it). Moreover, I can't reproduce the extreme overflow shown in the screenshot (possibly because the site content or layout has changed) -- the site barely overflows for me. Need to dig into that a bit, but I'm not seeing as severe brokenness as is captured in the screenshot.
 (C) On mogujie.com, the site specifies "-moz-box" after "-webkit-box", so emulating -webkit-box with new-flexbox doesn't help (because the site overrides it).  Moreover, there's some weird thing going on with the "fadeIn" animation that prevents the images from showing up. I'm not yet sure what's going on there, but I'll document it on the wiki page when I figure it out.

Jet suggested that we address the "-moz-box" stomping in (B) and (C) by making "-moz-box" & its related properties alias to the same "display:flex"-based styles that we're using for -webkit-box, for sites on our fixlist.  I think I agree -- though that gets a bit more complicated, because now -moz-box will have two possible behaviors.  And emulating e.g. "-[moz|webkit]-box-pack:justify;" (used on mogujie.com) is tricky, because the equivalent new-flexbox style uses a different value name ("justify-content: space-between").  So maybe this means a JS-implemented CSS-rewriting service is really what we want here.

Anyway, more to come soon, here & on that wiki page.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: patch v1

Here's a mostly-done patch for this bug. Feedback welcome.

Overview:
1) All of the functionality here is behind the pref
    "layout.css.unprefixing-service.enabled"

   For now, the pref is disabled by default, and you have to enable it to test
   this.  (Before this lands, we'll also put this code behind a "is this
   website on our list?" check, and then we can probably enable the pref by
   default so that this stuff Just Works.)

2) The patch creates a JS service to convert a list of -webkit-prefixed
   properties to modern equivalents that we support. Specifically, it
   treats these properties as aliases (just swapping the property-name):

      "-webkit-background-size" -->   "background-size"
      "-webkit-box-flex" -->          "flex-grow"
      "-webkit-box-ordinal-group" --> "order"
      "-webkit-box-sizing" -->        "box-sizing"

   And, it converts the property-name *and* the value keywords for these
   properties (we can't just alias these ones, because the value keywords
   are a bit different in their modern incarnations):

      "-webkit-box-align" -->  "align-items"
      "-webkit-box-orient" --> "flex-direction"
      "-webkit-box-pack" -->   "justify-content"

   (We can add more property mappings if we discover that they're needed.)

3) Since "display" isn't itself a prefixed property, we handle
   "display: -webkit-box" separately, treating it as "flex" in the C++ code
   that parses "display" keywords.
   (We also convert "display:-moz-box" to "display:flex" if we've previously
   unprefixed some -webkit style in the same CSS rule, to handle cases like
   (B) and (C) from comment 15.)

This seems to fix all of the main usability issues that I can see on the sites from comment 4, with one exception that we can't really do much about: mogujie.com's "tile" photos stay hidden because they're unintentionally relying on a Blink/WebKit CSS animation parsing bug ( https://code.google.com/p/chromium/issues/detail?id=453182 ). Blink seems like they're going to fix that bug soon, though, so the site will be broken in Android/Chrome (as well as Firefox) soon, which maybe will prompt mogujie to fix their CSS. :)

Comment 17

[Daniel Holbert [:dholbert]]

(I've got a Try run going, which should produce some testable Android builds. Once I've verified that it passes my automated tests for this feature on Android, I'll post links to those builds here, to get feedback from the Mozilla China team.)

Comment 18

[Daniel Holbert [:dholbert]]

Looks like the Try run is good (at least, my tests for this feature passed, which means the JS service is getting registered correctly & found by the CSS parser).

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=011a0fd533ba

Fennec build for Android 4.0:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-011a0fd533ba/try-android-api-11/fennec-38.0a1.en-US.android-arm.apk

Fennec build for Android 2.3:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-011a0fd533ba/try-android-api-9/fennec-38.0a1.en-US.android-arm.apk

Peipei, could you (or someone on your team) test one of these builds on your top 10 fixlist china sites & let me know how things look? Note that you need to toggle the pref "layout.css.unprefixing-service.enabled" in "about:config" before this will work.

As noted above, I've documented the various UI issues that I've encountered on your top 10 sites at https://wiki.mozilla.org/Platform/Layout/WebkitPrefixEmulation . The green labels there indicate issues that I believe should be fixed with the attached patch; the other colors indicate issue that I hit even with the patch (and that we probably cannot address with this particular strategy).  If you encounter anything else that I haven't discovered, it'd be great if you could note it on the wiki and/or on this bug.

Thanks!

Comment 19

[Karl Dubost💡 :karlcow]

Some other issues I see in Webkit CSS. 

Examples: 

# Yahoo! Japan (Japan)
https://bugzilla.mozilla.org/show_bug.cgi?id=956972

@media(min-resolution:144dpi),(-webkit-min-device-pixel-ratio:1.5) {…}
@-webkit-keyframes rotate {
    0%{ -webkit-transform:rotate(0deg) }
    etc.
    }


# Hao123 (China)
https://bugzilla.mozilla.org/show_bug.cgi?id=1094555

background:#f8f8f8 -webkit-gradient(linear,0 0,0 100%,from(#fcfcfc),to(#f5f5f5))
-webkit-transform:translateX(-1em)
@-webkit-keyframes myrotate{}
-webkit-animation-name:myrotate;
-webkit-animation-duration:.8s;
-webkit-animation-fill-mode:both;
-webkit-animation-timing-function:ease-in


# QQ (China)
https://bugzilla.mozilla.org/show_bug.cgi?id=866577

-webkit-transition:opacity .2s ease-in-out;


I'll see if I find other things later

Comment 20

[Daniel Holbert [:dholbert]]

Thanks, Karl! Could you clarify whether (& how) those styles cause noticeable issues in these pages?  

(I think you'll agree, we shouldn't simply emulate support for every prefixed property that a site in our fixlist happens to use.  Rather, we should only emulate prefixed properties if doing so provides a noticeable benefit on one or more of the sites on our fixlist [or equivalently, if there's some obvious brokenness caused by not emulating the property].)

Also, I want to be sure our fix-list here stays as limited as possible.  Is Yahoo Japan actually in-scope for getting this emulation treatment?  I'd assume that we'd be able to get them to fix their site, particularly now that we've got a search deal with Yahoo US.  Though I suppose bug 956972 is rather lacking in comments from Yahoo folks, so maybe we have reached out & aren't having any more luck there than with these Chinese sites...

Comment 21

[Peipei Cheng (needinfo if you need my action)]

> Peipei, could you (or someone on your team) test one of these builds on your
> top 10 fixlist china sites & let me know how things look? Note that you need
> to toggle the pref "layout.css.unprefixing-service.enabled" in
> "about:config" before this will work.

Xueqin will help test this.

Comment 22

[xshen]

Attachment: SansumgS2-shucheng.jpg

Comment 23

[xshen]

Attachment: GT-N5110-Chrome.png

Comment 24

[xshen]

Attachment: GT-N5110-Firefox.png

Comment 25

[xshen]

I have verified the top 10 sites and everything is OK except two things:

1)shucheng.baidu.com

Swipe to the left, we can see extra page not applied for the width of the mobile.

Especially, scroll down to the middle of the whole page. There are 4 previews, 2 previews are out of the screen width.

Please see the screenshot captured on Samsung S2:
https://bug1107378.bugzilla.mozilla.org/attachment.cgi?id=8561840

2)hao123.com

Minor issue: the navigator panel below the search box is moved downward a bit compared with Chrome which may results in the panel is covered on some devices.

Please see the screenshot captured on GT-N5110:
Chrome: https://bug1107378.bugzilla.mozilla.org/attachment.cgi?id=8561844
Firefox: https://bug1107378.bugzilla.mozilla.org/attachment.cgi?id=8561846

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to xshen from comment #25)
> I have verified the top 10 sites and everything is OK except two things:
> 
> 1)shucheng.baidu.com
> 
> Swipe to the left, we can see extra page not applied for the width of the
> mobile.

Thanks -- I'll look into this.

> 2)hao123.com
> 
> Minor issue: the navigator panel below the search box is moved downward a
> bit compared with Chrome which may results in the panel is covered on some
> devices.

Yup, I do see a slight difference in your screenshots, in the positioning of the text in the navbar below the search bar.  To me, it just looks like we're using a slightly different font than Chrome, and so there are slightly different glyph sizes & baselines, & hence slightly different layout.

Unless I'm missing a larger issue, I don't think that's worth worrying about as part of this bug (though if we do find cases where it actually causes overlap or other broken layout that doesn't happen in Chrome, it'd be worth filing a text-layout bug on it & CC'ing jfkthame or jdaggett).

Comment 27

[Karl Dubost💡 :karlcow]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> Thanks, Karl! Could you clarify whether (& how) those styles cause
> noticeable issues in these pages?  

good grief about fixing what is necessary.
webkit gradient are useful to fix. The reason is that in many sites they are used without a fallback and we often got white text on white background, because the webkit gradient was not applied. In the case of hao123: There is at least one gradient in the home page where it has a visible effect.

In `.img_bg`, without the gradient we get white text on light gray, when we apply the gradient we fix it with a darker background (accessibility, readability). Note there are additional issues which are related to JS. 
The `Haotab.prototype` is full of Webkit JS related to CSS in index_fcc8000.js


> Also, I want to be sure our fix-list here stays as limited as possible.  Is
> Yahoo Japan actually in-scope for getting this emulation treatment?

Not yet. We have Japanese Web sites failing the same way than Chinese Web sites because of using only WebKit CSS.

>  I'd
> assume that we'd be able to get them to fix their site, particularly now
> that we've got a search deal with Yahoo US. 

Yahoo! Japan != Yahoo! USA. It's confusing but it's a common mistake. :)
See https://en.wikipedia.org/wiki/Yahoo!_Japan


> Though I suppose bug 956972 is
> rather lacking in comments from Yahoo folks, so maybe we have reached out &
> aren't having any more luck there than with these Chinese sites...

We did reach out. The answer was "Thank you, because our site breaks on Firefox Android, Firefox OS, we will send you the desktop Web site." 
Acknowledging without saying no. It's a very common way of handling things in Japan. ;)

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to Karl Dubost :karlcow from comment #27)
> webkit gradient are useful to fix. [...] In the case of hao123: There is
> at least one gradient in the home page where it has a visible effect.
> 
> In `.img_bg` without the gradient we get white text on light gray

Ah, yes - I see this.  OK - I expect we can co-opt "createFixupGradientDeclaration" from hallvors' "css-fixme" tool to address this.

> Note there are additional issues which are related to JS. 
> The `Haotab.prototype` is full of Webkit JS related to CSS in
> index_fcc8000.js

Yup -- for reference, that script is here:
 http://s0.m.hao123img.com/static/html5-index/js/index_fcc8000.js

Fortunately, it seems to be limited to "-webkit-transform" and "-webkit-transition", and it seems to be setting styles as strings -- they're doing things like:
  $con.css({"-webkit-transform":"translate(-"+i*n.singleWidth+"%,0) translateZ(0)",
            "-webkit-transition":"-webkit-transform "+n.Speed+"ms ease-out 0"}))}

...instead of e.g.
  foo.style.WebkitTransition = "-webkit-transform"


So as long as their "css()" function there behaves in the realm of how I expect it to (by creating a CSS declaration from the given strings), we probably don't have to emulate DOM APIs for ".style.WebkitFoo" or anything to get this site to work. Hopefully I can just extend the unprefixer to add support for "-webkit-transform" and "-webkit-transition" (and to strip "-webkit" prefixes in the value for a "-webkit-transition" property, so that -webkit-transition: -webkit-transform [...]" works.)

> > Is Yahoo Japan actually in-scope for getting this emulation treatment?
> 
> Not yet. We have Japanese Web sites failing the same way than Chinese Web
> sites because of using only WebKit CSS.

OK. I'll probably punt on CSS animations until that site is in-scope, then. (presumably there may be a followup bug for japanese sites?)  I suspect CSS animations will require a bit more special-casing in our C++, and I'd rather not gate the rest of this bug on having animations working, when the sites we'll be limiting this to don't even use animations.

[It looks like hao123 does minimally use animations, per the style you quoted in comment 19, but it looks like it's just for a "refresh" (loading?) throbber, "refresh_rotate" in http://s0.m.hao123img.com/static/html5-index/css/index_2c2ec19.css -- and I haven't actually ever seen that rotation when browsing around their site in Chrome w/ Android UA, so it doesn't seem like something they use very often, if at all.]

Comment 29

[Daniel Holbert [:dholbert]]

RE the shucheng.baidu.com issue from comment 26 & attachment 8561840 [details] -- I didn't hit this exact issue (with images overflowing to the right), but I did see something similar, far down the page, with some text overflowing off the right side (and then getting ellipsized, but not soon enough).

This is basically an instance of bug 1043520 comment 6 -- having...
  flex container > flex item > element with overflow:hidden (and maybe text-overflow:ellipsis)
...and in this scenario, the flex item will refuses to shrink smaller than the min-content width of its child (and doesn't realize that the child can handle overflow itself).

The issue goes away if I add "min-width:0" to the flex item (the parent of the ellipsized thing), but I can't think of a clean way to force this behavior via the CSS Unprefixing Service, in general. (nor am I sure we want to, since -webkit-box may depend on some sort of magic nonzero "min-width" behavior to prevent overflow.  IIRC -moz-box does have some magic nonzero min-width behavior by default (disabled via "min-width:0%"), so I'd sort of expect -webkit-box to as well.)

So, I think the shucheng overflow issue may be something we have to live with, and/or address in a followup.

Comment 30

[Jet Villegas (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #29)
> So, I think the shucheng overflow issue may be something we have to live
> with, and/or address in a followup.

Agreed. I wouldn't hold this up for that last issue. 

I think we should file separate bugs for:

1. the fixlist implementation 
2. the fixlist entries 

...and constrain this bug to the implementation of the layout.css.unprefixing-service.

Comment 31

[Daniel Holbert [:dholbert]]

(In reply to Jet Villegas (:jet) from comment #30)
> I think we should file separate bugs for:
> 
> 1. the fixlist implementation 
> 2. the fixlist entries 

Agreed.

I also want to be sure we don't ship this bug's code *until* we have the fixlist implementation, though -- supporting these properties *outside* of the fixlist is a non-goal here, so we don't want to accidentally do that [via a magic pref that users can tweak & make the whole web compatible, while subtly breaking stuff].

With that in mind: given that merge day in just over a week (and given that I'm on PTO tomorrow, and next Monday is a holiday in the US), I think it'd be wise to hold off on landing anything here until just after the next merge, so that it has the whole Fx 39 nightly cycle to bake.  That should also give us time to get the fix list entries solidified & fix any fallout without having to worry about backports.

(In reply to Daniel Holbert [:dholbert] from comment #28)
> (In reply to Karl Dubost :karlcow from comment #27)
> > webkit gradient are useful to fix. [...]
>
> OK - I expect we can co-opt
> "createFixupGradientDeclaration" from hallvors' "css-fixme" tool to address
> this.

So, it actually turns out we've got two stumbling blocks for "gradient" support:
 (1) I'd forgotten that "-webkit-gradient" appears as *part of the value* of a *non-prefixed* property -- "background"/"background-image". So, this needs some extra C++ special cases, to get it into the CSS unprefixing service. (I've got a WIP patch locally that handles this, though, so this part isn't a big deal.)

 (2) As far as I can tell, hallvors' JS gradient-modernizing code doesn't accept the gradient syntax in comment 19. e.g. if I paste a style rule with that gradient value into http://hallvord.com/temp/moz/cssfixme.php and hit the button, it doesn't add an unprefixed version.  So, that JS likely needs some debugging/tweaking to handle this particular syntax.

So, this means that even if we imported the css-fixme gradient-handling code right now, it doesn't fix the one instance of a gradient that we know about on our fixlist -- so I think this is worth investigating/handling separately from the rest of this bug. Hence, I intend to spin this off -webkit-gradient support into a followup.

Comment 32

[Hallvord R. M. Steen [:hallvors]]

>  (2) As far as I can tell, hallvors' JS gradient-modernizing code doesn't
> accept the gradient syntax in comment 19.

Fixed. (The bug was in some scaffolding code trying to figure out if there is an equivalent style rule - a regexp used * instead of +. The output from the createFixupGradientDeclaration method was actually OK, I believe).

Comment 33

[Daniel Holbert [:dholbert]]

That was quick, thanks!

I just filed some followups/helper-bugs:
 * bug 1132743 covers the fixlist implementation.
 * bug 1132745 covers finalizing the fixlist.
 * bug 1132748 covers support for -webkit-gradient. (hopefully easy given comment 32!)
 * bug 1132754 covers support for prefixed animation code (-webkit-keyframes, -webkit-animation-*).

Comment 34

[Daniel Holbert [:dholbert]]

I'm splitting this bug into 3 patches:
 - part 1: Create the JS-implemented service (with no clients yet).
 - part 2: Add pref-controlled code in nsCSSParser that calls out to the service.
 - part 3: mochitest

I'll post a rollup patch, too, in case it helps w/ reviewing to have everything all together for easy searching.

Comment 35

[Daniel Holbert [:dholbert]]

Attachment: part 1: create JS-implemented CSS Unprefixing Service

Comment 36

[Daniel Holbert [:dholbert]]

Attachment: part 2: make parser record decl. & pass it to unprefixing service, when it hits prefixed property-name

Comment 37

[Daniel Holbert [:dholbert]]

Attachment: part 3: treat "display:-webkit-box" as "display: flex" (& same for "-moz-box" if we previously saw "-webkit-box")

I decided to split out "display: -webkit-box" support into its own patch, since it's logically separate from the other nsCSSParser changes (the changes in part 2).  Hopefully this makes it easier to grok & review.

Comment 38

[Daniel Holbert [:dholbert]]

Attachment: part 3 v2: treat "display:-webkit-box" as "display: flex" (& same for "-moz-box" if we previously saw "-webkit-box")

(reuploading part 3, to fix a typo & improve documentation)

Comment 39

[Daniel Holbert [:dholbert]]

Attachment: part 4: mochitest

Comment 40

[Daniel Holbert [:dholbert]]

Attachment: rollup patch (parts 1 - 4 combined)

Comment 41

[Daniel Holbert [:dholbert]]

Try run w/ this patch stack: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58d5a40cad4e

The main changes since my previous Try build (comment 18) are that I've added support for "-webkit-transform", "-webkit-transition", and "-webkit-transition: -webkit-transform".  (There's other code cleanup & refactoring & testing, but nothing else user-visible, as far as I can recall.)

Two notes on the new "transform" & "transition" support:
 - I didn't bother adding support for the "-webkit-transition-*" sub-properties, because I haven't noticed sites actually using those -- everyone just uses the shorthand, from my experience.  But if we find sites relying on the prefixed subproperties, we can easily add support for them.
 - I didn't add support for "-webkit-transition: [generalized -webkit-prefixed property]" -- the only prefixed property we'll support in transitions is "-webkit-transform", for now.  We could add support other "-webkit" prefixed properties inside the value for "-webkit-transition", but I don't think it's worth the complexity until we've found that sites actually rely on to transitioning other prefixed properties. (particularly given that very few of the prefixed properties we're supporting actually have numeric (transitionable) values.)

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8563948 [details] [diff] [review]
part 1: create JS-implemented CSS Unprefixing Service

The IID for nsICSSUnprefixingService should be different from the
CID for the implementation.  So you should change the uuid in the
IDL, but leave the classID in the .js file and the two occurrences
of it in the .manifest file the same.

moz.build:

>+XPIDL_MODULE = 'layout_base'

I guess this is ok; we duplicate XPIDL_MODULE lines elsewhere, e.g.,
with 'dom'

nsICSSUnprefixingService.idl:

>+   * @param   aUnprefixedDecl[out]
>+   *          The resulting unprefixed declaration, if we succeed.
>+   *
>+   * @return true if we succeed, false if we fail. Note that success doesn't
>+   *         necessarily mean aUnprefixedDecl is a valid CSS declaration; it
>+   *         just means we replaced aPropName with a supported property, and we
>+   *         may have converted things we found in aRightHalfOfDecl as well.

I'm a little hesitant about the use of "succeed" here.  How about
using "replace" or "substitute" instead?


CSSUnprefixingService.js:

>+const Cu = Components.utils;

Please also declare Ci as Components.interfaces and use it, and maybe
also just declare Cc as Components.classes by convention.

>+function CSSUnprefixingService() { }

Put the } on its own line in case you need to add to the constructor in
the future.

>+    // Convert our input strings to lower-case, for easier string-matching:
>+    aPropName = aPropName.toLowerCase();
>+    aRightHalfOfDecl = aRightHalfOfDecl.toLowerCase();

Add a comment pointing out that this might be moved into more specific
cases below if we add support for properties that have case-sensitive
parts.

>+    if (typeof(unprefixedPropName) != "undefined") {
...
>+    if (typeof(propInfo) != "undefined") {
...
>+    if (typeof(propInfo) != "undefined") {

Probably better to use if (unprefixedPropName !== undefined).  (Note
two ==, the !== being the inverse of ===.)

>+      if (typeof(mappedKeyword) == undefined) {

This test actually isn't right since the typeof() will be a string.

Instead, use mappedKeyword === undefined.

>+        newRightHalf = newRightHalf.replace(strToReplace, replacement);

Pass "g" as the third parameter to replace so that you replace all
occurrences.


CSSUnprefixingService.js should mention in the comment at the top that
the code is only invoked for sites on a whitelist.


r=dbaron with that

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8563958 [details] [diff] [review]
part 2: make parser record decl. & pass it to unprefixing service, when it hits prefixed property-name

>+  static bool sUnprefixingPrefEnabled;
>+  static bool sUnprefixingPrefEnabledCached = false;
>+
>+  if (!sUnprefixingPrefEnabledCached) {
>+    sUnprefixingPrefEnabledCached = true;
>+    Preferences::AddBoolVarCache(&sUnprefixingPrefEnabled,
>+                                 "layout.css.unprefixing-service.enabled",
>+                                 false);
>+  }
>+  if (!sUnprefixingPrefEnabled) {
>+    return false;
>+  }

Instead of doing this, just add a separate method to call
AddBoolVarCache without any guards, and call that method from
nsLayoutStatics::Initialize.  And then ShouldUseUnprefixingService
can be inline, simply returning a static member.

>+  if (!unprefixingSvc) {
>+    return false;
>+  }

Use NS_ENSURE_TRUE for its warning.

Also probably assert that ShouldUseUnprefixingService is true.


>+        if (!mInSupportsCondition &&
>+            !(aFlags & eParseDeclaration_FromUnprefixingSvc) && // no recursion
>+            ShouldUseUnprefixingService()) {

Please also condition on aContext == eCSSContext_General.

r=dbaron with that

I guess there isn't a need to reparse failed declarations for known
properties?

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8563988 [details] [diff] [review]
part 3 v2:  treat "display:-webkit-box" as "display: flex" (& same for "-moz-box" if we previously saw "-webkit-box")

>+  AutoRestore<bool> autoRestore(mDidUnprefixWebkitBoxInEarlierDecl);

Please assert before these that the member variable is currently false.

r=dbaron with that

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8563993 [details] [diff] [review]
part 4: mochitest

This might be a little bit interesting once you have the fix-list, since enabling the pref won't then be enough to get the behavior.  You might end up needing a "testing" pref that will let the code be used when a site isn't on the fix list -- or a way to temporarily add the mochitest domain to the fixlist.

r=dbaron

Comment 46

[Daniel Holbert [:dholbert]]

Attachment: part 1 v2: create JS-implemented CSS Unprefixing Service

(In reply to David Baron [:dbaron] (UTC+13) (needinfo? for questions) from comment #42)
> Comment on attachment 8563948 [details] [diff] [review]
> part 1: create JS-implemented CSS Unprefixing Service
> 
> The IID for nsICSSUnprefixingService should be different

Fixed -- thanks for catching that! I didn't realize that those IDs were different. (My main reference was https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Building_components_in_JavaScript , which didn't hint that 

> >+XPIDL_MODULE = 'layout_base'
> 
> I guess this is ok; we duplicate XPIDL_MODULE lines elsewhere, e.g.,
> with 'dom'

Yeah -- I was cribbing from layout/base/moz.build, as you probably inferred. :) If you prefer, I'm happy to change this (e.g. to 'layout_style') if it's better organizationally; I wasn't sure if that would require additional boilerplate somewhere.  I'll leave this as-is for now, though.

> nsICSSUnprefixingService.idl:
> 
> >+   * @param   aUnprefixedDecl[out]
> >+   *          The resulting unprefixed declaration, if we succeed.
> >+   *
> >+   * @return true if we succeed,[...SNIP...]
> I'm a little hesitant about the use of "succeed" here.  How about
> using "replace" or "substitute" instead?

Fair. I replaced the first "if we succeed" with "if we return true" (in the aUnprefixedDecl documentation), and I replaced the "@return true" documentation with:

   * @return true if we were able to unprefix -- i.e. if we were able to
   *         convert the property to a known unprefixed equivalent, and we also
   *         performed any known-to-be-necessary fixup on the value, and we put
   *         the result in aUnprefixedDecl.
   *         Otherwise, this function returns false.

> Please also declare Ci as Components.interfaces and use it, and maybe
> also just declare Cc as Components.classes by convention.

Done.

> >+function CSSUnprefixingService() { }
> 
> Put the } on its own line

Done.

> >+    // Convert our input strings to lower-case, for easier string-matching:
> >+    aPropName = aPropName.toLowerCase();
> >+    aRightHalfOfDecl = aRightHalfOfDecl.toLowerCase();

Done.

> Probably better to use if (unprefixedPropName !== undefined).  (Note
> two ==, the !== being the inverse of ===.)

Cool, fixed.

> >+      if (typeof(mappedKeyword) == undefined) {
> 
> This test actually isn't right since the typeof() will be a string.
> 
> Instead, use mappedKeyword === undefined.

Oops! (I guess this doesn't end up impacting behavior, which is why I didn't catch it; we just let bogus keywords survive a bit longer, & get rejected by the CSS parser instead of the prefixing service.)

Fixed with ===, as-suggested.

> >+        newRightHalf = newRightHalf.replace(strToReplace, replacement);
> 
> Pass "g" as the third parameter to replace so that you replace all
> occurrences.

Fixed.

> CSSUnprefixingService.js should mention in the comment at the top that
> the code is only invoked for sites on a whitelist.

Fixed (with an XXXdholbert comment referencing bug 1132743 for the whitelist impl).

Carrying forward r+.

Comment 47

[Daniel Holbert [:dholbert]]

Attachment: part 2 v2: make parser record decl. & pass it to unprefixing service, when it hits prefixed property-name

(In reply to David Baron [:dbaron] from comment #43)
> Instead of doing this, just add a separate method to call
> AddBoolVarCache without any guards, and call that method from
> nsLayoutStatics::Initialize.  And then ShouldUseUnprefixingService
> can be inline, simply returning a static member.

As noted in the XXX comment in ShouldUseUnprefixingService, I'm also intending for that to be the place where we check if our domain is on the fixlist/whitelist. So, it won't quite be a simple return-a-static-member one-liner. So, I'm leaning towards not making it explicitly-inline at this point. We can always add the "inline" keyword later if it makes sense (or trust the compiler to automatically inline it).

But I'm happy to bump the AddBoolVarCache() call to a startup method, to simplify things. I filed bug 1135200 on creating that startup method -- over there, I'm populating it with nsCSSParser's one pre-existing call to AddBoolVarCache(), and then my updated version of this patch here can just adding a new call to that startup function.

> Use NS_ENSURE_TRUE for its warning.

Fixed.

> Also probably assert that ShouldUseUnprefixingService is true.

Fixed.

> >+        if (!mInSupportsCondition &&
> >+            !(aFlags & eParseDeclaration_FromUnprefixingSvc) && // no recursion
> >+            ShouldUseUnprefixingService()) {
> 
> Please also condition on aContext == eCSSContext_General.

Fixed.

> I guess there isn't a need to reparse failed declarations for known
> properties?

Right -- I don't think there is such a need, in general. The only property where we currently would need to do that is "display", with value "-webkit-box", and that's easier to handle as a one-off, as shown in "part 3" here.

Carrying forward r+.

Comment 48

[Daniel Holbert [:dholbert]]

Attachment: part 3 v3: treat "display:-webkit-box" as "display: flex" (& same for "-moz-box" if we previously saw "-webkit-box")

(In reply to David Baron [:dbaron] (UTC+13) (vacation, returning March 2) from comment #44)
> >+  AutoRestore<bool> autoRestore(mDidUnprefixWebkitBoxInEarlierDecl);
> 
> Please assert before these that the member variable is currently false.

Done.

> r=dbaron with that

Thanks -- carrying forward r+.

Comment 49

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] (UTC+13) (vacation, returning March 2) from comment #45)
> This might be a little bit interesting once you have the fix-list, since
> enabling the pref won't then be enough to get the behavior.  You might end
> up needing a "testing" pref that will let the code be used when a site isn't
> on the fix list -- or a way to temporarily add the mochitest domain to the
> fixlist.

(Yup. I was planning on just hardcoding a predefined mochitest helper-domain into the whitelist, actually; I'm a bit uneasy about exposing any programmatic way to add to the whitelist. Anyway, we can sort that out in bug 1132743.)

> r=dbaron

Thanks!

Comment 50

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f510321fe1e

Landed:
part 1:   https://hg.mozilla.org/integration/mozilla-inbound/rev/23fb39cb0f97
part 2:   https://hg.mozilla.org/integration/mozilla-inbound/rev/960037d0fc98
part 3:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5a8d5e8ff524
part 4:   https://hg.mozilla.org/integration/mozilla-inbound/rev/44144b892414

Comment 51

[Daniel Holbert [:dholbert]]

[Tweaking summary to make it clearer that the "fixlist" part is coming in a separate bug: bug 1132743]

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23fb39cb0f97
https://hg.mozilla.org/mozilla-central/rev/960037d0fc98
https://hg.mozilla.org/mozilla-central/rev/5a8d5e8ff524
https://hg.mozilla.org/mozilla-central/rev/44144b892414

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed these out on inbound in a desperate attempt to figure out what caused the mochitest-dt bustages that appeared around the same time as this originally landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68df163b1792

https://treeherder.mozilla.org/logviewer.html#?job_id=7049812&repo=mozilla-inbound

Comment 54

[Daniel Holbert [:dholbert]]

The failure in that log is an exception that was thrown:
{
INFO TEST-UNEXPECTED-FAIL | browser/devtools/netmonitor/test/browser_net_sort-03.js | A promise chain failed to handle a rejection: - undefined
19:28:46 INFO - Stack trace:
19:28:46 INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: PendingErrors.register :: line 162
19:28:46 INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.completePromise :: line 675
19:28:46 INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 903
19:28:46 INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 746
19:28:46 INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.scheduleWalkerLoop/< :: line 688
19:28:46 INFO - native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 
}
devtools/netmonitor/test/browser_net_sort-03.js

This bug's changes are extremely unlikely to have been able to cause a failure w/ promise-handling in a JS file, for a networking devtools test.

If the backout helps, great -- but I think we may need to back something else out to actually fix this issue.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Re-landed, sorry for the churn.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb4cdcbb5c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f008bde94e23
https://hg.mozilla.org/integration/mozilla-inbound/rev/57410350de4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/81233d5e1394

Comment 56

[Daniel Holbert [:dholbert]]

Thanks!

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dcb4cdcbb5c2
https://hg.mozilla.org/mozilla-central/rev/f008bde94e23
https://hg.mozilla.org/mozilla-central/rev/57410350de4d
https://hg.mozilla.org/mozilla-central/rev/81233d5e1394

Comment 58

[Karl Dubost💡 :karlcow]

# Another candidate

@media (-webkit-min-device-pixel-ratio:2) {
  …
  }

can be rewritten as: 

@media only screen and (-webkit-min-device-pixel-ratio: 2), 
       only screen and (min-resolution: 192dpi), 
       only screen and (min-resolution: 2dppx) {
  …
  }


Some examples:

* https://webcompat.com/issues/1056
* https://webcompat.com/issues/1018
* https://bugzilla.mozilla.org/show_bug.cgi?id=1149862

Comment 59

[KastaQQ]

Hmm...
@media screen and (max-width: 1024px) {
.content-width { width: 800px; }
}
@media screen and (max-width: 768px) {
.content-width { width: 768px; }
}
@media screen and (max-width: 600px) {
.content-width { width: 600px; }
}
@media screen and (max-width: 414px) {
.content-width { width: 414px; }
}
and 375px then 360 px too for media. Hope this can help you with the media.. I use this for now for mobile version

Thanks.
*http://kastaqq.com