Closed Bug 1337655 Opened 3 years ago Closed Last year

Try disabling moz-prefixed gradient functions by default

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX
mozilla65
Tracking Status
firefox55 --- fixed
firefox65 --- affected

People

(Reporter: xidorn, Assigned: dholbert)

References

(Depends on 3 open bugs, 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.
Is Stylo planning on implementing the -webkit- prefixed gradients?
Thanks.
Whiteboard: [stylo]
Attached file Failing test cases
This makes many reftests fail. If we plan to remove them we can mark them as skip-if(stylo).
Blocks: 1356087
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)
(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)
(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)
(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.
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.
Flags: needinfo?(dholbert)
(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.)
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)
Blocks: unprefix
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)
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 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 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+
Assignee: nobody → xidorn+moz
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
Blocks: 1366146
https://hg.mozilla.org/mozilla-central/rev/991feb1c7873
https://hg.mozilla.org/mozilla-central/rev/8fc8c0fd8391
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1366526
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/748b8d25cc26
Turn on moz-prefixed gradient functions again.
Reopen given that we re-enable it again.
Assignee: xidorn+moz → nobody
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
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)
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)
Attachment #8868870 - Attachment is obsolete: true
Attachment #8868871 - Attachment is obsolete: true
This doesn't have high priority now, though, because -moz-prefixed gradient has been implemented for stylo :)
Actually... we may need to take extra effort to support the pref in stylo... Are we checking it in Stylo currently?
(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.
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)
Priority: -- → P3
Untracking from stylo, since we eventually implemented -moz-prefixed gradient in stylo.
Whiteboard: [stylo]
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)
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.
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.
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
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
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.
Side note: moz-phab didn't link up the two parts here -- I had to do that manually. I filed bug 1511109 on that.
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
https://hg.mozilla.org/mozilla-central/rev/11965d6a2fca
https://hg.mozilla.org/mozilla-central/rev/54649cf34d98
Status: REOPENED → RESOLVED
Closed: 3 years agoLast year
Resolution: --- → FIXED
Target Milestone: mozilla55 → mozilla65
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.
Thank you for the heads up, and for proactively addressing that instance of breakage.
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...)
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
Depends on: 1512577
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).
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.
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
Can someone file an issue on https://github.com/whatwg/compat given this seems to practically be part of the web platform? 🙁
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: Last yearLast year
Resolution: --- → WONTFIX
See Also: → 1547939
See Also: → 1557110
You need to log in before you can comment on or make changes to this bug.