Bug 67166 - Update CSS3 gradient support to the latest spec version
: Update CSS3 gradient support to the latest spec version
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar, WebExposed
: 66897
:
  Show dependency treegraph
 
Reported: 2011-08-29 16:37 PST by
Modified: 2013-01-16 01:04 PST (History)


Attachments
Patch (16.58 KB, patch)
2012-12-19 13:45 PST, Tab Atkins
no flags Review Patch | Details | Formatted Diff | Diff
Patch (22.13 KB, patch)
2012-12-19 17:38 PST, Tab Atkins
no flags Review Patch | Details | Formatted Diff | Diff
Patch (22.00 KB, patch)
2012-12-20 17:11 PST, Tab Atkins
no flags Review Patch | Details | Formatted Diff | Diff
Patch (341.00 KB, patch)
2012-12-21 13:41 PST, Tab Atkins
no flags Review Patch | Details | Formatted Diff | Diff
Patch (341.54 KB, patch)
2013-01-08 19:08 PST, Tab Atkins
no flags Review Patch | Details | Formatted Diff | Diff
Patch (341.48 KB, patch)
2013-01-09 11:40 PST, Tab Atkins
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (335.25 KB, patch)
2013-01-09 16:35 PST, Tab Atkins
no flags Review Patch | Details | Formatted Diff | Diff
Patch (341.39 KB, patch)
2013-01-09 16:42 PST, Tab Atkins
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-08-29 16:37:41 PST
We need to upgrade our gradients support to match the latest version of http://dev.w3.org/csswg/css3-images/#gradients
------- Comment #1 From 2011-08-29 16:37:55 PST -------
<rdar://problem/10042826>
------- Comment #2 From 2012-12-19 07:39:13 PST -------
Is there any target milestone for this?
------- Comment #3 From 2012-12-19 09:02:38 PST -------
Tab is working on this.
------- Comment #4 From 2012-12-19 13:45:19 PST -------
Created an attachment (id=180220) [details]
Patch
------- Comment #5 From 2012-12-19 13:46:02 PST -------
(From update of attachment 180220 [details])
Not yet ready for real review - submitting for help with a problem.
------- Comment #6 From 2012-12-19 13:53:59 PST -------
Tab: you'll have to add support for the magic corner-to-corner behavior, right?
------- Comment #7 From 2012-12-19 13:58:22 PST -------
(In reply to comment #6)
> Tab: you'll have to add support for the magic corner-to-corner behavior, right?

Already done, in CSSGradientValue.cpp#l670
------- Comment #8 From 2012-12-19 17:38:00 PST -------
Created an attachment (id=180253) [details]
Patch
------- Comment #9 From 2012-12-19 17:41:06 PST -------
Patch is now ready to review.  I'll add tests before I try to land it for real.

I added both linear-gradient() and radial-gradient(), and their repeating variants, unprefixed.  They both accept the spec's current grammar, rather than the older grammar that our prefixed variants use.  They also serialize slightly differently, following the guidance in Images 4 that we should omit everything that can be omitted without changing the meaning.

As part of this, I also fixed up a few areas where we were relying on some members being non-null, because I violated those assumptions when parsing things. (I needed to, so I could use null-ness to determine whether it was originally specified or not, which is necessary for some of the serialization code.)
------- Comment #10 From 2012-12-19 21:59:03 PST -------
(From update of attachment 180253 [details])
Attachment 180253 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15403957
------- Comment #11 From 2012-12-20 17:11:07 PST -------
Created an attachment (id=180446) [details]
Patch
------- Comment #12 From 2012-12-20 17:49:13 PST -------
(From update of attachment 180446 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=180446&action=review

Patches up for review should always include tests (i.e. if you're setting the r? flag). If you just want someone to take a look, then it seems people label their uploads "wip" or something like that.

After a quick glance this looks fine to me. You should go ahead and make the tests.

> Source/WebCore/ChangeLog:11
> +        * css/CSSGradientValue.cpp:
> +        (WebCore::CSSGradientValue::computeEndPoint):

I've been schooled to always provide comments here if possible. e.g. "Renaming parameters to make much more sense"
------- Comment #13 From 2012-12-21 13:41:50 PST -------
Created an attachment (id=180549) [details]
Patch
------- Comment #14 From 2012-12-21 13:43:27 PST -------
(In reply to comment #12)
> (From update of attachment 180446 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180446&action=review
> 
> Patches up for review should always include tests (i.e. if you're setting the r? flag). If you just want someone to take a look, then it seems people label their uploads "wip" or something like that.

Cool, I'll do so in the future.

> After a quick glance this looks fine to me. You should go ahead and make the tests.

Done!  Just copied all the existing -webkit-*-gradient() tests, and a few of the useful-looking old -webkit-gradient() tests, manipulating a few of them to increase test coverage.

> > Source/WebCore/ChangeLog:11
> > +        * css/CSSGradientValue.cpp:
> > +        (WebCore::CSSGradientValue::computeEndPoint):
> 
> I've been schooled to always provide comments here if possible. e.g. "Renaming parameters to make much more sense"

Done, and I'll do so in the future.  I believe all the other change lines are self-explanatory given the patch title.
------- Comment #15 From 2012-12-21 14:29:48 PST -------
(From update of attachment 180549 [details])
Attachment 180549 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15452553

New failing tests:
fast/gradients/unprefixed-gradient-parsing.html
------- Comment #16 From 2013-01-02 21:17:55 PST -------
(From update of attachment 180549 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=180549&action=review

> Source/WebCore/ChangeLog:8
> +        Tests: fast/gradients/unprefixed-color-stop-units.html

Using 'unprefixed' in a test name doesn't seem very forward-looking, but I don't have better ideas.

> Source/WebCore/css/CSSGradientValue.cpp:561
> +            result.append(m_angle->cssText());
> +            wroteSomething = true;
> +        }
> +        if (m_firstX || m_firstY) {
> +            result.appendLiteral("to ");

Might this ever write out "90degto " ?

> Source/WebCore/css/CSSGradientValue.cpp:703
> +                // "Magic" corners, so the 50% line touches two corners.
> +                float rise = size.width();
> +                float run = size.height();
> +                if (m_firstX && m_firstX->getIdent() == CSSValueLeft)
> +                    run *= -1;
> +                if (m_firstY && m_firstY->getIdent() == CSSValueBottom)
> +                    rise *= -1;
> +                // Compute angle, and flip it back to "bearing angle" degrees.
> +                float angle = 90.0 - rad2deg(atan2(rise, run));
> +                endPointsFromAngle(angle, size, firstPoint, secondPoint, m_gradientType);

Does this code get re-run when the size of the box changes, but gradient itself does not? I'm concerned that we don't always recompute the magic angle when we need to.

No need for ".0" on numeric literals that the compiler will convert to floating point for you.

> Source/WebCore/css/CSSParser.cpp:7330
> +    // Walk the arguments.

This comment doesn't add anything.

> Source/WebCore/css/CSSParser.cpp:7401
> +    // Walk the arguments.

Ditto.
------- Comment #17 From 2013-01-07 16:03:07 PST -------
(In reply to comment #16)
> (From update of attachment 180549 [details] [details])
> > Source/WebCore/css/CSSGradientValue.cpp:561
> > +            result.append(m_angle->cssText());
> > +            wroteSomething = true;
> > +        }
> > +        if (m_firstX || m_firstY) {
> > +            result.appendLiteral("to ");
> 
> Might this ever write out "90degto " ?

No - if the parser sees an angle, it never sets the x/y, and vice versa.

However, there's no good reason to not make that an if/else instead of just two adjacent ifs, so I'll do so.

(You pointing this out also led me to discover that I was missing a case in the "don't print things that are default" effort - "to bottom" was getting written out when it doesn't need to be.)

> > Source/WebCore/css/CSSGradientValue.cpp:703
> > +                // "Magic" corners, so the 50% line touches two corners.
> > +                float rise = size.width();
> > +                float run = size.height();
> > +                if (m_firstX && m_firstX->getIdent() == CSSValueLeft)
> > +                    run *= -1;
> > +                if (m_firstY && m_firstY->getIdent() == CSSValueBottom)
> > +                    rise *= -1;
> > +                // Compute angle, and flip it back to "bearing angle" degrees.
> > +                float angle = 90.0 - rad2deg(atan2(rise, run));
> > +                endPointsFromAngle(angle, size, firstPoint, secondPoint, m_gradientType);
> 
> Does this code get re-run when the size of the box changes, but gradient itself does not? I'm concerned that we don't always recompute the magic angle when we need to.

It better do so - this is the same code that used to set the non-magic corner behavior, which needed the same tracking.  ^_^  A quick experiment shows that it does:

<!doctype html>
<div style="background: linear-gradient(to top right, red, white, blue);
            height: 100px; width: 100px;"></div>
<input type=range min=0 max=200 
  oninput="document.querySelector('div').style.width = this.value + 'px';"
  style="width: 200px;">
<input type=range min=0 max=200 
  oninput="document.querySelector('div').style.height = this.value + 'px';" 
  style="width: 200px;">

> No need for ".0" on numeric literals that the compiler will convert to floating point for you.

Ah, thanks.

> > Source/WebCore/css/CSSParser.cpp:7330
> > +    // Walk the arguments.
> 
> This comment doesn't add anything.
> 
> > Source/WebCore/css/CSSParser.cpp:7401
> > +    // Walk the arguments.
> 
> Ditto.

Sorry, leftover cruft from the old parsing functions.
------- Comment #18 From 2013-01-07 16:27:23 PST -------
> <!doctype html>
> <div style="background: linear-gradient(to top right, red, white, blue);
>             height: 100px; width: 100px;"></div>
> <input type=range min=0 max=200 
>   oninput="document.querySelector('div').style.width = this.value + 'px';"
>   style="width: 200px;">
> <input type=range min=0 max=200 
>   oninput="document.querySelector('div').style.height = this.value + 'px';" 
>   style="width: 200px;">

What about <div style="background: linear-gradient(to top right, red, white, blue); height: 100px; width: 80%;"></div>
and resize the container (or the window)?
------- Comment #19 From 2013-01-07 16:52:24 PST -------
(In reply to comment #18)
> > <!doctype html>
> > <div style="background: linear-gradient(to top right, red, white, blue);
> >             height: 100px; width: 100px;"></div>
> > <input type=range min=0 max=200 
> >   oninput="document.querySelector('div').style.width = this.value + 'px';"
> >   style="width: 200px;">
> > <input type=range min=0 max=200 
> >   oninput="document.querySelector('div').style.height = this.value + 'px';" 
> >   style="width: 200px;">
> 
> What about <div style="background: linear-gradient(to top right, red, white, blue); height: 100px; width: 80%;"></div>
> and resize the container (or the window)?

I thought of that and tested it too.  ^_^  I wrapped the above div in a wrapper, changed the inner div to use percentages, and made the inputs change the wrapper's size.  It all works.

(Again, it *must* work, if our old gradient code worked.)
------- Comment #20 From 2013-01-07 17:00:23 PST -------
(In reply to comment #19)
> I thought of that and tested it too.  ^_^  I wrapped the above div in a wrapper, changed the inner div to use percentages, and made the inputs change the wrapper's size.  It all works.
> 
> (Again, it *must* work, if our old gradient code worked.)

Ah, I see CSSGradientValue::image(), which is called from painting code, caches based on size, so you're ok.
------- Comment #21 From 2013-01-08 19:08:24 PST -------
Created an attachment (id=181821) [details]
Patch
------- Comment #22 From 2013-01-08 19:09:51 PST -------
All right, this patch fixes Simon's issues, and fixes the crasher bug that I finally figured out in my parser.  Yay testing!
------- Comment #23 From 2013-01-08 20:25:01 PST -------
(From update of attachment 181821 [details])
Attachment 181821 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15760332

New failing tests:
fast/gradients/unprefixed-linear-angle-gradients.html
fast/gradients/unprefixed-repeating-linear-gradient.html
------- Comment #24 From 2013-01-09 11:40:15 PST -------
Created an attachment (id=181960) [details]
Patch
------- Comment #25 From 2013-01-09 11:40:57 PST -------
(In reply to comment #24)
> Created an attachment (id=181960) [details] [details]
> Patch

Bleh, tho(In reply to comment #23)
> (From update of attachment 181821 [details] [details])
> Attachment 181821 [details] [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15760332
> 
> New failing tests:
> fast/gradients/unprefixed-linear-angle-gradients.html
> fast/gradients/unprefixed-repeating-linear-gradient.html

Bleh, these test failures are single-pixel failures.  Rebased.

Can I get an r+ from one of my previous reviewers?
------- Comment #26 From 2013-01-09 11:45:10 PST -------
(From update of attachment 181960 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=181960&action=review

> Source/WebCore/css/CSSParser.cpp:7380
> +        if ((location = valueFromSideKeyword(a, isHorizontal))) {
> +            if (isHorizontal)
> +                endX = location;
> +            else
> +                endY = location;
> +
> +            a = args->next();
> +            if (!a)
> +                return false;
> +
> +            if ((location = valueFromSideKeyword(a, isHorizontal))) {
> +                if (isHorizontal) {
> +                    if (endX)
> +                        return false;
> +                    endX = location;
> +                } else {
> +                    if (endY)
> +                        return false;
> +                    endY = location;
> +                }
> +
> +                args->next();
> +            }
> +
> +            expectComma = true;
> +        } else
> +            return false;

I think this would be better as
location = valueFromSideKeyword(a, isHorizontal);
if (!location)
  return false

You should keep non-exceptional code outside big if statements if possible.

> LayoutTests/ChangeLog:48
> +        * fast/gradients/unprefixed-color-stop-units-expected.txt: Added.
> +        * fast/gradients/unprefixed-color-stop-units.html: Added.
> +        * fast/gradients/unprefixed-color-stops-expected.txt: Added.
> +        * fast/gradients/unprefixed-color-stops.html: Added.
> +        * fast/gradients/unprefixed-generated-gradients-expected.txt: Added.
> +        * fast/gradients/unprefixed-generated-gradients.html: Added.
> +        * fast/gradients/unprefixed-gradient-parsing-expected.txt: Added.
> +        * fast/gradients/unprefixed-gradient-parsing.html: Added.
> +        * fast/gradients/unprefixed-linear-angle-gradients-expected.txt: Added.
> +        * fast/gradients/unprefixed-linear-angle-gradients.html: Added.
> +        * fast/gradients/unprefixed-linear-right-angle-gradients-expected.txt: Added.
> +        * fast/gradients/unprefixed-linear-right-angle-gradients.html: Added.
> +        * fast/gradients/unprefixed-list-item-gradient-expected.txt: Added.
> +        * fast/gradients/unprefixed-list-item-gradient.html: Added.
> +        * fast/gradients/unprefixed-radial-gradients-expected.txt: Added.
> +        * fast/gradients/unprefixed-radial-gradients.html: Added.
> +        * fast/gradients/unprefixed-radial-gradients2-expected.txt: Added.
> +        * fast/gradients/unprefixed-radial-gradients2.html: Added.
> +        * fast/gradients/unprefixed-radial-gradients3-expected.txt: Added.
> +        * fast/gradients/unprefixed-radial-gradients3.html: Added.
> +        * fast/gradients/unprefixed-repeating-end-fill-expected.txt: Added.
> +        * fast/gradients/unprefixed-repeating-end-fill.html: Added.
> +        * fast/gradients/unprefixed-repeating-linear-gradient-expected.txt: Added.
> +        * fast/gradients/unprefixed-repeating-linear-gradient.html: Added.
> +        * fast/gradients/unprefixed-repeating-radial-gradients-expected.txt: Added.
> +        * fast/gradients/unprefixed-repeating-radial-gradients.html: Added.
> +        * fast/gradients/unprefixed-zero-range-repeating-gradient-hang-expected.txt: Added.
> +        * fast/gradients/unprefixed-zero-range-repeating-gradient-hang.html: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-color-stop-units-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-color-stops-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-generated-gradients-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-linear-angle-gradients-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-linear-right-angle-gradients-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-list-item-gradient-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-radial-gradients-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-radial-gradients2-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-radial-gradients3-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-repeating-end-fill-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-repeating-linear-gradient-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-repeating-radial-gradients-expected.png: Added.
> +        * platform/chromium-linux/fast/gradients/unprefixed-zero-range-repeating-gradient-hang-expected.png: Added.

You should edit the TestExpectation files for non-Chromium platforms to indicate that image results are missing for the tests that expect them.
------- Comment #27 From 2013-01-09 16:35:33 PST -------
Created an attachment (id=182008) [details]
Patch for landing
------- Comment #28 From 2013-01-09 16:42:27 PST -------
Created an attachment (id=182013) [details]
Patch
------- Comment #29 From 2013-01-15 12:57:44 PST -------
(From update of attachment 182013 [details])
Looks good, and Simon already reviewed a few times.
------- Comment #30 From 2013-01-15 21:52:03 PST -------
(From update of attachment 182013 [details])
setting cq+ based on Dino's r+
------- Comment #31 From 2013-01-15 22:16:58 PST -------
(From update of attachment 182013 [details])
Clearing flags on attachment: 182013

Committed r139836: <http://trac.webkit.org/changeset/139836>
------- Comment #32 From 2013-01-15 22:17:04 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #33 From 2013-01-16 01:04:33 PST -------
FYI I rebaselined the tests added in this change, and added new baselines for Chromium in r139845.