1241623 - Use a smarter (& more speccable) emulation behavior for -webkit-gradient(linear, ...)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

I'm not especially happy with the "throw up our hands" fallback scenario that we used in bug 1217643 for -webkit-gradient(linear,...).

Code quote:
> 10520   // OK, the gradient is angled, which means we likely can't represent it
> 10521   // exactly in |aGradient|, without doing analysis on the two points to
> 10522   // extract an angle (which we might not be able to do depending on the units
> 10523   // used).  For now, we'll just do something really basic -- just use the
> 10524   // first point as if it were the starting point in a legacy
> 10525   // -moz-linear-gradient() expression. That way, the rendered gradient will
> 10526   // progress from this first point, towards the center of the covered element,
> 10527   // to a reflected end point on the far side. Note that we have to use
> 10528   // mIsLegacySyntax=true for this to work, because standardized (non-legacy)
> 10529   // gradients place some restrictions on the reference point [namely, that it
> 10530   // use percent units & be on the border of the element].
> 10531   aGradient->mIsLegacySyntax = true;
> 10532   aGradient->mBgPos = aStartPoint;
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#10520
(added here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de257a913ac5#l2.86 )

IN PARTICULAR: I'm unhappy with the reliance on mIsLegacySyntax here, because it means we can't spec this cleanly in terms of modern gradient syntax.  And since this is a "throw up our hands" fallback case, we could just as easily throw up our hands in a way that's more speccable.

PROPOSED REPLACEMENT:
 (1) If aStartPoint.x and aEndPoint.x use different units, OR if aStartPoint.y and aEndPoint.y use different units, then we truly throw up our hands and just make the gradient go from left to right.
 (2) ELSE, we create a target point for our gradient, like "to top center","to bottom right", etc., as follows:
  - if aStartPoint.x < aEndPoint.x, use "right" (gradient progresses to the right)
  - if aStartPoint.x == aEndPoint.x, use "center" (gradient progresses vertically)
  - if aStartPoint.x > aEndPoint.x, use "left" (gradient progresses to the left)
...and similar for the .y component (with "bottom", "center", and "top")

So we may not match the exact angle that the author was going for, but we'll be in the ballpark at least -- as long as the author is consistent about their units.  (and if they're not consistent, then there's no way for us to know which direction the gradient should go, because it may vary depending on the size of the box it's applied to. e.g. if the start point is 10px,5% and the end position is 20%, 30px, then we have literally no idea which direction that will go -- because the percent values might be larger or smaller than the pixel values depending on the height & width of the box that's using this gradient.)

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> PROPOSED REPLACEMENT:
>  (1) If aStartPoint.x and aEndPoint.x use different units, OR if
> aStartPoint.y and aEndPoint.y use different units, then we truly throw up
> our hands and just make the gradient go from left to right.

Sorry, I meant "from top to bottom" here.

The idea is that our linear-gradient-direction of last resort (the ultimate fallback) should just match modern linear-gradient's default direction -- which is top to bottom, as you can see here:
 data:text/html,<div style="width:50px;height:50px;background:linear-gradient(red,blue)">

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1241623: Represent legacy -webkit-gradient(linear,...) expressions as an approximately-equivalent linear-gradient() (instead of -moz-linear-gradient()).

Note: Instead of exactly honoring the legacy -webkit-gradient(linear,...)
syntax, we aim to simply parse it into something that's approximately
equivalent for common use cases.

In the legacy -webkit-gradient(linear,...) syntax, authors provide *two
arbitrary points* to establish the direction of the gradient, whereas modern
linear-gradient() is simpler: it just takes a single "<side-or-corner>" point
and fills the box with a gradient in the direction of that side or corner.

Before this changeset, we parsed -webkit-gradient(linear,...) into a
slightly-less-legacy "-moz-linear-gradient" representation, so that we could
honor at least one of the author's specified points (since -moz-linear-gradient
accepts a single arbitrary point as the start of the gradient).  But that
prevents us from deprecating -moz-linear-gradient, and it makes it impossible
to spec our emulation behavior in terms of modern standards.

So, this changeset is just changing our approximate representation so that it
can always be serializeable as a modern linear-gradient() expression.

