Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 7 years ago

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.

	Mike Taylor [:miketaylr]
	Comment 1 • 7 years ago

Is Stylo planning on implementing the -webkit- prefixed gradients?

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 2 • 7 years ago

Yes. See https://github.com/servo/servo/issues/15441

	Mike Taylor [:miketaylr]
	Comment 3 • 7 years ago

Thanks.

	Shing Lyu [:shinglyu]
	Comment 4 • 7 years ago

This makes many reftests fail. If we plan to remove them we can mark them as skip-if(stylo).

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 5 • 7 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?

	Daniel Holbert [:dholbert] Assignee
	Comment 6 • 7 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.

	Mike Taylor [:miketaylr]
	Comment 7 • 7 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.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 8 • 7 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 9 • 7 years ago

Good point -- agreed.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 10 • 7 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 11 • 7 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.)

	Daniel Holbert [:dholbert] Assignee
	Comment 12 • 7 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 13 • 7 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

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 16 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=186bf8551f26f76bb42f321491475e4c7a54ca6d

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 17 • 7 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.

	David Baron :dbaron:
	Comment 18 • 7 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'?

	David Baron :dbaron:
	Comment 19 • 7 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

	Pulsebot
	Comment 23 • 7 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

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/991feb1c7873
https://hg.mozilla.org/mozilla-central/rev/8fc8c0fd8391

	Pulsebot
	Comment 25 • 7 years ago

Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/748b8d25cc26
Turn on moz-prefixed gradient functions again.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 26 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2f561494aa3cd5ba1933d8c623be16dfd14a86c

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 27 • 7 years ago

Reopen given that we re-enable it again.

	Phil Ringnalda (:philor)
	Comment 28 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/748b8d25cc26

	Daniel Holbert [:dholbert] 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)

	Xidorn Quan [:xidorn] UTC+11 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.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 32 • 7 years ago

This doesn't have high priority now, though, because -moz-prefixed gradient has been implemented for stylo :)

	Xidorn Quan [:xidorn] UTC+11 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?

	Daniel Holbert [:dholbert] 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.

	Xidorn Quan [:xidorn] UTC+11 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 :)

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 36 • 7 years ago

Untracking from stylo, since we eventually implemented -moz-prefixed gradient in stylo.

	BugBot [:suhaib / :marco/ :calixte]
	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?

	Daniel Holbert [:dholbert] 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.

	Daniel Holbert [:dholbert] 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 41 • 6 years ago

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

	Daniel Holbert [:dholbert] 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.

	Daniel Holbert [:dholbert] 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.

	Pulsebot
	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

	Razvan Maries
	Comment 46 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/11965d6a2fca
https://hg.mozilla.org/mozilla-central/rev/54649cf34d98

	Chris Morgan
	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.

	Daniel Holbert [:dholbert] Assignee
	Comment 48 • 6 years ago

Thank you for the heads up, and for proactively addressing that instance of breakage.

	Daniel Holbert [:dholbert] 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...)

	Pulsebot
	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

	Cosmin Sabou [:CosminS]
	Comment 51 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5d2f9b45ad97

	Daniel Holbert [:dholbert] 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).

	Daniel Holbert [:dholbert] 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.

	Daniel Holbert [:dholbert] Assignee
	Comment 54 • 6 years ago

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ae557a428bb1a50478db2fa6a868fb91372856

	Chris Mills [:cmills]
	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.

	Sam Sneddon [:gsnedders]
	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? 🙁

	Daniel Holbert [:dholbert] 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.