Bug 139872 – Incorrect dashed and dotted border painting.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

Created attachment 243633 [details]
dashed border1

In certain cases, corners don't get rendered properly. See attached screenshots.

[alan]

Created attachment 243634 [details]
dashed border 2

[alan]

rdar://problem/18024205

[alan]

Created attachment 243635 [details]
Patch

[Simon Fraser (smfr)]

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!

[alan]

Created attachment 243641 [details]
Patch

[alan]

(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.

[alan]

*** Bug 135952 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 243641 [details]
Patch

Clearing flags on attachment: 243641

Committed r177658: <http://trac.webkit.org/changeset/177658>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.

[Alexey Proskuryakov]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 139900

[alan]

Committed r177686: <http://trac.webkit.org/changeset/177686>