Bug 80560 - [CG] Canvas lineDashOffset does not handle negative numbers correctly
Summary: [CG] Canvas lineDashOffset does not handle negative numbers correctly
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P2 Normal
Assignee: Dirk Schulze
Depends on:
Reported: 2012-03-07 19:03 PST by webkit
Modified: 2014-04-03 15:36 PDT (History)
2 users (show)

See Also:

demonstration of problem (1.26 KB, text/html)
2012-03-07 19:03 PST, webkit
no flags Details
Patch (5.00 KB, patch)
2014-04-03 07:25 PDT, Dirk Schulze
dino: review+
Details | Formatted Diff | Diff
Patch for laning (5.00 KB, patch)
2014-04-03 13:53 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description webkit 2012-03-07 19:03:54 PST
Created attachment 130748 [details]
demonstration of problem

webkitLineDashOffset is sometimes not handled correctly -- or at least, is handled differently from Firefox's mozDashOffset, and I like how mozDashOffset is handled better. The attachment demonstrates the problem. Open it in Firefox 10 to see the desired rendering, then open it in WebKit to compare. I am currently using WebKit r110098.

I have only observed the problem when webkitLineDashOffset is negative and its absolute value is greater than the sum of all dash lengths. Uncommenting the commented-out line in the attachment works around the problem by setting webkitLineDashOffset to its modulo.
Comment 1 Dirk Schulze 2014-04-03 07:25:54 PDT
Created attachment 228507 [details]
Comment 2 Dean Jackson 2014-04-03 13:42:28 PDT
Comment on attachment 228507 [details]

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

> LayoutTests/ChangeLog:8
> +        Test correct rendering of negative offset fot Canvas dash arrays.

Comment 3 Dirk Schulze 2014-04-03 13:53:50 PDT
Created attachment 228540 [details]
Patch for laning
Comment 4 WebKit Commit Bot 2014-04-03 14:45:21 PDT
Comment on attachment 228540 [details]
Patch for laning

Clearing flags on attachment: 228540

Committed r166746: <http://trac.webkit.org/changeset/166746>
Comment 5 WebKit Commit Bot 2014-04-03 14:45:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2014-04-03 15:36:26 PDT
Comment on attachment 228540 [details]
Patch for laning

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1189
> +        for (size_t i = 0; i < dashes.size(); ++i)
> +            length += static_cast<float>(dashes[i]);

New style for loop is better here:

    for (auto& dash : dashes)
        length += static_cast<float>(dash);

Also wondering why the static_cast<float> is helpful.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1191
> +            dashOffset = fmod(dashOffset, length) + length;

Because we use fmod here instead of fmodf, I believe this involves two conversions from float to double and one conversion from double to float.

Please change to use fmodf, unless I am mistaken about that.