In addition, I'm removing the reftest "webkit-gradient-approx-linear-1.html"
(whose behavior this patch is changing slightly) and I'm replacing it with a
new chunk in test_computed_style.html to more directly test this parsing
behaivor.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8859332 [details]
Bug 1241623: Represent legacy -webkit-gradient(linear,...) expressions as an approximately-equivalent linear-gradient() (instead of -moz-linear-gradient()).

https://reviewboard.mozilla.org/r/131324/#review134206

I'm confused. Aren't we supposed to implement the functions speced in the Compat spec [1], which is basically the syntax speced in the specific version [2]? What is the -webkit-gradient thing here? If the Compat spec is not having something enough for webcompat, should we file a bug to change that?

[1] https://compat.spec.whatwg.org/#css-image-type
[2] https://www.w3.org/TR/2011/WD-css3-images-20110217/#gradients

Comment 4

[Daniel Holbert [:dholbert]]

Yeah -- we implement two different types of -webkit prefixed gradients.

(1) 2011 syntax (what you're talking about)
(2) 2008 syntax, which looks like this:
  -webkit-gradient(linear, ...)
  -webkit-gradient(radial, ...)
This older syntax isn't specced, but it's described here:
    https://webkit.org/blog/175/introducing-css-gradients/
I guess that didn't make it into the compat spec yet -- perhaps because (until this bug) we didn't have an easily-explainable/speccable strategy for emulating it.

This is tracked in this spec-issue, I think:
 https://github.com/whatwg/compat/issues/1#issuecomment-162098897

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Can we serialize everything to the 2011 syntax?

I believe Servo doesn't implement the 2008 version. Are we fine with that, or do we need to implement both for compatibility?

Comment 6

[Daniel Holbert [:dholbert]]

More background/history/context:
================================

You can see some examples of the 2008 syntax here:
 https://dxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js#636

This bug is only concerned with the question of "how do we establish a rough direction for the gradient (based on the 2 arbitrary points that the author provided in their -webkit-gradient(linear,...) expression)".  No need to worry much about color stops etc -- the code for handling those already works & can stay untouched.

So here's the tricky part -- the syntaxes are progressively less-expressive, when it comes to establishing the gradient's direction.
2008 era:
 -webkit-gradient(linear, ...) accepts TWO arbitrary points (as endpoints of the gradient line)
     These establish the start & end of the gradient.

2011 era (from different spec snapshots):
 -moz-linear-gradient(...) accepts ONE arbitrary point.
 -webkit-linear-gradient(...) accepts ONE point which can ONLY be specified as a box corner/side ("top" or "bottom right").
     The gradient goes from the specified point to the center of the box.

Modern spec:
 linear-gradient(...) accepts ONE point which can ONLY be specified as a box corner/side ("top" or "bottom right").
     The gradient goes from the center of the box through this point, basically.

SO, as you can see, -webkit-gradient is much more expressive that the other forms about its directionality. Our gradient structures are designed for the 2011-era behavior, so they only have space/logic to handle 1 point.   We could've reworked our internal impl to match the ancient -webkit-gradient expressiveness , but in the spirit of "good enough for a few sites in the long tail" legacy emulation, we instead simply try to convert it at parse-time into something *approximately* equivalent which we can later reserialize [if needed] into a more modern syntax.

Right now, the "more modern syntax" is -moz-linear-gradient, because [per the quote in comment 0 here] we TRY to honor at least one of the author-provided arbitrary points as determining our gradient's direction (and we just throw away the second point).  And -moz-linear-gradient is the only syntax which we can serialize to which supports an arbitrary author-provided point, so we use that as our representation. [1]  (So if we get rid of -moz-linear-gradient (bug 1357117), we won't be able to serialize our -webkit-gradient(linear,...) expressions anymore, which is why this bug is a prerequisite for that bug.)  SO: this bug is about simplifying our behavior to let us infer a box-position side/corner to direct the gradient towards, so that we can encode it into a representation that can be expressed in the modern unprefixed syntax.

As shown in the included mochitest, we may see e.g. a -webkit-gradient expression with "10 15, 5 20" (two points, implicitly in pixel units) and we'll now treat that as if it were a modern linear-gradient expression with "to bottom left" -- this is approximately equivalent, because the second point 5,20 is to the lower-left of the first point 10,15.

