1357117 - Serialize prefixed gradients using the same prefix they were specified with (-webkit/-moz)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Daniel Holbert [:dholbert]]

We currently support -moz and -webkit prefixed gradients (which have nearly the same syntax), along with unprefixed gradients (which have a more different syntax).

The prefixed forms can express some gradients that can't be exactly represented in unprefixed form -- so as a result, when we serialize prefixed gradient expressions, we have to use a prefix in the serialization.  (because there is no exactly-equivalent unprefixed expression)

Right now, we serialize them with a -moz prefix (regardless of whether they were originally provided with -webkit or -moz).  I'm filing this bug to track serializing them with -webkit instead.

This is for two reasons:
 (1) we'd like to unsupport -moz prefixed gradients if possible (see bug 1337655), while still supporting -webkit prefixed gradients for compatibility.  And it'd be bad if we serialized something we *do* support (-webkit prefixed gradients) into a format that we *don't* support (-moz prefixed gradients). So this is a prerequisite to that experiment.

 (2) Even without (1), we probably will have better interoperability/compatibility if we align with other engines on this & serialize -webkit prefixed gradients (at least) by adding back a -webkit prefix.

Comment 1

[Daniel Holbert [:dholbert]]

Actually, after digging more here, I'm (re)discovering that there are some -moz-linear-gradient/-moz-radial-gradient expressions which can *only* be serialized in those forms -- not as a -webkit-linear-gradient/-webkit-radial-gradient.  (In other words: the -moz versions are more expressive than the -webkit versions.)

Specifically, the syntaxes that are noted as "invalid for both -webkit & unprefixed, but valid for -moz" here:
https://dxr.mozilla.org/mozilla-central/rev/c697e756f738ce37abc56f31bfbc48f55625d617/layout/style/test/property_database.js#861-875
https://dxr.mozilla.org/mozilla-central/rev/c697e756f738ce37abc56f31bfbc48f55625d617/layout/style/test/property_database.js#892-899

So, we can't just always serialize prefixed stuff as -webkit, like I'd been wanting to in comment 0.  So, let's just serialize -webkit gradients as -webkit, and -moz gradients as -moz.  This still satisfies both of the goals outlined at the end of comment 0.

Comment 2

[Daniel Holbert [:dholbert]]

Hmm, and the plot thickens a bit further. :-/

So, the super-ancient "-webkit-gradient" formulation uses a completely different syntax which we emulate imperfectly -- and right now, that imperfect emulation depends on using a representation which we can only reserialize as a -moz-linear-gradient.  See the code-quote in bug 1241623 comment 0.  I've got an idea over in that bug about switching to a different imperfect-emulation that can be serialized using modern unprefixed gradients, but until we've done that, our -webkit-gradient() emulation depends on our support for -moz-linear-gradient.

So: I think we need to fix bug 1241623 before we fix this bug.  And then we can act on comment 1 here.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Here's what I think is a patch here, once we've fixed bug 1241623.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8858901 [details] [diff] [review]
fix v1

