Closed Bug 1357117 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
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.
Summary: Serialize prefixed gradients as their -webkit form, not -moz form → Serialize prefixed gradients using the same prefix they were specified with (-webkit/-moz)
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.
Depends on: 1241623
Attached patch fix v1 (obsolete) — Splinter Review
Here's what I think is a patch here, once we've fixed bug 1241623.
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.)
Attachment #8858901 - Attachment is obsolete: true
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.)
Attachment #8859761 - Flags: review?(xidorn+moz) → review?(cam)
Attachment #8859762 - Flags: review?(xidorn+moz) → review?(cam)
Attachment #8859763 - Flags: review?(xidorn+moz) → review?(cam)
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
Attachment #8859761 - Flags: review?(cam) → review+
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
Attachment #8859762 - Flags: review?(cam) → review+
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.)
Attachment #8859763 - Flags: review?(cam) → review+
(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.)
Pushed by dholbert@mozilla.com:
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
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/68a355805fb4
followup: update stylo-failures.md for test_specified_value_serialization.html changes. a=me
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.
See Also: → 1367299
You need to log in before you can comment on or make changes to this bug.