Closed
Bug 1259345
Opened 9 years ago
Closed 9 years ago
Let layout.css.prefixes.webkit ride the trains
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
(Keywords: dev-doc-needed)
Attachments
(1 file)
(deleted),
patch
|
u459114
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Comment 2•9 years ago
|
||
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
Assignee | ||
Comment 3•9 years ago
|
||
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.
Comment 4•9 years ago
|
||
(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 >_>).
Assignee | ||
Comment 5•9 years ago
|
||
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.)
Comment 6•9 years ago
|
||
(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.)
Comment 7•9 years ago
|
||
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).
Assignee | ||
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dholbert
Assignee | ||
Comment 10•9 years ago
|
||
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+
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
(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.
Assignee | ||
Comment 13•9 years ago
|
||
(If you think something is strange here and should be the opposite, I think you might be misunderstanding what's actually going on.)
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 16•9 years ago
|
||
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.
Assignee | ||
Comment 17•9 years ago
|
||
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 18•8 years ago
|
||
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+
Comment 19•8 years ago
|
||
bugherder uplift |
Assignee | ||
Comment 20•8 years ago
|
||
(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)
Comment 22•8 years ago
|
||
Daniel: Can you please suggest some wording for the release note? Thanks!
Flags: needinfo?(dholbert)
Assignee | ||
Comment 23•8 years ago
|
||
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)
Comment 25•8 years ago
|
||
Daniel, do you know when and how the feature will be disabled in beta? thanks
Flags: needinfo?(sledru) → needinfo?(dholbert)
Assignee | ||
Comment 26•8 years ago
|
||
(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)
Comment 27•8 years ago
|
||
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
Assignee | ||
Comment 28•8 years ago
|
||
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)
Assignee | ||
Comment 29•8 years ago
|
||
--> updating firefox48 status to "disabled", since this is preffed off there (for release), via bug 1277092.
Comment 30•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(dholbert)
Comment 31•8 years ago
|
||
We have a good overview here:
https://developer.mozilla.org/en-US/Firefox/Releases/49#Compatibility
Flags: needinfo?(dholbert)
Comment 32•8 years ago
|
||
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.
Description
•