Closed Bug 1259345 Opened 4 years ago Closed 3 years ago

Let layout.css.prefixes.webkit ride the trains

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- disabled
firefox49 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file)

Right now, layout.css.prefixes.webkit defaults to "true" on Nightly/Aurora, and "false" on Beta/Release (as of bug 1238827).

Filing this bug on letting it be unconditionally true, once it's ready to ride the trains.

For any bugs that block us from shipping webkit prefix support, we should mark them as blocking this bug.
Note: when we do this, we should probably go through all the sites on the CSSUnprefixingService whitelist* and make sure they're look just as good with layout.css.prefixes.webkit=true as they do with  layout.css.prefixes.webkit=false.

(Our "native" webkit prefix support will supercede the CSSUnprefixingService, and we want to be sure that it's strictly an improvement & won't regress the rendering of any of the sites that currently depend on CSSUnprefixingService for correct rendering in release builds.)

* http://mxr.mozilla.org/mozilla-central/source/caps/nsPrincipal.cpp?rev=febf0e69c996#469
Blocks: 1259348
Depends on: 1236979, 1241021, 1237101
Depends on: 1238580
Depends on: 1247777, 759568, 1248708
Depends on: 1239799
Daniel, do you want me to create a list of sites which are "fixed by layout.css.prefixes.webkit=true" Not all of them are in the list 
https://dxr.mozilla.org/mozilla-central/source/caps/nsPrincipal.cpp?rev=febf0e69c996#469
Maybe, if you like! That's orthogonal to this bug, though.

This bug isn't about cases where layout.css.prefixes.webkit=true helps -- it's about fixing cases where layout.css.prefixes.webkit=true *breaks sites*. Once all such issues in that category are fixed, we can ship the pref and be confident that we're giving users a strictly better experience.

