Bug 221294 – conic-gradient with stops starting after 1turn is blank

• Component: CSS
• Status: RESOLVED FIXED

[Tim Nguyen (:ntim)]

Testcase URL:

data:text/html,<div style="width: 200px;height: 200px; background: conic-gradient(red 1.1turn, orange 1.2turn);"/>


Associated WPT: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-images/normalization-conic-2.html


I think Chrome and Firefox behavior of rendering the first stop color is reasonable since it's consistent with how linear or radial gradients are handled (in all browsers):

data:text/html,<div style="width: 200px;height: 200px; background: linear-gradient(red 300px, orange 310px);"/>

data:text/html,<div style="width: 200px;height: 200px; background: radial-gradient(red 300px, orange 310px);"/>

[Radar WebKit Bug Importer]

<rdar://problem/74157218>

[Tim Nguyen (:ntim)]

Created attachment 423887 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 423887 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Tests: web-platform-tests/css/css-images/normalization-conic-2.html

Please add a description here of the bug and what you changed to fix it.

[Antti Koivisto]

Comment on attachment 423887 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        Tests: web-platform-tests/css/css-images/normalization-conic-2.html
> 
> Please add a description here of the bug and what you changed to fix it.

The test is currently marked failing in LayoutTests/TestExpectations. The patch should remove that line so the test runs.

> Source/WebCore/css/CSSGradientValue.cpp:243
> +        while (firstZeroOrGreaterIndex < numStops && * stops[firstZeroOrGreaterIndex].offset < 0) {

WebKit coding style is '*stops' without a space. See https://webkit.org/code-style-guidelines/ for more.

> Source/WebCore/css/CSSGradientValue.cpp:245
> +            firstZeroOrGreaterIndex++;
>          }

"One-line control clauses should not use braces unless comments are included or a single statement spans multiple lines"

> Source/WebCore/css/CSSGradientValue.cpp:271
> +        while (lastOneOrLessIndex > 0 && *stops[lastOneOrLessIndex].offset > 1) {
> +            lastOneOrLessIndex--;
>          }

same here

> Source/WebCore/css/CSSGradientValue.cpp:290
> -                    stops[i].offset = 1;
> +                    *stops[i].offset = 1;

I suppose this saves a branch if we know stop Optionals are already initialized. Not sure if that is meaningful, at least it should be done consistently in other offset assignments in this function too.

[Tim Nguyen (:ntim)]

Created attachment 423928 [details]
Patch v2

There were some unintended changes in the previous patch, undid them.

[Tim Nguyen (:ntim)]

Created attachment 423931 [details]
Patch v2.1

I submitted the wrong patch earlier, sorry.

[Antti Koivisto]

Comment on attachment 423931 [details]
Patch v2.1

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

> LayoutTests/TestExpectations:-4467
> -webkit.org/b/214456 imported/w3c/web-platform-tests/css/css-images/normalization-conic-2.html [ ImageOnlyFailure ]

You should re-run prepare-ChangeLog to update LayoutTests/ChangeLog too.

> Source/WebCore/ChangeLog:9
> +        1. Notably fixes the case where all stops have an offset above 1.

You should explain what the bug was and what you changed to fix it.

> Source/WebCore/ChangeLog:10
> +        2. Remove trailing whitespace from CSSGradientValue.cpp

No need to mention this sort of things.

> Source/WebCore/ChangeLog:11
> +        3. Adjust test expectations.

This is for LayoutTests/ChangeLog

[Tim Nguyen (:ntim)]

Created attachment 423943 [details]
Patch v2.2

Adjusted changelogs according to feedback, thanks Antti!

[Tim Nguyen (:ntim)]

Comment on attachment 423943 [details]
Patch v2.2

