Closed
Bug 1337655
Opened 8 years ago
Closed 6 years ago
Try disabling moz-prefixed gradient functions by default
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
WONTFIX
mozilla65
People
(Reporter: xidorn, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
(Keywords: site-compat)
Attachments
(3 files, 3 obsolete files)
Given bug 1176496 comment 29, we probably should try disabling moz-prefixed gradient functions now.
We would not need to implement those (somehow undocumented) functions for Stylo if we can remove them now.
Updated•8 years ago
|
Keywords: dev-doc-needed,
site-compat
Comment 1•8 years ago
|
||
Is Stylo planning on implementing the -webkit- prefixed gradients?
Reporter | ||
Comment 2•8 years ago
|
||
Comment 3•8 years ago
|
||
Thanks.
Updated•8 years ago
|
Whiteboard: [stylo]
Comment 4•8 years ago
|
||
This makes many reftests fail. If we plan to remove them we can mark them as skip-if(stylo).
Reporter | ||
Comment 5•8 years ago
|
||
dholbert, miketaylr, do you think it makes sense to simply remove all moz-prefixed gradient code and only leave the webkit-prefixed one? I suppose no code would rely on moz-prefixed gradient without having webkit-prefixed counterpart?
If you think we shouldn't remove that code immediately, probably we can change the serialization code to generate webkit-prefixed form instead of moz-prefixed, and put the parsing code behind a pref?
Flags: needinfo?(miket)
Flags: needinfo?(dholbert)
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #5)
> dholbert, miketaylr, do you think it makes sense to simply remove all
> moz-prefixed gradient code and only leave the webkit-prefixed one?
The moz-prefixed gradient code is already pref-controlled via "layout.css.prefixes.gradients" from bug 1186636. Seems like it'd be strictly simpler to just turn that pref off by default and see how that goes, right?
I'd say we should do that! (Note that the pref *is* "moz"-specific -- it doesn't disable the webkit version of the prefix, though it might sound like it would.)
> no code would rely on moz-prefixed gradient without having webkit-prefixed
> counterpart?
We can hope! It's possible we'll hit things like bug 1183504, but if those issues are rare enough, we'll probably be fine.
Flags: needinfo?(dholbert)
Comment 7•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #5)
> dholbert, miketaylr, do you think it makes sense to simply remove all
> moz-prefixed gradient code and only leave the webkit-prefixed one? I suppose
> no code would rely on moz-prefixed gradient without having webkit-prefixed
> counterpart?
+1, worth a shot.
Flags: needinfo?(miket)
Reporter | ||
Comment 8•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #6)
> (In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May)
> from comment #5)
> > dholbert, miketaylr, do you think it makes sense to simply remove all
> > moz-prefixed gradient code and only leave the webkit-prefixed one?
>
> The moz-prefixed gradient code is already pref-controlled via
> "layout.css.prefixes.gradients" from bug 1186636. Seems like it'd be
> strictly simpler to just turn that pref off by default and see how that
> goes, right?
>
> I'd say we should do that! (Note that the pref *is* "moz"-specific -- it
> doesn't disable the webkit version of the prefix, though it might sound like
> it would.)
It would break lots of tests for webkit-prefixed version though, because IIUC, we currently serialize all prefixed gradient functions to moz-prefixed form. We would need to change that before we can disable moz-prefixed functions.
Assignee | ||
Comment 9•8 years ago
|
||
Good point -- agreed.
Reporter | ||
Comment 10•8 years ago
|
||
dholbert, could you help implementing the serialization part? I'm not quite familiar with the difference between the -moz-prefixed form and the -webkit-prefixed form. It seems simply changing "-moz-" to "-webkit-" in nsCSSValue.cpp and nsComputedDOMStyle.cpp leads to tons of test failures, and I'm not completely sure what should be done to fix them.
I think we only need to support serializing to -webkit-prefixed form, and we can just disable the -moz-prefixed form at the same time.
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(dholbert)
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #10)
> dholbert, could you help implementing the serialization part? I'm not quite
> familiar with the difference between the -moz-prefixed form and the
> -webkit-prefixed form. It seems simply changing "-moz-" to "-webkit-" in
> nsCSSValue.cpp and nsComputedDOMStyle.cpp leads to tons of test failures,
> and I'm not completely sure what should be done to fix them.
I'm on PTO tomorrow, but I can take a look on Monday. (Leaving needinfo open.)
> I think we only need to support serializing to -webkit-prefixed form, and we
> can just disable the -moz-prefixed form at the same time.
(I'd prefer to decouple those two parts into separate changesets at least, if not separate bugs/landings, for more straightforward regression-tracking.)
Assignee | ||
Comment 12•8 years ago
|
||
I believe we should be OK to proceed here, once bug 1357117 (and its dependency bug 1241623) have gotten us to stop using -moz prefixed gradients in our serialization of authors' -webkit prefixed gradients.
Flags: needinfo?(dholbert)
Updated•8 years ago
|
Blocks: stylo-reftest
Assignee | ||
Comment 13•8 years ago
|
||
Xidorn, now that bug 1358710 has landed (getting rid of moz prefixed gradients in our tests), did you want to drive this bug in?
I think it's just the pref-flip, now, plus a followup to your intent to unship thread[1].
[1] https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/egVDMiu86m0
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 16•8 years ago
|
||
Flags: needinfo?(xidorn+moz)
Reporter | ||
Comment 17•8 years ago
|
||
Thanks for your and canaltinova's work on this!
Try run looks fine (the quantum render failures seem to be preexisting permafail). If dbaron agrees, we can land it and see :)
dbaron, part 1 is adding values removed in bug 1358710 back. If you think we don't need that, I can just land the second patch.
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8868870 [details]
Bug 1337655 part 1 - Add moz-prefixed gradient values back in property_database behind the pref.
https://reviewboard.mozilla.org/r/140476/#review144350
::: layout/style/test/property_database.js:7814
(Diff revision 1)
> if (IsCSSPropertyPrefEnabled("layout.css.text-align-unsafe-value.enabled")) {
> gCSSProperties["text-align"].other_values.push("true left");
> } else {
> gCSSProperties["text-align"].invalid_values.push("true left");
> }
While you're here, could you fix that this bit is incorrectly conditioned on 'unset'?
Attachment #8868870 -
Flags: review?(dbaron) → review+
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8868871 [details]
Bug 1337655 part 2 - Turn off moz-prefixed gradient functions for nightly.
https://reviewboard.mozilla.org/r/140478/#review144352
I think you should probably do this for nightly only for now -- so that you can make an explicit decision to evaluate whether any bugs resulted and we can then decide to advance to beta and release. I'd rather not just put this on autopilot to go all the way to release right now.
So r=dbaron on an
#ifdef NIGHTLY_BUILD
...false
#else
...true
#endif
Attachment #8868871 -
Flags: review?(dbaron) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request) |
Comment 23•8 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/991feb1c7873
part 1 - Add moz-prefixed gradient values back in property_database behind the pref. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/8fc8c0fd8391
part 2 - Turn off moz-prefixed gradient functions for nightly. r=dbaron
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/991feb1c7873
https://hg.mozilla.org/mozilla-central/rev/8fc8c0fd8391
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 25•8 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/748b8d25cc26
Turn on moz-prefixed gradient functions again.
Reporter | ||
Comment 26•8 years ago
|
||
Reporter | ||
Comment 27•8 years ago
|
||
Reopen given that we re-enable it again.
Reporter | ||
Updated•8 years ago
|
Comment 28•8 years ago
|
||
bugherder |
Assignee | ||
Comment 29•7 years ago
|
||
Per bug 1182775 comment 21, we might be able to try this again... (if Gmail was the only thing blocking us)
Flags: needinfo?(xidorn+moz)
Reporter | ||
Comment 30•7 years ago
|
||
Yeah, as far as I know, that is the only known regression so far when we do this last time.
I can confirm that I do see correct unprefixed declaration for the button in Gmail.
Will try turning off it again.
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Attachment #8868870 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Attachment #8868871 -
Attachment is obsolete: true
Reporter | ||
Comment 32•7 years ago
|
||
This doesn't have high priority now, though, because -moz-prefixed gradient has been implemented for stylo :)
Reporter | ||
Comment 33•7 years ago
|
||
Actually... we may need to take extra effort to support the pref in stylo... Are we checking it in Stylo currently?
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #33)
> Actually... we may need to take extra effort to support the pref in stylo...
> Are we checking it in Stylo currently?
Apparently we aren't checking it. Testcase: https://jsfiddle.net/q0jxatk7/4/
In stock Nightly, the gradient on that testcase (behind "HI") goes away if I disable layout.css.prefixes.gradients, as expected. But if I flip the stylo pref, then we always render the gradient there, regardless of the value of the "layout.css.prefixes.gradients" pref.
Reporter | ||
Comment 35•7 years ago
|
||
Comment on attachment 8883843 [details]
Bug 1337655 - Turn off moz-prefixed gradient functions for nightly.
Ok, then let's not touch it for now :)
Attachment #8883843 -
Flags: review?(dbaron)
Updated•7 years ago
|
Priority: -- → P3
Reporter | ||
Comment 36•7 years ago
|
||
Untracking from stylo, since we eventually implemented -moz-prefixed gradient in stylo.
Whiteboard: [stylo]
Comment 37•6 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?
Flags: needinfo?(svoisen)
Assignee | ||
Comment 38•6 years ago
|
||
No, this is still something we should do. ("leave-open" is here because this was backed out, but we should reland it when we can, webcompat-wise. Which maybe we can do now/soonish?)
Note, the issue in comment 34 [that held us back last time we were thinking about landing] was fixed in bug Bug 1451874.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(svoisen)
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → dholbert
Assignee | ||
Comment 39•6 years ago
|
||
Assignee | ||
Comment 40•6 years ago
|
||
I've had this pref disabled in my normal browsing profile for quite a while -- long enough that I'd forgotten that I'd disabled it. :)
I haven't run across any issues, so I think this should be safe enough for Nightly, webcompat-wise.
Assignee | ||
Updated•6 years ago
|
Attachment #8883843 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9028405 -
Attachment description: Bug 1337655 - Turn off moz-prefixed gradient functions for nightly. r?emilio → Bug 1337655 - Turn off moz-prefixed CSS gradient functions for nightly. r?emilio
Assignee | ||
Comment 41•6 years ago
|
||
Assignee | ||
Comment 42•6 years ago
|
||
Updated•6 years ago
|
Attachment #9028405 -
Attachment description: Bug 1337655 - Turn off moz-prefixed CSS gradient functions for nightly. r?emilio → Bug 1337655 part 2: Turn off moz-prefixed CSS gradient functions for nightly. r?emilio
Assignee | ||
Comment 43•6 years ago
|
||
Try run shows we've got some moz-prefixed gradient values in property_database.js that aren't guarded by the pref-check (like mots of the moz-prefixed gradient values are there)
Looks like these values were moved in https://hg.mozilla.org/mozilla-central/rev/9791b3b0efc940d1ec5fa07d3e1dd82222135b42#l14.15 from a dedicated "broken in servo" section to a general section, and they were just mistakenly moved to the a more-general section than they should've been.
I'll move 'em to join their (pref-controlled) prefixed bretheren in this bug - see just-posted new patch.
Assignee | ||
Comment 44•6 years ago
|
||
Side note: moz-phab didn't link up the two parts here -- I had to do that manually. I filed bug 1511109 on that.
Comment 45•6 years ago
|
||
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11965d6a2fca
part 1: Move some prefixed gradient values to pref-controlled sections of property_database.js. r=emilio
https://hg.mozilla.org/integration/autoland/rev/54649cf34d98
part 2: Turn off moz-prefixed CSS gradient functions for nightly. r=emilio
Comment 46•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11965d6a2fca
https://hg.mozilla.org/mozilla-central/rev/54649cf34d98
Status: REOPENED → RESOLVED
Closed: 8 years ago → 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla55 → mozilla65
Comment 47•6 years ago
|
||
FYI: the Tree Style Tab extension hit visual problems due to this, which I’ve submitted https://github.com/piroor/treestyletab/pull/2082 to fix. I don’t expect the web at large to be affected by this, because there are other web browsers out there that don’t support the -moz- prefix, but I’m sure there will be a few more extensions that need to be updated.
Assignee | ||
Comment 48•6 years ago
|
||
Thank you for the heads up, and for proactively addressing that instance of breakage.
Assignee | ||
Comment 49•6 years ago
|
||
This has caused one issue with Zimbra (bug 1183994, which we mistakenly thought had been fixed on Zimbra's end, per bug 1183994 comment 8). I'm going to push a followup to narrow this pref-disabling to be Nightly-only while we investigate that, because we don't want to uplift that breakage to Beta.
(Depending on the outcome of the investigation on zimbra & on any other content that we discover is broken over the next chunk of time, maybe we'll need to revert this altogether, but hopefully we can get things fixed to use modern syntax...)
Comment 50•6 years ago
|
||
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2f9b45ad97
followup: Only turn off moz-prefixed gradients on Nightly for now, since some sites still use them. rs=emilio
Comment 51•6 years ago
|
||
bugherder |
Assignee | ||
Comment 52•6 years ago
|
||
We've had 3 regressions filed in ~1 week just from nightly testers (all cases where sites seem to be doing UA sniffing & serving Firefox *only* the moz-prefixed syntax here).
Given this & given that gradient backgrounds don't fail gracefully (the element just ends up disappearing or being unreadable), I'm losing confidence that we can realistically un-ship this without breaking the web. We can fix some sites with outreach, but there may be too many... so I'm tempted to backout here again (i.e. re-enable moz-prefixed gradients in Nightly).
Assignee | ||
Comment 53•6 years ago
|
||
I think we should backout & close this as WONTFIX. The costs... (broken/unusable websites ruining users' days & possibly driving them away; time spent diagnosing & trying to do possibly-fruitless outreach to address these)... seem high, and the benefits (theoretical purity, slight maintenance reduction) seem low.
This bug was premised on the following assumption:
(In reply to Xidorn Quan [:xidorn] UTC+11 from comment #5)
> I suppose
> no code would rely on moz-prefixed gradient without having webkit-prefixed
> counterpart?
...and we've discovered that this assumption is sadly not correct. Each time we've attempted to land here and in bug 1176496, we've quickly gotten reports of broken sites that do UA sniffing & serve us *only* moz-prefixed gradients (no webkit-prefixed, no unprefixed), which is a signal that there's likely a long tail of yet-to-be-discovered sites that have this issue. For this most recent attempt, we've only gotten 3 sites reported, but that's a signal that there are more unreported sites, and probably even more sites that our Nightly users don't use but that our broader beta/release population might.
Bottom line, it looks like moz-prefixed gradients are one piece of the vendor-prefixed web platform that have cemented themselves (for browsers that send out the Firefox UA string at least), due to sizeable numbers of sites doing UA sniffing and failing to serve any fallback/standard syntax.
Assignee | ||
Comment 54•6 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ae557a428bb1a50478db2fa6a868fb91372856
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•6 years ago
|
Comment 55•6 years ago
|
||
Note to MDN Writers:
Looks like this isn't happening after all; I have therefore not added a note to the Fx65 rel notes.
And it looks like -moz- prefixed gradients are recorded properly in the browser compat data.
Keywords: dev-doc-needed
Comment 56•6 years ago
|
||
Can someone file an issue on https://github.com/whatwg/compat given this seems to practically be part of the web platform? 🙁
Assignee | ||
Comment 57•6 years ago
•
|
||
OK, I filed https://github.com/whatwg/compat/issues/111 (though as noted there, I'm not sure other engines/browsers need to care about this wart, since it only matters if UA sniffing determines you to be Firefox).
I'm going to call this WONTFIX, per my thoughts on costs/benefits laid out at the beginning of comment 53.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•