[1] Specifically, in the general case we've been throwing away the second point and just parsing it into something that's equivalent to -moz-linear-gradient(first-author-provided-point, ...)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #5)
> Can we serialize everything to the 2011 syntax?

This bug is about serializing the 2008 syntax in the modern syntax (leapfrogging 2011 altogether)

> I believe Servo doesn't implement the 2008 version. Are we fine with that,
> or do we need to implement both for compatibility?

We do need to implement the 2008 version for compatibility, yeah.  Certain mobile sites in Asia depend on it -- there's one example in e.g. bug 1107378 comment 18.  (If we don't honor it, we end up with white-text-on-white-backgrounds and stuff like that, which are big usability problems.)

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #5)
> Can we serialize everything to the 2011 syntax?

This bug is about serializing the 2008 syntax in the modern syntax (leapfrogging 2011 altogether)

> I believe Servo doesn't implement the 2008 version. Are we fine with that,
> or do we need to implement both for compatibility?

We do need to implement the 2008 version for compatibility, yeah.  Certain mobile sites in Asia depend on it -- there's one example in e.g. bug 1107378 comment 19.  (If we don't honor it, we end up with white-text-on-white-backgrounds and stuff like that, which are big usability problems.)

Comment 9

[Mike Taylor [:miketaylr]]

> This is tracked in this spec-issue, I think:
> https://github.com/whatwg/compat/issues/1#issuecomment-162098897

