1391534 - Computed value of `-moz-linear-gradient(bottom, ...)` is wrong

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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 1

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: -moz-linear-gradient testcase

Comment 2

[Chris Peterson [:cpeterson]]

Comment on attachment 8898622 [details]
-moz-linear-gradient testcase

Nazim, is this a Stylo bug?

Comment 3

[Chris Peterson [:cpeterson]]

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.

Comment 4

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: -moz-linear-gradient testcase

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.

Comment 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

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’")

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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.

Comment 8

[Daniel Holbert [:dholbert]]

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.

Comment 9

[Daniel Holbert [:dholbert]]

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a8f682801a6d&tochange=1751d97cc9e4

Looks like a regression from bug 752187.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (with explicit "var foo" to make older builds happy)

(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.)

Comment 11

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1391534 - Fix computed prefixed linear gradient direction serialization

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.

Review commit: https://reviewboard.mozilla.org/r/174646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174646/

Comment 12

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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.

Comment 13

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8902923 [details]
Bug 1391534 - Fix computed prefixed linear gradient direction serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174646/diff/1-2/

Comment 14

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Well, that's cleaner.

Comment 15

[Daniel Holbert [:dholbert]]

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 16

[Daniel Holbert [:dholbert]]

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)

Comment 17

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8902923 [details]
Bug 1391534 - Fix computed prefixed linear gradient direction serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174646/diff/2-3/

Comment 18

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Thanks for the review. Addressed the comments.

Comment 19

[Daniel Holbert [:dholbert]]

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").

Comment 20

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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 :( )

Comment 21

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8902923 [details]
Bug 1391534 - Fix computed prefixed linear gradient direction serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174646/diff/3-4/

Comment 22

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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.

Comment 23

[Daniel Holbert [:dholbert]]

(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 24

[Daniel Holbert [:dholbert]]

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 [...]"

Comment 25

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8902923 [details]
Bug 1391534 - Fix computed prefixed linear gradient direction serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174646/diff/4-5/

Comment 26

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f97c8d93c68
Fix computed prefixed linear gradient direction serialization  r=dholbert

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3f97c8d93c68

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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.