I keep forgetting to use `-a` before amending patches (I'm used to working with mercurial which does that automatically), sorry.

I'll upload the updated patch soon.

[youenn fablet]

> I keep forgetting to use `-a` before amending patches (I'm used to working
> with mercurial which does that automatically), sorry.

Tools/Scripts/webkit-patch upload without '-g' also works and will take uncommitted changes to generate the patch.

[Tim Nguyen (:ntim)]

Created attachment 424006 [details]
Patch v3

[youenn fablet]

Comment on attachment 424006 [details]
Patch v3

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

> LayoutTests/ChangeLog:7
> +        Reviewed by Antti Koivisto.

It should probably be still Reviewed by nobody (OOPS) since Antti did not yet r+ed the patch from what I see.
Once the patch is r+ed, the commit queue will update the changelogs automatically.
Or, if you need to reupload a patch, you might need to do it yourself.

> LayoutTests/ChangeLog:9
> +        * imported/w3c/web-platform-tests/css/css-images/normalization-conic-2.html: Passed.

s/imported/w3c/web-platform-tests/css/css-images/normalization-conic-2.html: Passed./TestExpectations: enabled imported/w3c/web-platform-tests/css/css-images/normalization-conic-2.html.

> Source/WebCore/css/CSSGradientValue.cpp:243
> +        while (firstZeroOrGreaterIndex < (numStops - 1) && *stops[firstZeroOrGreaterIndex].offset < 0)

Is it safe to do (numStops - 1) in case numStops = 0?

[Tim Nguyen (:ntim)]

Created attachment 424009 [details]
Patch v3.1

Address webkit-style issue.

[Tim Nguyen (:ntim)]

Comment on attachment 424009 [details]
Patch v3.1

Will address Youenn's feedback (thanks!), and look at the repeating-conic-gradient failure.

[Tim Nguyen (:ntim)]

Created attachment 424018 [details]
Patch v4

This should hopefully fix all the issues. :)

[Tim Nguyen (:ntim)]

(In reply to youenn fablet from comment #12)
> > Source/WebCore/css/CSSGradientValue.cpp:243
> > +        while (firstZeroOrGreaterIndex < (numStops - 1) && *stops[firstZeroOrGreaterIndex].offset < 0)
> 
> Is it safe to do (numStops - 1) in case numStops = 0?

conic-gradient() with 0 stops is illegal CSS syntax, so I think that would probably be sanitized earlier.

In any case, there seems to be an assert here catching this: https://webkit-search.igalia.com/webkit/rev/49548842a98b68dfe318ce29f901962b1318e593/Source/WebCore/css/CSSGradientValue.cpp#386

[Darin Adler]

Comment on attachment 424018 [details]
Patch v4

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

This patch does fix the bug, but I think it makes the code more confusing, so not thrilled with it the way it is.

> Source/WebCore/css/CSSGradientValue.cpp:240
>          auto numStops = stops.size();

I think this code might be easier to read if we defined a lastStopIndex local variable containing numStops - 1.

> Source/WebCore/css/CSSGradientValue.cpp:244
> +        size_t firstZeroOrGreaterIndex = 0;
> +        while (firstZeroOrGreaterIndex < (numStops - 1) && *stops[firstZeroOrGreaterIndex].offset < 0)
> +            firstZeroOrGreaterIndex++;

This is not an improvement.

I find this while loop harder to read than the for loop we had before. The old one was a straightforward "look through all the stops" idiomatic loop, so we only had to read the body of the loop. The new loop is less idiomatic and thus harder to read, for me at least.

The old loop would set firstZeroOrGreaterIndex past the end of the set of stops if there was no stop >= 0. But the new code instead points at the last stop, so the index does not point to a zero-or-greater index. The old logic is better and more straightforward than the new. The new logic instead relies a case where firstZeroOrGreaterIndex points to an index which contains a negative number stop value.

If we think that "numStops" is a confusing sentinel value, we could use Optional<size_t> instead to clarify the logic. I think that would be a better direction than what this patch does.

> Source/WebCore/css/CSSGradientValue.cpp:263
>                  // All stops are below 0; just clamp them.
> -                for (size_t i = 0; i < firstZeroOrGreaterIndex; ++i)
> +                for (size_t i = 0; i <= firstZeroOrGreaterIndex; ++i)
>                      stops[i].offset = 0;

If the comment here was correct, then the loop should go through all the stops, and use numStops, not firstZeroOrGreaterIndex, as the loop limit.

If the comment is not correct, then the comment should be rewritten to be correct.

> Source/WebCore/css/CSSGradientValue.cpp:269
> +        size_t lastOneOrLessIndex = numStops - 1;
> +        while (lastOneOrLessIndex > 0 && *stops[lastOneOrLessIndex].offset > 1)
> +            lastOneOrLessIndex--;

This has the same problem as the change above. The loop is harder to read, but also lastOneOrLessIndex can be left set to 0 both in the case where it's one or less, and in the case where it's not. Changing the loop makes the variable name wrong. I’m not sure this is a good change.

As above, if we think that "numStops" is a confusing sentinel value, we could use Optional<size_t> instead to clarify the logic. I think that would be a better direction than what this patch does.

[Brent Fulgham]

Comment on attachment 424018 [details]
Patch v4

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

> LayoutTests/ChangeLog:1
> [phone]  Tim Nguyen  <[e-mail]>

Please switch to your apple.com e-mail address.

> Source/WebCore/ChangeLog:1
> [phone]  Tim Nguyen  <[e-mail]>

Ditto.

[Tim Nguyen (:ntim)]

Created attachment 424238 [details]
Patch

[Antti Koivisto]

Comment on attachment 424238 [details]
Patch

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

Much better!

> Source/WebCore/css/CSSGradientValue.cpp:250
> +        if (firstZeroOrGreaterIndex != WTF::nullopt) {

you can just do

if (firstZeroOrGreaterIndex) {

> Source/WebCore/css/CSSGradientValue.cpp:251
> +            size_t index = firstZeroOrGreaterIndex.value();

you could do

auto index = *firstZeroOrGreaterIndex;

> Source/WebCore/css/CSSGradientValue.cpp:254
> +                float prevOffset = *stops[index - 1].offset;

We usually use full words 'previousOffset' (I know the existing code used prevOffset but there is no need to stick to it).

> Source/WebCore/css/CSSGradientValue.cpp:281
> +        if (lastOneOrLessIndex != WTF::nullopt) {

Here too

> Source/WebCore/css/CSSGradientValue.cpp:282
> +            size_t index = lastOneOrLessIndex.value();

Here too

> Source/WebCore/css/CSSGradientValue.cpp:285
> +                float prevOffset = *stops[index].offset;

Here too

[Tim Nguyen (:ntim)]

Created attachment 424240 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 424248 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 424249 [details]
Patch

[Antti Koivisto]

Looks like you'll also need to skip the tests on windows (by adding them to LayoutTests/platform/win/TestExpectations). I think the feature is not enabled there (it is behind ENABLE(CSS_CONIC_GRADIENTS) flag).

[Tim Nguyen (:ntim)]

Created attachment 424251 [details]
Patch

[Darin Adler]

Comment on attachment 424251 [details]
Patch

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

This looks OK. Still have some suggestions. Also, I am setting review+ but please only land if all tests pass.

> Source/WebCore/css/CSSGradientValue.cpp:240
> +        size_t lastStopIndex = stops.size() - 1;

I had meant to suggest that we would have *both* numStops and lastStopIndex, not that we would drop numStops. It’s idiomatic to have "< size" or "< numStops" and replacing them all with "<= lastStopIndex" is not an improvement.

> Source/WebCore/css/CSSGradientValue.cpp:269
> +            for (size_t i = 0; i <= lastStopIndex; ++i)
> +                stops[i].offset = 0;

This is a loop through the entire vector. Should use a modern range-based for loop, which is more obviously correct than other types of loops:

    for (auto& stop : stops)
        stop.offset = 0;

> Source/WebCore/css/CSSGradientValue.cpp:282
> +            if (index <= lastStopIndex - 1) {

"< lastStopIndex" makes more sense than "<= lastStopIndex - 1" to me, bit it could just be personal preference.

> Source/WebCore/css/CSSGradientValue.cpp:299
> +            for (size_t i = 0; i <= lastStopIndex; ++i)
> +                stops[i].offset = 1;

Same:

    for (auto& stop : stops)
        stop.offset = 1;

[Tim Nguyen (:ntim)]

Created attachment 424268 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 424270 [details]
Patch

[EWS]

[e-mail] does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 424270 [details] from commit queue.

[EWS]

Committed r275055: <https://commits.webkit.org/r275055>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424270 [details].