(It's not quite as simple as this, actually.  Our "mIsLegacySyntax" codepath currently tries to use hardcoded percentages everywhere -- so e.g. if we're given "right top", we'll serialize that as "100% 0%".  That's serialization is valid for the -moz-linear-gradient syntax, but not for -webkit-linear-gradient syntax. -webkit-linear-gradient only allows the point to be specified using box-position keywords, not raw percentages.

So in order for our serializations to be parseable, we need to make -webkit-linear-gradient use the more modern linear-gradient() position-serialization codepath, which uses box-position keywords.  New patches coming up which will do that.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1357117 part 1: Change linear-gradient serialization code to group space separator with the "to" token.

This patch doesn't change our behavior -- we'll still produce the same serialization that we would've before.

MOTIVATION: A later patch will make us share this codepath to serialize into
-webkit-linear-gradient() syntax.  That syntax uses the same representation for
points as unprefixed modern linear-gradient() (with box-side-or-corner keywords
"top", "right", etc.), but it does *not* use the word "to".  So we'd like to
allow "to"-and-its-subsequent-space-character to be optional. Hence, this patch
groups the space together with "to", rather than as a prefix on the next token,
so that we can skip right to printing the point (without a leading space)
when we add support for -webkit-linear-gradient() serialization.

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

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1357117 part 2: Add flag to distinguish between -moz & -webkit prefixed gradient expressions.

This patch doesn't change our behavior -- we won't actually act on the new flag
until the next patch in this series.

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

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1357117 part 3: Serialize -webkit-linear-gradient() expressions using the (less-expressive) -webkit-linear-gradient syntax, instead of -moz-linear-gradient.

Specifically:
* This patch uses a flag added in a prior patch to let us use the author's
  chosen prefix (-webkit or -moz) when serializing.  (We treat the -moz version
  as a special case, because that makes it more straightforward to unsupport
  -moz if/when we can.)
* This patch makes us share the linear-gradient() side-or-corner serialization
  codepath when serializing points for -webkit-linear-gradient. (The
  alternative is the -moz-linear-gradient codepath, which defaults to
  serializing with percent values 0%/100% for sides & corners -- and raw
  percentages are invalid in -webkit-linear-gradient(), so we can't share that
  codepath.) Notably, we have to skip the "to " token that the
  linear-gradient() codepath would normally print out -- that was a late
  addition to the spec and so it only exists in the unprefixed modern syntax.
  (Instead, -webkit-linear-gradient syntax is implicitly "from" the given
  point).

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

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8859761 [details]
Bug 1357117 part 1: Change linear-gradient serialization code to group space separator with the "to" token.

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

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8859762 [details]
Bug 1357117 part 2: Add flag to distinguish between -moz & -webkit prefixed gradient expressions.

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

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8859763 [details]
Bug 1357117 part 3: Serialize -webkit-linear-gradient() expressions using the (less-expressive) -webkit-linear-gradient syntax, instead of -moz-linear-gradient.

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

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8859761 [details]
Bug 1357117 part 1: Change linear-gradient serialization code to group space separator with the "to" token.

https://reviewboard.mozilla.org/r/131754/#review134628

::: layout/style/nsCSSValue.cpp:1842
(Diff revision 2)
> +        bool didAppendX = false;
>          if (!(gradient->mBgPos.mXValue.GetIntValue() & NS_STYLE_IMAGELAYER_POSITION_CENTER)) {
> -          aResult.Append(' ');
>            gradient->mBgPos.mXValue.AppendToString(eCSSProperty_background_position_x,
>                                                    aResult, aSerialization);
> +          didAppendX = true;
>          }
>          if (!(gradient->mBgPos.mYValue.GetIntValue() & NS_STYLE_IMAGELAYER_POSITION_CENTER)) {
> +          if (didAppendX) {
> +            // We're appending both an x-keyword and a y-keyword.
> +            // Add a space between them here.
> -          aResult.Append(' ');
> +            aResult.Append(' ');

Note: you might notice that the x/y ordering is reversed in this nsCSSValue chunk as compared to the equivalent nsComputedDOMStyle chunk (which appends y and then x).

That's an unfortunate quirk and we should fix it -- I spun off bug 1357932 for that.  (Not high-priority, so I might use it as an intern good-first-bug perhaps.)

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8859761 [details]
Bug 1357117 part 1: Change linear-gradient serialization code to group space separator with the "to" token.

https://reviewboard.mozilla.org/r/131754/#review136058

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8859762 [details]
Bug 1357117 part 2: Add flag to distinguish between -moz & -webkit prefixed gradient expressions.

https://reviewboard.mozilla.org/r/131756/#review136060

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8859763 [details]
Bug 1357117 part 3: Serialize -webkit-linear-gradient() expressions using the (less-expressive) -webkit-linear-gradient syntax, instead of -moz-linear-gradient.

https://reviewboard.mozilla.org/r/131758/#review136062

Can we add some tests for this changed serialization?  (It's a little surprising existing tests don't fail.)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8859761 [details]
Bug 1357117 part 1: Change linear-gradient serialization code to group space separator with the "to" token.

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

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8859762 [details]
Bug 1357117 part 2: Add flag to distinguish between -moz & -webkit prefixed gradient expressions.

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

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8859763 [details]
Bug 1357117 part 3: Serialize -webkit-linear-gradient() expressions using the (less-expressive) -webkit-linear-gradient syntax, instead of -moz-linear-gradient.

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

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #14)
> Can we add some tests for this changed serialization?  (It's a little
> surprising existing tests don't fail.)

Good call! Added: https://reviewboard.mozilla.org/r/131758/diff/2-3/

(We had existing property_database.js-based tests that were testing that a wide variety of prefixed gradient styles would serialize as *something* that we can still parse.  But no tests for what that exact serialization was, at least in the case of -webkit prefixed gradients.)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b42a4b4665f
part 1: Change linear-gradient serialization code to group space separator with the "to" token. r=heycam
https://hg.mozilla.org/integration/autoland/rev/0ea57ecd33a0
part 2: Add flag to distinguish between -moz & -webkit prefixed gradient expressions. r=heycam
https://hg.mozilla.org/integration/autoland/rev/706f342e2942
part 3: Serialize -webkit-linear-gradient() expressions using the (less-expressive) -webkit-linear-gradient syntax, instead of -moz-linear-gradient. r=heycam

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e6789da10b9
Update stylo expectations a=me

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68a355805fb4
followup: update stylo-failures.md for test_specified_value_serialization.html changes. a=me

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8859763 [details]
Bug 1357117 part 3: Serialize -webkit-linear-gradient() expressions using the (less-expressive) -webkit-linear-gradient syntax, instead of -moz-linear-gradient.

https://reviewboard.mozilla.org/r/131758/#review136500

::: layout/style/test/test_specified_value_serialization.html:126
(Diff revision 3)
> +      "-moz-linear-gradient(left top , red, blue)",
> +      //                            ^
> +      // (NOTE: our -moz-linear-gradient serialization inserts an extra space
> +      // before the first comma in some cases. This is ugly but fine,
> +      // particularly given bug 1337655).

Note: as indicated here, we've got a stray space in some -moz-linear-gradient serializations -- I believe it's there in case we have an angle after the position.

This behavior predates my patches here, and it comes from the -moz-linear-gradient serialization code that I'm switching -webkit-linear-gradient *away* from using here.

As noted in the code-comment here, I'm not particularly concerned about this since we're hoping to disable/rip-out the implicated codepath soon anyway.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b42a4b4665f
https://hg.mozilla.org/mozilla-central/rev/0ea57ecd33a0
https://hg.mozilla.org/mozilla-central/rev/706f342e2942
https://hg.mozilla.org/mozilla-central/rev/3e6789da10b9
https://hg.mozilla.org/mozilla-central/rev/68a355805fb4