In this test case, background-image is `-moz-linear-gradient(bottom, red, blue)`. Specified value of this linear gradient is right. It is `-moz-linear-gradient(center bottom , red, blue)` but computed value is `-moz-linear-gradient(rgb(255, 0, 0), rgb(0, 0, 255))`. This value should be `-moz-linear-gradient(50% 100%, rgb(255, 0, 0), rgb(0, 0, 255))` instead.

Comment on attachment 8898622 [details] -moz-linear-gradient testcase Nazim, is this a Stylo bug?

I see "-moz-linear-gradient(50% 0%, rgb(255, 0, 0), rgb(0, 0, 255))" in Firefox 55 and Nightly 57, so this is not a new bug or a Stylo bug. The attached test case doesn't work in Chrome or Edge.

Oops, actually testcase was wrong. The gradient should be `bottom` not `top`. Fixed it. This is a gecko bug. But since we use the gecko computed value serialization on both stylo and gecko, it affects both of them.

I think there is a stylo bug involved here, and the problem is that stylo thinks the default start side to fill in here should be is "bottom", but really the default should be "top". Or something like that. This causes a rendering change, as shown in bug 1395189.

In particular: I think the observed behavior [the computed style leaving out any representation of a side keyword] would be correct, if only "bottom" were the default for vendor-prefixed gradients -- and Firefox-with-stylo seems to think "bottom" is the default, as shown in bug 1395189. But it's not supposed to be the default, per Firefox/Chrome behavior on these prefixed gradients, and per https://www.w3.org/TR/2011/WD-css3-images-20110217/#gradients ("It [the gradient line] may be omitted; if so, it defaults to ‘top’")

Yes, that was a bug on stylo but the other bug seems a different bug. This bug is not only for Firefox with stylo, gecko also thinks that `-moz-linear-gradient(bottom, red, blue)` is the default value and try to remove bottom keyword while serializing.

Yup, you're right. The issue is here: http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/style/nsComputedDOMStyle.cpp#2136,2150-2152,2156-2159 We return early if we see the "bottom" keyword (since 'to bottom' is the default in the modern syntax). We should really extend that early-return to check for yValue = 0.0 ("top") if mLegacySyntax is set.

Regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a8f682801a6d&tochange=1751d97cc9e4 Looks like a regression from bug 752187.

(older mozregression-spawned builds [2011-era at least] didn't like the previous testcase - they needed an explicit "var foo". Here's a testcase with that tweaked for use with older builds.)

I tried to add `-moz-linear-gradient(bottom, red, blue)` but was giving an error inside property_database.js in bug 1391432 and I had to change it to `-moz-linear-gradient(top, red, blue)`. Now we can add this and -webkit- one back. We can't add a reftest probably since this is a serialization bug.

Well, that's cleaner.

Comment on attachment 8902923 [details] Bug 1391534 - Fix computed prefixed linear gradient direction serialization https://reviewboard.mozilla.org/r/174646/#review179740 Do the included tests (the tweaks to property_database.js) cause test failures without the patch? (I'm hoping they do -- but if not, we might need to add a more thorough test to e.g. test_computed_values.html.) ::: commit-message-1b4c5:1 (Diff revision 1) > +Bug 1391534 - Fix computed prefixed linear gradient direction serialization r?dholbert > + > +Default direction of linear gradient functions is `top` but we were trying to remove > +`bottom` keyword from computed values. That was making the serialized gradient wrong. The extra lines here are nice, but a bit too vague to be helpful. Could you add a bit more detail? (RE the distinction between the implicit "from" syntax in prefixed gradients, vs. the explicit "to" syntax in modern gradients, and how that means we need to check against the opposite position in prefixed vs. unprefixed gradients to see if we're using the default bottom-to-top direction or not) ::: layout/style/nsComputedDOMStyle.cpp:2150 (Diff revision 1) > - if (yValue == 1.0f && xValue == 0.5f) { > - // omit "to bottom" > + if ((yValue == 1.0f && xValue == 0.5f && !aGradient->mLegacySyntax) || > + (yValue == 0.0f && xValue == 0.5f && aGradient->mLegacySyntax)) { The xValue == 0.5f check is redundant -- can you split that out to avoid duplicated code and to make it clearer what actually differs? Something like: if (xValue == 0.5f && ((yValue == ... && !aGradient... (yValue == ... && aGradient...)) { } ::: layout/style/nsComputedDOMStyle.cpp:2251 (Diff revision 1) > // linear-gradient() or -webkit-linear-gradient() > AppendCSSGradientToBoxPosition(aGradient, aString, needSep); > } else if (aGradient->mBgPosX.GetUnit() != eStyleUnit_Percent || > aGradient->mBgPosX.GetPercentValue() != 0.5f || > aGradient->mBgPosY.GetUnit() != eStyleUnit_Percent || > - aGradient->mBgPosY.GetPercentValue() != (isRadial ? 0.5f : 1.0f)) { > + aGradient->mBgPosY.GetPercentValue() != (isRadial ? 0.5f : 0.0f)) { Please add a comment here to explain this condition. e.g. // [-vendor-]radial-gradient -moz-linear-gradient, with // non-default box position, which we output here.

