Bug 14320 - <canvas> gradient stops at identical offsets are applied in the wrong order
Summary: <canvas> gradient stops at identical offsets are applied in the wrong order
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Windows XP
: P2 Minor
Assignee: Pam Greene (IRC:pamg)
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2007-06-22 15:53 PDT by Pam Greene (IRC:pamg)
Modified: 2007-07-01 07:27 PDT (History)
0 users

See Also:


Attachments
testcase (811 bytes, text/html)
2007-06-22 15:54 PDT, Pam Greene (IRC:pamg)
no flags Details
correct & incorrect screenshot snippets (13.97 KB, image/png)
2007-06-25 11:18 PDT, Pam Greene (IRC:pamg)
no flags Details
Add a sequence number to stops and use it in comparisons (81.11 KB, patch)
2007-06-27 13:13 PDT, Pam Greene (IRC:pamg)
hyatt: review-
Details | Formatted Diff | Diff
Use std::stable_sort and create <canvas> dynamically (77.63 KB, patch)
2007-06-28 22:34 PDT, Pam Greene (IRC:pamg)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pam Greene (IRC:pamg) 2007-06-22 15:53:28 PDT
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.
Comment 1 Pam Greene (IRC:pamg) 2007-06-22 15:54:29 PDT
Created attachment 15188 [details]
testcase
Comment 2 Pam Greene (IRC:pamg) 2007-06-22 19:29:08 PDT
I can't assign this to myself, but I'm working on it.
Comment 3 David Kilzer (:ddkilzer) 2007-06-23 10:59:59 PDT
Thanks for the bug report, Pam!  Could you post screenshots of the correct and incorrect renderings as well?

Comment 4 David Kilzer (:ddkilzer) 2007-06-23 11:00:49 PDT
<rdar://problem/5290084>
Comment 5 Pam Greene (IRC:pamg) 2007-06-25 11:18:44 PDT
Created attachment 15230 [details]
correct & incorrect screenshot snippets
Comment 6 Pam Greene (IRC:pamg) 2007-06-27 13:13:29 PDT
Created attachment 15276 [details]
Add a sequence number to stops and use it in comparisons
Comment 7 Dave Hyatt 2007-06-27 22:03:44 PDT
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.
Comment 8 Dave Hyatt 2007-06-27 22:06:31 PDT
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.
Comment 9 Adam Roben (:aroben) 2007-06-27 22:10:01 PDT
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.
Comment 10 Adam Roben (:aroben) 2007-06-27 22:10:34 PDT
(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.

Comment 11 Pam Greene (IRC:pamg) 2007-06-28 22:34:53 PDT
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.
Comment 12 Darin Adler 2007-06-30 11:40:13 PDT
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".
Comment 13 Mark Rowe (bdash) 2007-07-01 07:27:09 PDT
Landed in r23912.