(I'm in the middle of handing off some other webcompat.com dev to give me more time for spec stuff, hopefully this is realistic Q2)

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8859332 [details]
Bug 1241623: Represent legacy -webkit-gradient(linear,...) expressions as an approximately-equivalent linear-gradient() (instead of -moz-linear-gradient()).

Let me know if you have any other questions.  I could tag heycam for review instead, too, if you'd prefer.  (He reviewed the original patches for this syntax a year ago in bug 1217643, but I thought maybe you'd be a good reviewer too at this point, since you're looking into related code/changes recently in bug 1337655.)

MozReview doesn't seem to let me tag you for review again, after you've cleared the review request -- so I'll just tag you on the bugzilla attachment since I know how to do that. :)

Comment 11

[Anthony Ramine [:nox]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> SO, as you can see, -webkit-gradient is much more expressive that the other
> forms about its directionality. Our gradient structures are designed for the
> 2011-era behavior, so they only have space/logic to handle 1 point.   We
> could've reworked our internal impl to match the ancient -webkit-gradient
> expressiveness , but in the spirit of "good enough for a few sites in the
> long tail" legacy emulation, we instead simply try to convert it at
> parse-time into something *approximately* equivalent which we can later
> reserialize [if needed] into a more modern syntax.

Why don't we just support them correctly?

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Anthony Ramine [:nox] from comment #11)
> Why don't we just support [-webkit-gradient] correctly?

Because:
 - this is a non-standard feature anyway
 - that'd be a lot more work [to spend our limited engineering time on]
 - that'd require more research to figure out more about what "correctly" actually means
 - it'd involve more refactoring and memory overhead with our gradient structs (they'd need to support two storing two points instead of one, even though modern syntax only accepts one point).
...all for an obsolete feature used on a small number of sites.

The goal here is to avoid white-text-on-white-background type issues on those sites.  For that threshold, what we've got is pretty good.

Having said that, if someone came along with a patch that implemented it correctly & had good tests, we might take it -- though again, we'd have to strongly consider whether it introduced additional memory overhead in the common case (with modern gradients) and decide whether it's worth it.

Comment 13

[Daniel Holbert [:dholbert]]

(And there'd be a lot more work on the spec side, too, if we were to implement the full -webkit-gradient behavior.  We're aiming to spec all of the -webkit prefixed features that we emulate [though we haven't specced this one yet].  If we can get away with implementing -webkit-gradient in terms of "similar" modern linear-gradient expressions, then that spec task is much easier and much more grokkable.)

Anyway -- hopefully that answers your question. If you have further thoughts on that, let's take it to a mailing list like dev-platform or dev-tech-layout.

Comment 14

[Cameron McCormack (:heycam)]

Daniel, you mention some Asian mobile sites that require support for the 2008 syntax.  Did/can you test them with this new mostly-good-enough mapping to modern syntax?  I can believe that it is indeed good enough, but it'd be good to do some spot checking.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8859332 [details]
Bug 1241623: Represent legacy -webkit-gradient(linear,...) expressions as an approximately-equivalent linear-gradient() (instead of -moz-linear-gradient()).

https://reviewboard.mozilla.org/r/131324/#review135500

The patch looks good, so r=me if you've done a quick check of the sites we know need this.

::: layout/style/nsCSSParser.cpp:10797
(Diff revision 1)
>  // Finalize our internal representation of a -webkit-gradient(linear, ...)
>  // expression, given the parsed points.  (The parsed color stops
>  // should already be hanging off of the passed-in nsCSSValueGradient.)
>  //
>  // Note: linear gradients progress along a line between two points.  The
>  // -webkit-gradient(linear, ...) syntax lets the author precisely specify the
>  // starting and ending point. However, our internal gradient structures
>  // only store one point, and the other point is implicitly its reflection
>  // across the painted area's center. (The legacy -moz-linear-gradient syntax
>  // also lets us store an angle.)
>  //
>  // In this function, we try to go from the two-point representation to an
>  // equivalent or approximately-equivalent one-point representation.

Does this comment need adjusting (slightly)?

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8859332 [details]
Bug 1241623: Represent legacy -webkit-gradient(linear,...) expressions as an approximately-equivalent linear-gradient() (instead of -moz-linear-gradient()).

https://reviewboard.mozilla.org/r/131324/#review135510

Comment 17

[Cameron McCormack (:heycam)]

Not sure why MozReview doesn't want my r+ to stick, but the flag is set here in Bugzilla...

Comment 18

[Anthony Ramine [:nox]]

Is anyone planning to file an issue on https://compat.spec.whatwg.org for this?

Comment 19

[Cameron McCormack (:heycam)]

(In reply to Anthony Ramine [:nox] from comment #18)
> Is anyone planning to file an issue on https://compat.spec.whatwg.org for
> this?

https://github.com/whatwg/compat/issues/1

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #14)
> Daniel, you mention some Asian mobile sites that require support for the
> 2008 syntax.  Did/can you test them with this new mostly-good-enough mapping
> to modern syntax?  I can believe that it is indeed good enough, but it'd be
> good to do some spot checking.

I dug up the originally affected sites that I could find:

* Bug 1158383 mentions webcompat issues for two sites: http://www.daily.co.jp/ and http://girlschannel.net/ (the latter in bug 1158383 comment 3). BUT, neither of those sites have -webkit-gradient() in their HTML/CSS anymore, that I could see -- I viewed them in Responsive Design Mode and did "save as complete" and grepped through the saved files.
* Bug 1107378 comment 19 mentions -webkit-gradient() usage at hao123.com , which I can confirm is still present, though I didn't immediately find elements that actually use those gradients. I extracted the gradients and they all render the same before and after this change. They look like:
  background-image:-webkit-gradient(linear,0 0,0 100%,from(rgba(0,0,0,0)),to(rgba(0,0,0,.7)))
...which we now can't extract a direction from, because the y-coordinate differs on units (0 vs 100%), so we default to top-to-bottom, which happens to be correct. :)

* I couldn't quickly find any other sites where this was the primary usability issue.  But as I recall, for *all* sites that had usability problems from us-not-supporting-webkit-prefixed-gradients (of any form, 2011/2008 & radial/linear), the exact details of the directionality weren't especially important -- we just needed to paint *some* background for text to pop out against.  So I'm not especially worried about behavior-changes resulting from this patch.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #15)
> Does this comment need adjusting (slightly)?

Perhaps, yeah. It's still basically correct, but I'll add a bit more specifics about what the function does now.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8859332 [details]
Bug 1241623: Represent legacy -webkit-gradient(linear,...) expressions as an approximately-equivalent linear-gradient() (instead of -moz-linear-gradient()).

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58f94f305112
Represent legacy -webkit-gradient(linear,...) expressions as an approximately-equivalent linear-gradient() (instead of -moz-linear-gradient()). r=heycam

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa53571a3e13
Update stylo test expectations. r=me

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58f94f305112
https://hg.mozilla.org/mozilla-central/rev/fa53571a3e13