Comment on attachment 8902923 [details] Bug 1391534 - Fix computed prefixed linear gradient direction serialization https://reviewboard.mozilla.org/r/174646/#review179740 > Please add a comment here to explain this condition. > > e.g. > // [-vendor-]radial-gradient -moz-linear-gradient, with > // non-default box position, which we output here. (sorry, I was missing an "or" before "-moz" in my sample-comment here. Anyway, this isn't meant to be perfect, just an idea)

Thanks for the review. Addressed the comments.

Mochitest run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be067c5a9a8b It's possible the test failures there are from an existing separate bug (and might just be a serialization-consistency thing, rather than a more serious issue... not sure) If those test failures aren't regressions from this patch, then you might consider punting on those new property_database.js lines and instead test this bug in a more targeted way, by extending (or duplicating) this test code: https://dxr.mozilla.org/mozilla-central/rev/04b6be50a2526c7a26a63715f441c47e1aa1f9be/layout/style/test/test_computed_style.html#612-618 ...with explicit specified/expected value tuples. And then in that case, you'd want to file a followup bug about the test failures you ran into when extending property_database.js (probably with a dummy property_database.js patch as the initial "testcase").

Yes, you're right. I will update the test case and file a bug. Also test failures on stylo builds look like related to bug 1395189. We should wait for it to land first. (It's in the servo merge queue for one day :( )

Actually, I think we don't need to file another bug. This failure is the behavior we want. The failed test is `-moz-linear-gradient(top, red, blue)`. During serialization it becomes `-moz-linear-gradient(rgb(255, 0, 0), rgb(0, 0, 255))` which is what we want. And when roundtripping, since `-moz-linear-gradient(rgb(255, 0, 0), rgb(0, 0, 255))` is a valid unprefixed gradient syntax, it's removing the prefix.

(In reply to Nazım Can Altınova [:canaltinova] from comment #22) > Actually, I think we don't need to file another bug. This failure is the > behavior we want. I don't think it quite is the behavior we want. I suspect this mochitest test has some justification for what it's asserting. We probably want computed values to be "steady states", or something like that. hg blame on the relevant test code might have more information. > The failed test is `-moz-linear-gradient(top, red, blue)`. During > serialization it becomes `-moz-linear-gradient(rgb(255, 0, 0), rgb(0, 0, > 255))` which is what we want. And when roundtripping, since > `-moz-linear-gradient(rgb(255, 0, 0), rgb(0, 0, 255))` is a valid unprefixed > gradient syntax, it's removing the prefix. Right, these are all valid ways of expressing the same gradient, and each individual conversion makes sense on its own; but as a whole, it ends up meaning that we're kind of unstable in how we represent this. I suspect we need to change what happens during parsing, at one stage or the other -- e.g. (a) interpret "-moz-linear-gradient(top, red, blue)" the same way we parse that same expression if the "top" keyword were missing (into a valid unprefixed syntax), ...or: (b) preserve the -moz prefix in more cases (including in the final step of what you described) I prefer (a), but in any case, I think we need to do something for this, and it probably wants to be in a followup.

Comment on attachment 8902923 [details] Bug 1391534 - Fix computed prefixed linear gradient direction serialization https://reviewboard.mozilla.org/r/174646/#review180638 r=me with two extended-commit-message tweaks, and with followup filed as discussed. ::: commit-message-1b4c5:6 (Diff revision 4) > +Bottom-to-top direction is the default value for gradients. Therefore `top` is default > +value of prefixed linear gradients and `to bottom` is default one for unprefixed one. s/Bottom-to-top/Top-to-bottom/ ::: commit-message-1b4c5:8 (Diff revision 4) > + > +Prefixed linear gradients use direction keyword to indicate starting point of the > +gradient but modern syntax uses this keyword to indicate ending point of the gradient. > +Bottom-to-top direction is the default value for gradients. Therefore `top` is default > +value of prefixed linear gradients and `to bottom` is default one for unprefixed one. > +We were trying to remove `bottom` keyword from prefixed computed values which is wrong. Please extend this last sentence slightly, to make it clearer what you're talking about RE removal here. "For brevity, we omit the direction keyword from our serialization when it matches the default direction, but we were incorrectly trying to remove [...]"

Addressed the comments and filed bug 1396102 for that. Thanks for the review!

Pushed by canaltinova@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3f97c8d93c68 Fix computed prefixed linear gradient direction serialization r=dholbert

Seems like something we can let ride the 57 train given how old the issue is. Feel free to set the status for 56 back affected and nominate a rebased patch for approval if you feel otherwise, however.