Bug 137171 – Add support for midpoint to CSS gradients

• Component: CSS
• Status: RESOLVED FIXED

[Rik Cabanier]

CSS images level 4 defines support for gradient midpoints: http://dev.w3.org/csswg/css-images-4/#color-interpolation-hint
This patch will implement this.

[Rik Cabanier]

Created attachment 238758 [details]
Proposed fix

[Dirk Schulze]

Comment on attachment 238758 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=238758&action=review

> Source/WebCore/ChangeLog:11
> +        tests: fast/gradients/unprefixed-color-stops.html

Tests

> Source/WebCore/ChangeLog:20
> +        (WebCore::GradientStop::GradientStop): initialize the midpoint variable

Real sentences please.

> Source/WebCore/css/CSSGradientValue.cpp:260
> +    // walk over the colorstops. look for midpoints and add stops as needed
> +    // if mid < 50%, add 2 stops to the left and 6 to the right.
> +    // else add 6 stop to the left and 2 to the right
> +    // stops on the lesser side start midway so color shifts are minimized

Real sentences please. Is there a formula for the 2 to right and 6 to left?

I'll need a bit more time to look at the algorithm.

> Source/WebCore/css/CSSGradientValue.cpp:276
> +                    newStops[y-1].offset = offset1 + (offset - offset1) * (6+y) / 13;

6 + y. Note the spaces

> Source/WebCore/css/CSSGradientValue.cpp:288
> +            for (size_t y = 0; y < 9; y++) {

++y

> Source/WebCore/css/CSSParser.cpp:8617
> +            if (previousStopWasMidPoint) // can't have 2 midpoint in a row

Real sentence.

[Rik Cabanier]

Created attachment 238778 [details]
Patch

[Build Bot]

Comment on attachment 238778 [details]
Patch

Attachment 238778 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5829386800463872

New failing tests:
fast/gradients/unprefixed-gradient-parsing.html
fast/gradients/unprefixed-radial-gradients.html
fast/gradients/unprefixed-repeating-linear-gradient.html
fast/gradients/unprefixed-linear-angle-gradients.html
fast/gradients/unprefixed-repeating-radial-gradients.html
fast/gradients/unprefixed-color-stops.html
fast/gradients/unprefixed-radial-gradients2.html

[Build Bot]

Created attachment 238782 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 238778 [details]
Patch

Attachment 238778 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5938445415350272

New failing tests:
fast/gradients/unprefixed-gradient-parsing.html
fast/gradients/unprefixed-radial-gradients.html
fast/gradients/unprefixed-repeating-linear-gradient.html
fast/gradients/unprefixed-linear-angle-gradients.html
fast/gradients/unprefixed-repeating-radial-gradients.html
fast/gradients/unprefixed-color-stops.html
fast/gradients/unprefixed-radial-gradients2.html

[Build Bot]

Created attachment 238784 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Rik Cabanier]

Created attachment 238787 [details]
Patch

[Rik Cabanier]

