Bug 139872 - Incorrect dashed and dotted border painting.
Summary: Incorrect dashed and dotted border painting.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
: 135952 (view as bug list)
Depends on: 139900
Blocks: 141967
  Show dependency treegraph
 
Reported: 2014-12-22 12:25 PST by zalan
Modified: 2015-08-13 09:34 PDT (History)
7 users (show)

See Also:


Attachments
dashed border1 (16.83 KB, image/png)
2014-12-22 12:25 PST, zalan
no flags Details
dashed border 2 (29.33 KB, image/png)
2014-12-22 12:25 PST, zalan
no flags Details
Patch (72.40 KB, patch)
2014-12-22 12:47 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (72.23 KB, patch)
2014-12-22 14:04 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2014-12-22 12:25:17 PST
Created attachment 243633 [details]
dashed border1

In certain cases, corners don't get rendered properly. See attached screenshots.
Comment 1 zalan 2014-12-22 12:25:34 PST
Created attachment 243634 [details]
dashed border 2
Comment 2 zalan 2014-12-22 12:40:28 PST
rdar://problem/18024205
Comment 3 zalan 2014-12-22 12:47:58 PST
Created attachment 243635 [details]
Patch
Comment 4 Simon Fraser (smfr) 2014-12-22 13:02:37 PST
Comment on attachment 243635 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:304
> +            CGContextFillRect(context, FloatRect(point1.x(), point2.y()  - cornerWidth, thickness, cornerWidth));

Extra space in point2.y() - cornerWidth

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:331
> +        CGContextRestoreGState(context);
> +        strokeWidth -= 2 * cornerWidth;
> +        float patternWidth = strokeStyle == DottedStroke ? thickness : std::min(3 * thickness, std::max(thickness, strokeWidth / 3));
> +        // Check if corner drawing sufficiently covers the line.
> +        if (strokeWidth <= patternWidth + 1)
> +            return;
> +
> +        // Pattern starts with full fill and ends with the empty fill.
> +        // 1. Let's start with the empty phase after the corner.
> +        // 2. Check if we've got odd or even number of patterns and whether they fully cover the line.
> +        // 3. In case of even number of patterns and/or remainder, move the pattern start position
> +        // so that the pattern is balanced between the corners.
> +        float patternOffset = patternWidth;
> +        int numberOfSegments = floorf(strokeWidth / patternWidth);
> +        bool oddNumberOfSegments = numberOfSegments % 2;
> +        float remainder = strokeWidth - (numberOfSegments * patternWidth);
> +        if (oddNumberOfSegments && remainder)
> +            patternOffset -= remainder / 2;
> +        else if (!oddNumberOfSegments) {
> +            if (remainder)
> +                patternOffset += patternOffset - (patternWidth + remainder)  / 2;
> +            else
> +                patternOffset += patternWidth  / 2;

I feel that this should be in shared code, rather than here in GraphicsContextCG, so that all platforms have the same dashed painting behavior.

> Source/WebCore/rendering/RenderObject.cpp:743
> +            graphicsContext->drawLine(roundPointToDevicePixels(LayoutPoint(x1, y1), deviceScaleFactor), roundPointToDevicePixels(LayoutPoint(x2, y2), deviceScaleFactor));

Only need one drawLine!
Comment 5 zalan 2014-12-22 14:04:29 PST
Created attachment 243641 [details]
Patch
Comment 6 zalan 2014-12-22 14:05:21 PST
(In reply to comment #4)
> Comment on attachment 243635 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243635&action=review
> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:304
> > +            CGContextFillRect(context, FloatRect(point1.x(), point2.y()  - cornerWidth, thickness, cornerWidth));
> 
> Extra space in point2.y() - cornerWidth
Done.

> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:331
> > +        CGContextRestoreGState(context);
> > +        strokeWidth -= 2 * cornerWidth;
> > +        float patternWidth = strokeStyle == DottedStroke ? thickness : std::min(3 * thickness, std::max(thickness, strokeWidth / 3));
> > +        // Check if corner drawing sufficiently covers the line.
> > +        if (strokeWidth <= patternWidth + 1)
> > +            return;
> > +
> > +        // Pattern starts with full fill and ends with the empty fill.
> > +        // 1. Let's start with the empty phase after the corner.
> > +        // 2. Check if we've got odd or even number of patterns and whether they fully cover the line.
> > +        // 3. In case of even number of patterns and/or remainder, move the pattern start position
> > +        // so that the pattern is balanced between the corners.
> > +        float patternOffset = patternWidth;
> > +        int numberOfSegments = floorf(strokeWidth / patternWidth);
> > +        bool oddNumberOfSegments = numberOfSegments % 2;
> > +        float remainder = strokeWidth - (numberOfSegments * patternWidth);
> > +        if (oddNumberOfSegments && remainder)
> > +            patternOffset -= remainder / 2;
> > +        else if (!oddNumberOfSegments) {
> > +            if (remainder)
> > +                patternOffset += patternOffset - (patternWidth + remainder)  / 2;
> > +            else
> > +                patternOffset += patternWidth  / 2;
> 
> I feel that this should be in shared code, rather than here in
> GraphicsContextCG, so that all platforms have the same dashed painting
> behavior.
bug 139878
> 
> > Source/WebCore/rendering/RenderObject.cpp:743
> > +            graphicsContext->drawLine(roundPointToDevicePixels(LayoutPoint(x1, y1), deviceScaleFactor), roundPointToDevicePixels(LayoutPoint(x2, y2), deviceScaleFactor));
> 
> Only need one drawLine!
Done.
Comment 7 zalan 2014-12-22 14:45:02 PST
*** Bug 135952 has been marked as a duplicate of this bug. ***
Comment 8 WebKit Commit Bot 2014-12-22 15:19:09 PST
Comment on attachment 243641 [details]
Patch

Clearing flags on attachment: 243641

Committed r177658: <http://trac.webkit.org/changeset/177658>
Comment 9 WebKit Commit Bot 2014-12-22 15:19:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Csaba Osztrogonác 2014-12-22 22:41:06 PST
(In reply to comment #8)
> Comment on attachment 243641 [details]
> Patch
> 
> Clearing flags on attachment: 243641
> 
> Committed r177658: <http://trac.webkit.org/changeset/177658>

It made 21 tests assert in debug mode, 3 perf tests crash.
Comment 11 Alexey Proskuryakov 2014-12-22 23:05:41 PST
Link to results: https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r177676%20(1092)/results.html

I'm going to roll out.
Comment 12 WebKit Commit Bot 2014-12-22 23:11:27 PST
Re-opened since this is blocked by bug 139900
Comment 13 zalan 2014-12-23 10:04:43 PST
Committed r177686: <http://trac.webkit.org/changeset/177686>