My point with comment 1 is that this pref supercedes the (older, crufty) CSSUnprefixingService, so for any sites that currently depend on the CSSUnprefixingService, we should probably make sure that the newer "native" support really is at least as good as the CSSUnprefixingService-provided emulation that we're currently shipping in our release builds.
(In reply to Daniel Holbert [:dholbert] from comment #1)
> Note: when we do this, we should probably go through all the sites on the
> CSSUnprefixingService whitelist* and make sure they're look just as good
> with layout.css.prefixes.webkit=true as they do with 
> layout.css.prefixes.webkit=false.

I'm running my url-screenshot addon on the whitelist right now. I'll post a link to a page so we can compare the true/false results when it's ready (or, rather, when digital ocean solves its DNS problem so I can actually ssh into my site >_>).
You're awesome -- thanks!

(One thing to watch out for -- IIRC the explicitly-whitelisted domains aren't exactly the same as the actual sites that they exist to fix.  There were some cases where www.foo.com pulled its stylesheets from cdn.foobar.com, which meant the whitelist contained cdn.foobar.com. In cases like that, we'd still want to screenshot/inspect www.foo.com, not the whitelist-entry cdn.foobar.com.)
(yeah, seems like we (or probably you :p) were good about leaving a comment for the intended domain, so i've made sure to request actual sites and no CDNS, subdirs, etc.)
OK. Results are at the link in the next sentence. But only click on it if you're fine with 145MB of images being downloaded.

https://miketaylr.com/bzla/comparisons/comparison-03-24-16.html

Notes:

→bellemaison.jp: The semi-transparent thing on bellemaison.jp is the result of timing of the screenshot.
It's fine when loaded on a page (for both modes).

→ks.baidu.com, shucheng.baidu.com -- the install this app button (I'm guessing) at the top looks slightly different than in chrome. lots of things are fixed with webkit:true that are broken with webkit:false. 

→m.mogujie.com -- note: the strange looking pink button with a pink x is not a bug-- that hugs the bottom viewport and the screenshot is of the entire site. a number of layout bugs are fixed with webkit:true that exist with wekit:false.

→mixi.jp - the different in the ad rendering is just a timing thing. looks ok when loading again.

→sina.cn - the missing ad for webkit:true is just a timing thing.

→wappass.baidu.com - captcha image is wonky for both, but I can't reproduce again locally? O-o

→www.kuronekoyamato.co.jp - webkit:true has better gradient support.

→www.ntv.co.jp - we're missing the orange callout bg image. I'm guessing it's border-image quirks that the prefixing service fixed.

→www.sapporobeer.jp - with webkit:true, headers/captions to images now appear.

→www.yamada-denkiweb.com - with webkit:false, the horiz slider images are vertical and I can't scroll. with webkit:true, they're horizontal but TINY. We need to investigate further here.

So I only see 3 sites to investigate further are: yamada-denkiweb.com, ks.baidu.com (probably flexbox related, we should re-visit when bug 1238580 & bug 1256664 land.), and ntv.co.jp (probably border-image quirk). 

(Note that Chrome is dropping border-image quirks in M51, so I kinda hope their developers notice that quickly and fix it.)

All things considered, I think we're in good shape wrt to how we behave with webkit:true vs the unprefixing service (webkit:false).
(In reply to Mike Taylor [:miketaylr] from comment #7)
> →www.yamada-denkiweb.com - [...] with webkit:true, they're horizontal but TINY.
> We need to investigate further here.

They get non-tiny if I add "flex-shrink:0", so this is a version of bug 1256664 & friends. (known issue, which I plan to fix soon.

Thanks a ton for doing that investigation, Mike!
I filed another issue to enable background-clip-text(bug 1263516). I think this one may depend on the new one, instead of bug 759568.
Depends on: 1263516
No longer depends on: 759568
Depends on: 1264210
Depends on: 1261578
Assignee: nobody → dholbert
Depends on: 1264905
No longer depends on: 1263516
Blocks: 658467
No longer blocks: 658467
Attached patch fixSplinter Review
Here's the patch -- though, we shouldn't take this until we've taken the fix on bug 1264905.

(And similarly, we shouldn't request uplift here until we've gotten bug 1264905 uplifted.)
Attachment #8747985 - Flags: review?(cku)
Attachment #8747985 - Flags: review?(cku) → review+
Blocks: 1229757
Having -webkit- prefixes work in Aurora (Developer Edition) but break on release builds does sound pretty strange. If anything, it should be the opposite way around.
(In reply to Yuhong Bao from comment #11)

-webkit prefix support is a new feature in Firefox.

With most new features, we let them prove themselves (and we watch for regressions & fix them) by enabling the feature on Nightly and Aurora for a release or two, for testing, before we let it ride the trains to release builds.

This bug is simply about declaring this feature ready-to-ship to release builds.
(If you think something is strange here and should be the opposite, I think you might be misunderstanding what's actually going on.)
https://hg.mozilla.org/mozilla-central/rev/a33d612c1ed6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1274096
Just talked to Jet -- in the intrests of getting broader early testing for this feature (webkit prefix support/emulation), we'd like to do a short trial (maybe 2 weeks) on beta, and then disable it well before it hits release.

(We don't want it to quite let it ride all the way to release in Aurora 48 because there are some known bugs at this point, which are small & not-known-to-be-hit-by-any-real-world-content but worth fixing before release, and we anticipate that more bugs will be filed once this gets to a broader audience. Hence, in the interests of getting those bugs filed sooner so we can fix them sooner, we'd like to have this tested by a broader audience sooner.)

So: I'd like to uplift this bug's patch (a pref-unguarding) to Aurora 48, so that it rides the trains to Beta 48, with the understanding that I'll back out (reguarding the pref) during the beta cycle at some point that we'll prearrange with release drivers.
Comment on attachment 8747985 [details] [diff] [review]
fix

Approval Request Comment
[Feature/regressing bug #]: Support for webkit prefixed CSS features & extensions

[User impact if declined]:
- Technically this patch has zero effect for Aurora48 users, since this feature is already enabled on Aurora (but in such a way that it gets disabled when Aurora48 migrates to Beta, via an "#ifdef RELEASE_BUILD" guard).
- If this uplift request is declined, we won't be able to run this trial for the first few weeks of Beta48, which means Beta48 users won't get access to this feature for those few weeks -- they'll experience the same webkit-prefix web compatibility issues they've always experienced (and will continue to experience after the trial).
 - But if it's approved, some sites (particularly mobile sites but desktop sites as well) which were only tested in safari/chrome will start rendering better for our users during the trial period.

[Describe test coverage new/current, TreeHerder]: We have reftests & mochitests for this feature. It's been getting Nightly & testing since Jan 1st (bug 1213126) and Aurora testing since Aurora46.

[Risks and why]:
 - Zero risk to Firefox 48 release users, since I intend to back out this patch before Firefox 48 hits release.
 - During the Beta 48 trial (see comment 16), it's possible/likely that a very small fraction of sites will break due to imperfect emulation, or due to unforseen dependencies between -webkit prefixed CSS that we do support & other features that we don't support.  We've found & fixed some of those sorts of things already.  I'm requesting this backport in the interests of finding out about those things sooner rather than later, via a short Beta48 trial as mentioned in comment 16.

[String/UUID change made/needed]: None.
Attachment #8747985 - Flags: approval-mozilla-aurora?
Comment on attachment 8747985 [details] [diff] [review]
fix

"New" cool feature, taking it.
Should we relnote it for 48?
Attachment #8747985 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Sylvestre Ledru [:sylvestre] from comment #18)
> "New" cool feature, taking it.

Thanks! Note that we need to backport bug 1274096 and bug 1272721 as well, for users to have the lowest likelihood of breakage. (comment 16) -- could you grant backport approval for those as well?

With those bugs, we should be ready for our "beta trial" period. (comment 16)

> Should we relnote it for 48?

(Probably, but just for beta48 (not release), per comment 16.)
Flags: needinfo?(sledru)
Approved too.
Thanks!
relnote-firefox: --- → ?
Flags: needinfo?(sledru)
Daniel: Can you please suggest some wording for the release note? Thanks!
Flags: needinfo?(dholbert)
Blocks: 1277092
Suggested release note:
"For first few weeks of 48 beta, some legacy webkit-prefixed CSS aliases & features will be supported, for improved web compatibility. This support is only enabled for early testing, and will be disabled in later betas and 48 release.)"
Flags: needinfo?(dholbert)
Sylvestre: Do you want to relnote this for 48?
Flags: needinfo?(sledru)
Depends on: 1281936
Daniel, do you know when and how the feature will be disabled in beta? thanks
Flags: needinfo?(sledru) → needinfo?(dholbert)
(In reply to Sylvestre Ledru [:sylvestre] from comment #25)
> Daniel, do you know when and how the feature will be disabled in beta? thanks

Yup -- so, this feature is enabled via the EARLY_BETA_OR_EARLIER flag (I added an #ifdef guard in bug 1277092).

Whenever that flag gets dropped (I'm told that's part of the release-management later-beta process), this feature will go back to being disabled.
Flags: needinfo?(dholbert)
No longer blocks: 1171872
No longer blocks: 1229757
As of bug 1247777 comment 69 (and bug 1277092) the pref is actually off by default in 48.
Daniel, can you please clarify which version will ship with the pref on by default?

Sebastian
Flags: needinfo?(dholbert)
Keywords: dev-doc-needed
Version 49 is the first version that will ship with the pref by default.

(While the patch here *was* backported to 48, it was later restricted to EARLY_BETA_OR_EARLIER in bug 1277092, which means it won't ship. This was the plan all along [disabling on 48 before shipping], as noted at the end of comment 17.)

Perhaps the "firefox48:fixed" status here is confusing & should be adjusted to avoid confusion...
Flags: needinfo?(dholbert)
--> updating firefox48 status to "disabled", since this is preffed off there (for release), via bug 1277092.
Daniel, I won't put that in the release notes until we have some docs on MDN, "some legacy webkit-prefixed CSS aliases & features" is too unclear.
Please need info Liz or me when we have some more information to share.
Flags: needinfo?(dholbert)
Blocks: 1169931
This was covered in the MDN notes for 49, removing the relnote-firefox flag.
relnote-firefox: ? → ---
You need to log in before you can comment on or make changes to this bug.