(In reply to comment #2)
> 
> I'll need a bit more time to look at the algorithm.

A good site to compare the "ideal" gradient with a midpoint to the approximated one is here: http://codepen.io/adobe/full/fhnBJ/
You can shift the midpoint by moving the little triangle that appears when you hover over the line.

[Dirk Schulze]

(In reply to comment #9)
> (In reply to comment #2)
> > 
> > I'll need a bit more time to look at the algorithm.
> 
> A good site to compare the "ideal" gradient with a midpoint to the approximated one is here: http://codepen.io/adobe/full/fhnBJ/
> You can shift the midpoint by moving the little triangle that appears when you hover over the line.

Why the "two to the left and 6 to the right"?

[Rik Cabanier]

(In reply to comment #2)
> (From update of attachment 238758 [details])
...
> > Source/WebCore/css/CSSGradientValue.cpp:288
> > +            for (size_t y = 0; y < 9; y++) {
> 
> ++y

Why? I don't think it works correctly anymore after that change,

[Rik Cabanier]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #2)
> > > 
> > > I'll need a bit more time to look at the algorithm.
> > 
> > A good site to compare the "ideal" gradient with a midpoint to the approximated one is here: http://codepen.io/adobe/full/fhnBJ/
> > You can shift the midpoint by moving the little triangle that appears when you hover over the line.
> 
> Why the "two to the left and 6 to the right"?

This is how the exponential curve behaves. The more to the one side you go, the other side needs more points to provide a smooth appearance. 
The first stop is set a 50% between the midpoint and its counterpart. the 5 others are spaced evenly.

[Darin Adler]

Comment on attachment 238787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238787&action=review

Looks fine. Needs a little work before landing.

Test coverage doesn’t seem sufficient. I could make the math that generates the stops completely wrong and the tests would still pass, I think. How can we test this code?

> Source/WebCore/css/CSSGradientValue.cpp:98
> +    bool isMidPoint;

Since midpoint is a word, not the two words mid and point, this should be isMidpoint.

> Source/WebCore/css/CSSGradientValue.cpp:259
> +    // else add 6 stop to the left and 2 to the right.

6 stops

> Source/WebCore/css/CSSGradientValue.cpp:260
> +    // Stops on the lesser side start midway so color shifts are minimized

Missing period here.

This comment is long on what the code does does, but short on why. Why is it a good idea to add 2 stops to the left and 6 to the right? Is that a standard? A specific trick for rendering well?

> Source/WebCore/css/CSSGradientValue.cpp:263
> +    size_t x = 1;
> +    while (x < stops.size() - 1)

Even fi the incrementing is done inside the loop, I would prefer we use a for lop here rather than a while loop. We can just leave the third expression empty.

> Source/WebCore/css/CSSGradientValue.cpp:264
> +        if (stops[x].isMidPoint) {

Early continue style would make this easier to read.

    if (!stops[x].isMidpoint) {
        ++x;
        continue;
    }

    ...

> Source/WebCore/css/CSSGradientValue.cpp:266
> +            // find previous color

This is not a good comment. Below there is code that sets up many variables, and "find previous color" describes only the first line.

> Source/WebCore/css/CSSGradientValue.cpp:270
> +            Color color1 = stops[x-1].color;
> +            Color color2 = stops[x+1].color;
> +            float offset1 = stops[x-1].offset;
> +            float offset2 = stops[x+1].offset;

WebKit coding style puts spaces around the "-" and "+" operators.

> Source/WebCore/css/CSSGradientValue.cpp:273
> +            Vector<GradientStop> newStops(9);

Something with a fixed size like this should be an array, not a Vector. There is no need to allocate this on the heap.

    GradientStop newStops[9];

> Source/WebCore/css/CSSGradientValue.cpp:276
> +                    newStops[y-1].offset = offset1 + (offset - offset1) * (6 + y) / 13;

WebKit coding style puts spaces around the "-" operator in "y - 1".

> Source/WebCore/css/CSSGradientValue.cpp:284
> +                for (size_t y = 0; y < 7; y++)

The loop above uses 1-7, and this uses 0-6. I don’t understand why we made those choices. I think 0-6 for both would be better, but maybe there's something I am missing.

> Source/WebCore/css/CSSGradientValue.cpp:293
> +                int red = color1.red() + pow(relativeOffset, log(.5f) / log(midpoint)) * (color2.red() - color1.red());
> +                int green = color1.green() + pow(relativeOffset, log(.5f) / log(midpoint)) * (color2.green() - color1.green());
> +                int blue = color1.blue() + pow(relativeOffset, log(.5f) / log(midpoint)) * (color2.blue() - color1.blue());
> +                int alpha = color1.alpha() + pow(relativeOffset, log(.5f) / log(midpoint)) * (color2.alpha() - color1.alpha());

This code is converting from float to double. Is that intentional? If we want this to stay float, then it needs to be powf and logf, or maybe some modern overloaded C++ function.

I don’t think we should compute "pow(relativeOffset, log(.5f) / log(midpoint))" four times. The compiler might be able to find the common subexpression, but I think we should put it into a local variable for clarity.

> Source/WebCore/css/CSSGradientValue.cpp:295
> +                newStops[y].color = Color(red, green, blue, alpha);

The code to compute the new color would read better if it was written as a helper function. We should consider doing that.

> Source/WebCore/css/CSSGradientValue.h:51
> +    CSSGradientColorStop() : m_colorIsDerivedFromElement(false), m_isMidPoint(false) { };

The trailing semicolon here is incorrect. Please remove it.

Better to set m_isMidpoint to false using the new modern C++ syntax rather than in the constructor. We can just write:

    bool m_isMidpoint = false;

Below.

> Source/WebCore/css/CSSGradientValue.h:56
>      RefPtr<CSSPrimitiveValue> m_position; // percentage or length
>      RefPtr<CSSPrimitiveValue> m_color;
>      Color m_resolvedColor;
>      bool m_colorIsDerivedFromElement;
> +    bool m_isMidPoint;

Since this is a struct, these should not have the m_ prefix. Not new to this patch, but worth fixing.

> Source/WebCore/css/CSSParser.cpp:8599
> +    bool previousStopWasMidPoint = true;

Again, midpoint is a single word, so should be WasMidpoint.

> Source/WebCore/css/CSSParser.cpp:8638
> +    // we can't end on a midpoint

Comments should start with a capital letter and end with a period in WebKit code.

[Rik Cabanier]

Comment on attachment 238787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238787&action=review

>> Source/WebCore/css/CSSGradientValue.cpp:260
>> +    // Stops on the lesser side start midway so color shifts are minimized
> 
> Missing period here.
> 
> This comment is long on what the code does does, but short on why. Why is it a good idea to add 2 stops to the left and 6 to the right? Is that a standard? A specific trick for rendering well?

It's a trick :-)
Maybe we can improve this later by adding less if the midpoint is closer to the middle or the colors are closer together in luminance.
The correct way would be to draw it using a CGShading but smfr told me that is too slow.

>> Source/WebCore/css/CSSGradientValue.cpp:263
>> +    while (x < stops.size() - 1)
> 
> Even fi the incrementing is done inside the loop, I would prefer we use a for lop here rather than a while loop. We can just leave the third expression empty.

yeah, the 'while' makes the code look strange. Will fix

>> Source/WebCore/css/CSSGradientValue.cpp:284
>> +                for (size_t y = 0; y < 7; y++)
> 
> The loop above uses 1-7, and this uses 0-6. I don’t understand why we made those choices. I think 0-6 for both would be better, but maybe there's something I am missing.

No good reason. I will update to 0-6.

>> Source/WebCore/css/CSSGradientValue.cpp:293
>> +                int alpha = color1.alpha() + pow(relativeOffset, log(.5f) / log(midpoint)) * (color2.alpha() - color1.alpha());
> 
> This code is converting from float to double. Is that intentional? If we want this to stay float, then it needs to be powf and logf, or maybe some modern overloaded C++ function.
> 
> I don’t think we should compute "pow(relativeOffset, log(.5f) / log(midpoint))" four times. The compiler might be able to find the common subexpression, but I think we should put it into a local variable for clarity.

Will do.

>> Source/WebCore/css/CSSGradientValue.cpp:295
>> +                newStops[y].color = Color(red, green, blue, alpha);
> 
> The code to compute the new color would read better if it was written as a helper function. We should consider doing that.

There's probable already an interpolate function somewhere. I'll wire it up or write one myself.

[Rik Cabanier]

(In reply to comment #13)
> (From update of attachment 238787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238787&action=review
> 
> Looks fine. Needs a little work before landing.
> 
> Test coverage doesn’t seem sufficient. I could make the math that generates the stops completely wrong and the tests would still pass, I think. How can we test this code?

Can you paste your example? I added a number of incorrect cases in the test but you must have found a case that I didn't cover.
One weirdness is that an empty container is now allowed ie lineargradient(0% white,, 100%black)

[Rik Cabanier]

Comment on attachment 238787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238787&action=review

>> Source/WebCore/css/CSSParser.cpp:8599
>> +    bool previousStopWasMidPoint = true;
> 
> Again, midpoint is a single word, so should be WasMidpoint.

I'm not doing the right calculations in this routine. I need to sort the stops first since you're allowed to rearrange them.

[Rik Cabanier]

Created attachment 238878 [details]
Patch

[Rik Cabanier]

Created attachment 238884 [details]
Patch

[Rik Cabanier]

(In reply to comment #16)
> (From update of attachment 238787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238787&action=review
> 
> >> Source/WebCore/css/CSSParser.cpp:8599
> >> +    bool previousStopWasMidPoint = true;
> > 
> > Again, midpoint is a single word, so should be WasMidpoint.
> 
> I'm not doing the right calculations in this routine. I need to sort the stops first since you're allowed to rearrange them.

I was confused on how gradient stop were treated. The code was fine as is.
I added more tests to check for bad conditions and midpoints that fall on color stops.

[Benjamin Poulain]

Comment on attachment 238884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238884&action=review

The test coverage looks like a good start, but IMHO no patch changing rendering should be without visual test.

Ideally you should have ref-tests, but pixel tests would be a start.

> Source/WebCore/ChangeLog:9
> +        This patch adds support for gradient midpoints. It also updates the 
> +        gradient tests so they use the feature.

You should reference the spec in your changelog.

> LayoutTests/ChangeLog:17
> +        * fast/gradients/unprefixed-color-stops.html:
> +        * fast/gradients/unprefixed-gradient-parsing.html:
> +        * fast/gradients/unprefixed-linear-angle-gradients.html:
> +        * fast/gradients/unprefixed-radial-gradients.html:
> +        * fast/gradients/unprefixed-radial-gradients2.html:
> +        * fast/gradients/unprefixed-repeating-linear-gradient.html:
> +        * fast/gradients/unprefixed-repeating-radial-gradients.html:

At least some of them are pixel tests, yet you did not update any pixel results.

> LayoutTests/fast/gradients/unprefixed-color-stops.html:63
> +    .linear10 {
> +      background-image: linear-gradient(to right, red -80%, -50%, green, blue, green, 150%, blue 170%);
> +    }
> +
> +    .linear11 {
> +      background-image: linear-gradient(to right, red, 10%, green 100px, 60%, blue);
> +    }
> +
> +    .linear12 {
> +      background-image: linear-gradient(to right, red, 10%, green, 60%, blue);
> +    }

I would also have tests for absolute length in addition to percentage values.

> LayoutTests/fast/gradients/unprefixed-gradient-parsing.html:52
> +shouldBe('testGradient("background-image: linear-gradient(10deg, black 0%, 50%, green 50%, 50%, white)")', '"linear-gradient(10deg, black 0%, 50%, green 50%, 50%, white)"');

I would also put a "boundary test", having ~1000 midpoints in a valid gradient.

> LayoutTests/fast/gradients/unprefixed-gradient-parsing.html:83
> +shouldBe('testGradient("background-image: radial-gradient(25%, black)")', '"none"');
> +shouldBe('testGradient("background-image: radial-gradient(white, 25%)")', '"none"');
> +shouldBe('testGradient("background-image: radial-gradient(ellipse 10px, white, 25%, 75%, black)")', '"none"');

You did not put the empty case here.

[Benjamin Poulain]

Hum, do you have tests for midpoints that are positioned outside the two color stops?

[Rik Cabanier]

(In reply to comment #21)
> Hum, do you have tests for midpoints that are positioned outside the two color stops?

This test does that: 
  shouldBe('testGradient("background-image: linear-gradient(10deg, 50%, white)")', '"none"');

but I'll add a couple more to test more boundary conditions.

[Rik Cabanier]

Created attachment 238909 [details]
Patch

[Darin Adler]

Comment on attachment 238909 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238909&action=review

> Source/WebCore/css/CSSGradientValue.cpp:98
> +    bool isMidpoint;

Would be nice to use modern C++ syntax and write:

    bool isMidpoint = false;

And then not touch the constructor. Maybe do that for the existing data members too and remove the explicit constructor.

> Source/WebCore/css/CSSGradientValue.cpp:131
>      for (unsigned i = 0; i < result->m_stops.size(); i++)
> -        result->m_stops[i].m_resolvedColor = styleResolver->colorFromPrimitiveValue(result->m_stops[i].m_color.get());
> +        if (!result->m_stops[i].isMidpoint)
> +            result->m_stops[i].m_resolvedColor = styleResolver->colorFromPrimitiveValue(result->m_stops[i].m_color.get());

Two line body requires braces. Should make this a modern for loop.

> Source/WebCore/css/CSSGradientValue.cpp:136
> +static int interpolate(int min, int max, float position)

inline?

> Source/WebCore/css/CSSGradientValue.cpp:141
> +static Color interpolateColor(const Color& color1, const Color& color2, float position)

inline?

Not sure this needs color in its name. Might be better to use Color for arguments instead of const Color&; when the struct is simple and relatively small it’s sometimes slower to pass by reference than by value. Non-issue, I think, if inlined, though.

> Source/WebCore/css/CSSGradientValue.cpp:274
> +    // If mid < 50%, add 2 stops to the left and 6 to the right
> +    // else add 6 stops to the left and 2 to the right.

Comment still doesn’t explain the trick. Says what we are doing, but not why.

> Source/WebCore/css/CSSGradientValue.cpp:288
> +        Color color1 = stops[x-1].color;
> +        Color color2 = stops[x+1].color;

Still have the "x-1" formatting mistake that I commented on in an earlier patch.

> Source/WebCore/css/CSSGradientValue.cpp:294
> +        // check if everything coincides. If so, ignore the midpoint.

First word "check" should be capitalized.

> Source/WebCore/css/CSSGradientValue.cpp:300
> +        // Check if we coincide with the left colorstop.

Should consistently use two words, "color stop" instead of "colorstop".

> Source/WebCore/css/CSSGradientValue.cpp:303
> +            // Morph the midpoint to a regular stop with the color of the next
> +            // color stop.

Should make this a single line comment, now break the line.

> Source/WebCore/css/CSSGradientValue.cpp:312
> +            // Morph the midpoint to a regular stop with the color of the previous
> +            // color stop.

Should make this a single line comment, now break the line.

> Source/WebCore/css/CSSGradientValue.cpp:321
> +            for (size_t y = 0; y < 7; y++)

Should be ++y rather than y++.

> Source/WebCore/css/CSSGradientValue.cpp:330
> +            for (size_t y = 0; y < 7; y++)

Should be ++y rather than y++.

> Source/WebCore/css/CSSGradientValue.cpp:343
> +        }

Brace indented incorrectly here.

> Source/WebCore/css/CSSGradientValue.h:51
> +    CSSGradientColorStop() { }

Should just remove this entirely instead of just emptying it out.

[Rik Cabanier]

Comment on attachment 238909 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238909&action=review

>> Source/WebCore/css/CSSGradientValue.cpp:131
>> +            result->m_stops[i].m_resolvedColor = styleResolver->colorFromPrimitiveValue(result->m_stops[i].m_color.get());
> 
> Two line body requires braces. Should make this a modern for loop.

What is a modern for loop?

>> Source/WebCore/css/CSSGradientValue.cpp:141
>> +static Color interpolateColor(const Color& color1, const Color& color2, float position)
> 
> inline?
> 
> Not sure this needs color in its name. Might be better to use Color for arguments instead of const Color&; when the struct is simple and relatively small it’s sometimes slower to pass by reference than by value. Non-issue, I think, if inlined, though.

OK. Will do.

>> Source/WebCore/css/CSSGradientValue.cpp:288
>> +        Color color2 = stops[x+1].color;
> 
> Still have the "x-1" formatting mistake that I commented on in an earlier patch.

argh. Sorry, I don't know what that change got eaten.

[Rik Cabanier]

Created attachment 238990 [details]
Patch

[Rik Cabanier]

Created attachment 238992 [details]
Patch

[Benjamin Poulain]

Oh oh, two of the test results show scrollbars. It looks like some test cases are outside of the 800*600 box. Those test should be split, nothing below the fold gets tested!

Thanks for upgrading your tests! Lots of interesting new cases.

[Rik Cabanier]

(In reply to comment #28)
> Oh oh, two of the test results show scrollbars. It looks like some test cases are outside of the 800*600 box. Those test should be split, nothing below the fold gets tested!
> 
> Thanks for upgrading your tests! Lots of interesting new cases.

oops! Will do

[Rik Cabanier]

Created attachment 238995 [details]
Patch

[Darin Adler]

Comment on attachment 238995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238995&action=review

> Source/WebCore/css/CSSGradientValue.cpp:116
> +    for (unsigned i = 0; i < m_stops.size(); i++) {
> +        if (!m_stops[i].isMidpoint && styleResolver->colorFromPrimitiveValueIsDerivedFromElement(m_stops[i].m_color.get())) {
>              m_stops[i].m_colorIsDerivedFromElement = true;
>              derived = true;
>              break;
>          }
> +    }

Should come back here after this patch and change this to:

    for (auto& stop : m_stops)

This would read more clearly written that way.

> Source/WebCore/css/CSSGradientValue.cpp:133
> +    for (unsigned i = 0; i < result->m_stops.size(); i++) {
> +        if (!result->m_stops[i].isMidpoint)
> +            result->m_stops[i].m_resolvedColor = styleResolver->colorFromPrimitiveValue(result->m_stops[i].m_color.get());
> +    }

Should come back here after this patch and change this to:

    for (auto& stop : result->m_stops)

This would read more clearly written that way.

> Source/WebCore/css/CSSGradientValue.cpp:279
> +    // a line in that region. We then add 5 more color stops on that side to minimize the change
> +    // how the luminance changes at each of the colorstops. We don't have to add as many on the other side

"minimize the change how the luminance changes"?

"colorstops" -> "color stops"

[WebKit Commit Bot]

Comment on attachment 238995 [details]
Patch

Clearing flags on attachment: 238995

Committed r174191: <http://trac.webkit.org/changeset/174191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.