Bug 14320 – <canvas> gradient stops at identical offsets are applied in the wrong order

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Pam Greene (IRC:pamg)]

According to the spec, stops in linear gradients should be applied in the order in which they are given.  If more than one stop is given at the same offset, all but the first and last are irrelevant.

Safari on Windows applies the stops in the wrong order.  This is probably because the stability -- whether identical elements are kept in the same order -- of qsort (used in WebCore/html/CanvasGradient.cpp) isn't defined, and it's different from one compiler (on Mac) to another (on Windows).

Simplified test case to follow.

[Pam Greene (IRC:pamg)]

Created attachment 15188 [details]
testcase

[Pam Greene (IRC:pamg)]

I can't assign this to myself, but I'm working on it.

[David Kilzer (:ddkilzer)]

Thanks for the bug report, Pam!  Could you post screenshots of the correct and incorrect renderings as well?

[David Kilzer (:ddkilzer)]

<rdar://problem/5290084>

[Pam Greene (IRC:pamg)]

Created attachment 15230 [details]
correct & incorrect screenshot snippets

[Pam Greene (IRC:pamg)]

Created attachment 15276 [details]
Add a sequence number to stops and use it in comparisons

[Dave Hyatt]

Comment on attachment 15276 [details]
Add a sequence number to stops and use it in comparisons

I don't think qsort is guaranteed to be stable on Mac either, so seems like this code could be switched to use std::stable_sort like RenderLayer.cpp does for example.

[Dave Hyatt]

Comment on attachment 15276 [details]
Add a sequence number to stops and use it in comparisons

Should just fix this to use the right sorting algorithm instead.

[Adam Roben (:aroben)]

Comment on attachment 15276 [details]
Add a sequence number to stops and use it in comparisons

The test would be a bit cleaner if it added the canvas elements dynamically through the DOM instead of specifying them in the markup.

[Adam Roben (:aroben)]

(In reply to comment #8)
> (From update of attachment 15276 [details] [edit])
> Should just fix this to use the right sorting algorithm instead.

One sorting algorithm you could use is std::stable_sort.

[Pam Greene (IRC:pamg)]

Created attachment 15308 [details]
Use std::stable_sort and create <canvas> dynamically

> this code could be switched to use std::stable_sort like RenderLayer.cpp does

> Should just fix this to use the right sorting algorithm instead.

> One sorting algorithm you could use is std::stable_sort.

Okay okay, I get the point. :-)  Here's that, along with a test change to create the canvas elements dynamically.

[Darin Adler]

Comment on attachment 15308 [details]
Use std::stable_sort and create <canvas> dynamically

r=me

One small thing should be improved.

-static int compareStops(const void* a, const void* b)
+inline bool compareStops(const CanvasGradient::ColorStop &a, const CanvasGradient::ColorStop &b)

It's good for this to be inline, but it still needs to be internal linkage, so it should say "static inline", not just "inline".

[Mark Rowe (bdash)]

Landed in r23912.