Closed Bug 1396102 Opened 7 years ago Closed 7 years ago

Specified and computed values of `-webkit-linear-gradient(top, ...)` are not consistent

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: canova, Assigned: canova)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1396102 - Add some testcases to verify that we consistently serialize the default line directions in linear gradients (iff stylo is enabled)
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
The specified value of `-webkit-linear-gradient(top, ...)` is preserving the `top` keyword. But its computed value is removing this keyword. That behavior is inconsistent. Re-parsing the specified value gives prefixed value again but, re-parsing the computed value gives unprefixed value since it's modern syntax.
We have two options per bug 1391534 comment 23,
(a) serialize the specified value of `-webkit-linear-gradient(top, ...)` without `top` keyword.
(b) preserve the -moz prefix in more cases.

First one looks better.
This makes certain values trigger some test failures like:
====
parse+compute+serialize should be idempotent for 'border-image-source: -moz-linear-gradient(top, red, blue)'
got "linear-gradient(rgb(255, 0, 0), rgb(0, 0, 255))", expected "-moz-linear-gradient(rgb(255, 0, 0), rgb(0, 0, 255))"
====

...e.g. in the Try run discussed in bug 1391534 comment 19.  (Affects both -moz and -webkit prefixing, I think.)

(It's not idempotent because right now, it gets progressively simplified one step at a time, as Nazim described in comment 0 here.)
I think this was already a problem with -webkit-linear-gradient(bottom,...) / -moz-webkit-linear-gradient(bottom,...), before bug 1391534's patch landed [because that version of the code mistakenly thought that "bottom" was the default).  And now after bug 1391534's patch, it's a problem with "top" instead.
Stylo does the (a), btw. If we try to serialize specified value of `linear-gradient(to bottom, red, blue)` it will print `linear-gradient(red, blue)` and if we try to serialize specified value of `-webkit-linear-gradient(top, red, blue)` it will print `-webkit-linear-gradient(red, blue)`.
It's not the perfect specified value since it's not preserving the given exact input but we can do the same for gecko as well to avoid this inconsistency between computed and specified values.

Also this bug was about prefixed functions because of the mochitest failure but I guess we can convert the unprefixed one as well for the consistency between all linear gradients(like stylo).
Priority: -- → P3
Comment on attachment 8911979 [details]
Bug 1396102 - stylo: Don't serialize default position on -moz- prefixed linear gradient

https://reviewboard.mozilla.org/r/183388/#review188490
Attachment #8911979 - Flags: review?(manishearth) → review+
Pushed to try to see test results. Fixed -moz- prefixed linear gradient serialization on Stylo and fixed the gecko consistency. We can just discard the second patch if it's really unnecessary because of Stylo but I'm not sure
Assignee: nobody → canaltinova
Status: NEW → ASSIGNED
Comment on attachment 8911980 [details]
Bug 1396102 - Add some testcases to verify that we consistently serialize the default line directions in linear gradients (iff stylo is enabled)

https://reviewboard.mozilla.org/r/183390/#review188494

::: layout/style/nsCSSValue.h:1716
(Diff revision 1)
> +    // We should early return false if positions are not keywords since
> +    // only keyword positions can point downwards.
> +    if (mBgPos.mXValue.GetUnit() != eCSSUnit_Enumerated ||
> +        mBgPos.mYValue.GetUnit() != eCSSUnit_Enumerated) {
> +      return false;

This comment ("only keyword positions can point downwards") isn't strictly true.  -moz-linear-gradient can accept a percent-valued starting-position, which can give us a downwards-oriented gradient without any keywords.

Like so:
-moz-linear-gradient(50% 0%, red, blue)
-moz-linear-gradient(50% top, red, blue)
-moz-linear-gradient(center 0%, red, blue)

Live examples:
https://jsfiddle.net/1psf1r18/

The keywords-only exclusion might still be correct/justifiable, but please be sure whatever we do for these no-keywords examples is consistent with/without stylo, and that it doesn't leave behind a version of the same "idempotent" test failures.
Could you add the entries in comment 8 to property_database.js, as expected-to-be-valid moz-prefixed gradients?

Also, I think this bug should be accompanied by some other property_database.js tweak, right? We need to add e.g. " -moz-linear-gradient(top, red, blue)", the value whose rejection prompted us to file this bug, per comment 1.
(In reply to Nazım Can Altınova [:canaltinova] from comment #7)
> We can just discard the second patch if it's really unnecessary because of Stylo but I'm not sure

(I'm OK either way, as long as tests pass.  If it makes things simpler to land the second patch, then it'll need a function-rename and/or a comment-tweak to address comment 8, at least...  The sort of tweak depends on to what extent we strip the position from values like those in comment 8.

Also -- here are a few more values that use this same default "downward" direction, with at least one *non-percent* numeric value (to specify zero for "top" -- note that zero is conveniently the same regardless of its units):

 -moz-linear-gradient(50% 0, red, blue)
 -moz-linear-gradient(50% 0px, red, blue)
 -moz-linear-gradient(center 0em, red, blue)
The values in comment 8 and comment 10 all seem to be accepted as valid, by my testing, though I'm not sure how we serialize them right now [with/without the direction]. They're all values that you should test to be sure that our behavior here ends up reasonable, though.

Reasonable might mean "don't ever serialize the default direction, for any of these values", since they're all the default.  Or, it might mean "don't serialize the direction if it's specified using strictly keywords [and is the default]".

I'm fine either way -- but if we do change Gecko, we should be sure that we're changing it such that it aligns with our [patched] servo behavior.
Comment on attachment 8911980 [details]
Bug 1396102 - Add some testcases to verify that we consistently serialize the default line directions in linear gradients (iff stylo is enabled)

https://reviewboard.mozilla.org/r/183390/#review188524
Attachment #8911980 - Flags: review?(dholbert) → review-
Yes, you're right about the values in comment 8. But the values in comment 10 contain length values and we are serializing them at computed values currently(e.g. `-moz-linear-gradient(50% 0px, red, blue)` becomes `-moz-linear-gradient(50% 0px, rgb(255, 0, 0), rgb(0, 0, 255))` in computed value).

So I preserved that behavior in specified values to align with computed values.
Comment on attachment 8911980 [details]
Bug 1396102 - Add some testcases to verify that we consistently serialize the default line directions in linear gradients (iff stylo is enabled)

https://reviewboard.mozilla.org/r/183390/#review189306

Sorry, I still don't feel great about the logic here, as noted below.

If you'd like to update it, great!  Alternately, if it doesn't feel worth it (debatable), you might perhaps want to do something like what I did in https://hg.mozilla.org/integration/autoland/rev/fd98557b7d2a to add testcases here and have them executed only in stylo-enabled builds.

::: layout/style/nsCSSValue.cpp:1852
(Diff revision 2)
> -    } else if (gradient->mBgPos.mXValue.GetUnit() != eCSSUnit_None ||
> +    } else if (!gradient->PointsDownwards() &&
> +        (gradient->mBgPos.mXValue.GetUnit() != eCSSUnit_None ||
> -        gradient->mBgPos.mYValue.GetUnit() != eCSSUnit_None ||
> +         gradient->mBgPos.mYValue.GetUnit() != eCSSUnit_None ||
> -        gradient->mAngle.GetUnit() != eCSSUnit_None) {
> +         gradient->mAngle.GetUnit() != eCSSUnit_None)) {
> +      // -moz- prefixed linear gradients and radial gradients

I have several concerns about this logic.
 - It feels redundant that we're checking it twice (here in this "else if", and above in the "if").
 - Here, we might be calling it on a radial gradient (since this clause handles radial gradients), and that feels a bit bogus.
 - The PointsDownwards() condition on these clauses feel unrelated to the comments inside of the clauses ("Unprefixed and -webkit- prefixed linear gradients" / "-moz-prefixed linear gradient and radial gradients").  The PointsDownwards() API doesn't at all tell us whether a gradient is in those categories, so it feels confusing to check it before we enter the code for those categories.

Really, I think PointsDownwards wants to be renamed to "HasDefaultDirection()", and it should be extended to support radial gradients, and we should wrap this whole "serialize the direction" chunk with that call (and simplify away the checks here that are already about checking for default direction).  That would make this updated logic sensible, I think.

For radial gradients, I think "default direction" means having eCSSUnit_None for *all of* mBGPos.mXValue, mBGPos.mYValue, and mAngle, I think...  If any of those are non-"none", it looks like we serialize the position here, at least.

::: layout/style/test/test_computed_style.html
(Diff revision 2)
> -    // Linear gradient with default keyword (should be serialized without keyword):
> -    [ "-webkit-linear-gradient(top, red, blue)",
> -      "-webkit-linear-gradient(rgb(255, 0, 0), rgb(0, 0, 255))",
> -      "-webkit-linear-gradient with legacy default direction keyword" ],
> -

(Why are you removing this testcase?  I see you're adding this same CSS value to property_database.js -- maybe you're just removing it because it feels redundant to have it in both places?

I don't think it's redundant -- there's value in keeping it here as well.  The check that we perform here [against a specific expected computed value] is more specific than what the property_database.js-backed tests perform.  (They don't have any knowledge of what the expected computed value should be for this value.)
Attachment #8911980 - Flags: review?(dholbert) → review-
Comment on attachment 8911980 [details]
Bug 1396102 - Add some testcases to verify that we consistently serialize the default line directions in linear gradients (iff stylo is enabled)

https://reviewboard.mozilla.org/r/183390/#review189306

> I have several concerns about this logic.
>  - It feels redundant that we're checking it twice (here in this "else if", and above in the "if").
>  - Here, we might be calling it on a radial gradient (since this clause handles radial gradients), and that feels a bit bogus.
>  - The PointsDownwards() condition on these clauses feel unrelated to the comments inside of the clauses ("Unprefixed and -webkit- prefixed linear gradients" / "-moz-prefixed linear gradient and radial gradients").  The PointsDownwards() API doesn't at all tell us whether a gradient is in those categories, so it feels confusing to check it before we enter the code for those categories.
> 
> Really, I think PointsDownwards wants to be renamed to "HasDefaultDirection()", and it should be extended to support radial gradients, and we should wrap this whole "serialize the direction" chunk with that call (and simplify away the checks here that are already about checking for default direction).  That would make this updated logic sensible, I think.
> 
> For radial gradients, I think "default direction" means having eCSSUnit_None for *all of* mBGPos.mXValue, mBGPos.mYValue, and mAngle, I think...  If any of those are non-"none", it looks like we serialize the position here, at least.

I'm ok with changing the logic like you said here. But the test cases that I added is failing in gecko because of -webkit-/-moz- prefix serialization on default value.
For example `-{moz,webkit}-linear-gradient(red, blue)` serializes like `-{moz,webkit}-linear-gradient(red, blue)` in stylo. But serializes like `linear-gradient(red, blue)` in gecko and that doesn't make these values idempotent. Maybe leaving the gecko side and adding the tests like you did is a better idea?

> (Why are you removing this testcase?  I see you're adding this same CSS value to property_database.js -- maybe you're just removing it because it feels redundant to have it in both places?
> 
> I don't think it's redundant -- there's value in keeping it here as well.  The check that we perform here [against a specific expected computed value] is more specific than what the property_database.js-backed tests perform.  (They don't have any knowledge of what the expected computed value should be for this value.)

I added this test case back then because we couldn't add this into properties_database.js because of this bug. I wanted to remote that once we solve that problem. But yeah, we can leave this as well.
(In reply to Nazım Can Altınova [:canaltinova] from comment #17)
> I'm ok with changing the logic like you said here. But the test cases that I
> added is failing in gecko because of -webkit-/-moz- prefix serialization on
> default value.
> For example `-{moz,webkit}-linear-gradient(red, blue)` [...] serializes like
> `linear-gradient(red, blue)` in gecko and that doesn't make these values
> idempotent.

Yeah -- or more generally, the Gecko problem here is that -{moz,webkit}-linear-gradient(top, red, blue) round-trips back to a string by dropping "top" (but keeping the prefix), and then *that* serialized value round-trips by dropping the prefix.

I think the issue there is in CSSParserImpl::ParseLinearGradient -- even if we're prefixed, that function only sets mIsLegacySyntax (i.e. it only makes us produce a prefixed serialized value) if there is an initial "gradient line" representation, like "top" in this case.  Otherwise -- if there's no initial gradient-line (i.e. no point), then it doesn't bother setting mIsLegacySyntax, and we parse just as if we were unprefixed.

So to really fix this in Gecko, I think we'd need to amend that function as well to *always* set mIsLegacySyntax if we're prefixed -- i.e. if (aFlags & eGradient_AnyLegacy) is truthy.

(And probably also set the default x/y point to the *top* in that case.)

I don't think this is worth doing, though.

> Maybe leaving the gecko side and adding the tests like you did
> is a better idea?

Yeah -- I'd say you should do that.  You'll probably want to add a new array alongside mine, maybe called e.g. "gradientsValidInServoBrokenInGecko" (where "Broken" is intentionally vague -- it's not that we reject them, but rather that our tests have problems with them).  And append the array contents to validGradientAndElementValues IFF we're styled by servo, (in the first branch of the existing "if/else" block there) and otherwise do nothing with it.

How does that sound?

> I added this test case back then because we couldn't add this into
> properties_database.js because of this bug. I wanted to remote that once we
> solve that problem. But yeah, we can leave this as well.

Ah! I see. Yeah, let's leave it in - might as well keep the improved test-strength that we get from having it there.
Thanks for the explanation! Reverted the gecko side and added the tests for stylo.
Comment on attachment 8911980 [details]
Bug 1396102 - Add some testcases to verify that we consistently serialize the default line directions in linear gradients (iff stylo is enabled)

https://reviewboard.mozilla.org/r/183390/#review189892

thanks! r=me, just some editorial nits:

::: commit-message-eddd9:3
(Diff revision 3)
> +Default line directions should not serialized in specified linear gradients
> +like computed linear gradient. But currently Gecko incorrectly serializes the
> +default line directions. Stylo correcly serializes it. Rather than trying to

I found this comment a bit hard to understand -- and I'm also not sure that the "correctness" / "should not" etc. requirements as actually clear-cut as this commit message makes them out to be.  (Who says explicitly-specified default line directions should not be serialized? I'm not sure that's a strict spec requirement.)


Please replace the extended commit message with something like the following, to make this clearer:
======
We exclude these testcases from being tested under Gecko, because they fail some mochitest checks for parse+serialize idempotence.  The failures happen because of a combination of these factors:
 (a) Gecko's style system is inconsistent about whether an explicitly-specified (but default) gradient direction should get serialized.
 (b) Gecko's style system is inconsistent about whether the vendor prefix is preserved, when parsing a prefixed gradient.

Since these prefixed gradients aren't super-important and since Gecko's style-system is being replaced via stylo, we're not bothering to fix the gecko style-system for this -- we'll just make these tests stylo-specific.

::: layout/style/test/property_database.js:511
(Diff revision 3)
> +// Gradient values that are consistently serialized in Stylo but not
> +// in Gecko. Gecko drops the prefix during roundtrip.
> +let gradientsValidInServoBrokenInGecko = [

s/Servo/Stylo/ in this variable name, for consistency with the other array.

(my fault for using the wrong term in suggested code above -- sorry)
Attachment #8911980 - Flags: review?(dholbert) → review-
Comment on attachment 8911980 [details]
Bug 1396102 - Add some testcases to verify that we consistently serialize the default line directions in linear gradients (iff stylo is enabled)

https://reviewboard.mozilla.org/r/183390/#review189894

(sorry, meant to mark r+ - fixing)
Attachment #8911980 - Flags: review- → review+
Attachment #8911979 - Attachment is obsolete: true
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8c7e7ec5fd38
Add some testcases to verify that we consistently serialize the default line directions in linear gradients (iff stylo is enabled) r=dholbert
https://hg.mozilla.org/mozilla-central/rev/8c7e7ec5fd38
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: