Bug 12150 – The cases where no stops are defined, or when a stop offset is less than previous not handled correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Rémi Zara]

When there are no stops, ToT uses a black fill, when something equivalent to fill="none" should be used.
Tot also sorts stop offsets, but should not, and should take for each offset value the max between the offset value and all previous offset values.

This breaks the W3C test pservers-grad-16-b

Patch following.

[Rémi Zara]

Created attachment 12279 [details]
Handle stops corner cases better

Use a transparent fill when there are no stops.
Don't sort stop offsets, and always take the max between current offset value and previous offset values.

[Nikolas Zimmermann]

Hey Remi,

patch looks great! Two tiny comments:

     m_stops = stops;
-    std::sort(m_stops.begin(), m_stops.end(), compareStopOffset);

you can remove the static compareStopOffset helper completelty.

+        CGFloat currOffset = max(stops[i].first,previousOffset);

missing space between first and previousOffset.

Keep up the good work.
Niko

[Rémi Zara]

Created attachment 12287 [details]
Addressed WildFox's comments

[Eric Seidel (no email)]

This is covered in the spec, it seems:

Some notes on gradients:

Gradient offset values less than 0 (or less than 0%) are rounded up to 0%. Gradient offset values greater than 1 (or greater than 100%) are rounded down to 100%.
It is necessary that at least two stops defined to have a gradient effect. If no stops are defined, then painting shall occur as if 'none' were specified as the paint style. If one stop is defined, then paint with the solid color fill using the color defined for that gradient stop.
Each gradient offset value is required to be equal to or greater than the previous gradient stop's offset value. If a given gradient stop's offset value is not equal to or greater than all previous offset values, then the offset value is adjusted to be equal to the largest of all previous offset values.
If two gradient stops have the same offset value, then the latter gradient stop controls the color value at the overlap point. In particular:
<stop offset="0" stop-color="white"/>
<stop offset=".2" stop-color="red"/>
<stop offset=".2" stop-color="blue"/>
<stop offset="1" stop-color="black"/>
will have approximately the same effect as:
<stop offset="0" stop-color="white"/>
<stop offset=".1999999999" stop-color="red"/>
<stop offset=".2" stop-color="blue"/>
<stop offset="1" stop-color="black"/>
which is a gradient that goes smoothly from white to red, then abruptly shifts from red to blue, and then goes smoothly from blue to black.

[Darin Adler]

Comment on attachment 12287 [details]
Addressed WildFox's comments

Looks good. r=me

Is there also a maximum value for stops of 1.0 that should be enforced?

[Rémi Zara]

Created attachment 12309 [details]
updated patch, with new test case and handling of offset>1.0

[Eric Seidel (no email)]

Comment on attachment 12309 [details]
updated patch, with new test case and handling of offset>1.0

Looks good to me.  Thanks for the great patch!

[Sam Weinig]

Landed in r18681.

[Darin Adler]

Comment on attachment 12309 [details]
updated patch, with new test case and handling of offset>1.0

memset is not a correct way to set floating point values to 0 -- the memset part of this patch is incorrect

[David Kilzer (:ddkilzer)]

Reopened per Comment #9.

[David Kilzer (:ddkilzer)]

Comment on attachment 12287 [details]
Addressed WildFox's comments

Clearing darin's r+ as this patch was never landed.

[David Kilzer (:ddkilzer)]

Comment on attachment 12309 [details]
updated patch, with new test case and handling of offset>1.0

Clearing macdome's r+ so this bug doesn't show up in the commit queue.

We still need to address the issue in Comment #9.

[David Kilzer (:ddkilzer)]

Comment on attachment 12309 [details]
updated patch, with new test case and handling of offset>1.0

(In reply to comment #12)
> Clearing macdome's r+ so this bug doesn't show up in the commit queue.
> 
> We still need to address the issue in Comment #9.

REALLY clearing the flag this time.

[Rémi Zara]

Created attachment 12335 [details]
Don't use memset to set a CGFloat to 0

Do not use memset.
Patch in attachment #12309 [details] was committed as r18681

[Rémi Zara]

Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

I'm no reviewer; correct the flag

[Eric Seidel (no email)]

Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

r=me, sorry for leading you astray.

[Darin Adler]

Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

>+        outColor[0] = 0;
>+        outColor[1] = 0;
>+        outColor[2] = 0;
>+        outColor[3] = 0;

I think "0" should be "0.0f" here.

[Rémi Zara]

(In reply to comment #18)
> >+        outColor[0] = 0;
> >+        outColor[1] = 0;
> >+        outColor[2] = 0;
> >+        outColor[3] = 0;
> 
> I think "0" should be "0.0f" here.
> 
The original code used "0"

[David Kilzer (:ddkilzer)]

Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

This version of Bugzilla apparently doesn't have collision detection for review flags.

Please see Comment #16 or #17 for a "real" reviewer to credit.

[Darin Adler]

Comment on attachment 12335 [details]
Don't use memset to set a CGFloat to 0

0.0 or 0.0f would be OK (CGFloat is double under 64-bit). But in general this does not matter, because the conversion works fine. In expressions (not simple constants) the type is more likely to matter because of the intermediate values.

[David Kilzer (:ddkilzer)]

(In reply to comment #20)
> This version of Bugzilla apparently doesn't have collision detection for review
> flags.

In case anyone is interested in collision detection for flags in Bugzilla:

https://bugzilla.mozilla.org/show_bug.cgi?id=99215

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> Created an attachment (id=12335) [edit]
> Don't use memset to set a CGFloat to 0

Committed revision 18731.