Bug 92383 – [Qt] Dotted borders not drawn with rounded dots

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

[Martin Leutelt]

As per CSS spec here http://www.w3.org/TR/css3-background/#border-style dotted borders should be drawn
with rounded dots.

[Bruno Abinader (history only)]

Assigning to Martin, adding block to bug 25737.

[Martin Leutelt]

Created attachment 154657 [details]
Draw dotted borders with rounded dots

[WebKit Review Bot]

Attachment 154657 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Leutelt]

Created attachment 154660 [details]
Draw dotted borders with rounded dots

Fixed style

[WebKit Review Bot]

Comment on attachment 154660 [details]
Draw dotted borders with rounded dots

Attachment 154660 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13351791

New failing tests:
fast/forms/range/slider-delete-while-dragging-thumb.html
css2.1/t170602-bdr-conflct-w-43-d.html
css2.1/t170602-bdr-conflct-w-45-d.html
css2.1/t170602-bdr-conflct-w-42-d.html
fast/borders/outline-alpha-block.html
css2.1/t0805-c5517-brdr-s-00-c.html
fast/borders/borderRadiusAllStylesAllCorners.html
css2.1/t170602-bdr-conflct-w-46-d.html
css2.1/t170602-bdr-conflct-w-47-d.html
css2.1/t170602-bdr-conflct-w-74-d.html
css2.1/t170602-bdr-conflct-w-44-d.html
fast/borders/borderRadiusDotted05.html
css2.1/t170602-bdr-conflct-w-84-d.html
css2.1/t170602-bdr-conflct-w-34-d.html
css2.1/t170602-bdr-conflct-w-64-d.html
css2.1/t170602-bdr-conflct-w-94-d.html
fast/forms/range/slider-mouse-events.html
fast/borders/border-mixed-alpha.html
css2.1/t170602-bdr-conflct-w-54-d.html
css1/box_properties/border_style.html
css2.1/t170602-bdr-conflct-w-24-d.html
fast/forms/control-clip.html
css2.1/t170602-bdr-conflct-w-41-d.html
css2.1/t170602-bdr-conflct-w-04-d.html
css2.1/t170602-bdr-conflct-w-14-d.html
css2.1/t170602-bdr-conflct-w-48-d.html
css2.1/t170602-bdr-conflct-w-49-d.html

[WebKit Review Bot]

Created attachment 154671 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Martin Leutelt]

Created attachment 154910 [details]
Draw dotted borders with rounded dots

Patch now only changes GraphicsContextQt so other ports aren't influenced by changes made in GraphicsContext.

[Bruno Abinader (history only)]

Comment on attachment 154910 [details]
Draw dotted borders with rounded dots

Requesting review and commit-queue flags.

[Noam Rosenthal]

Comment on attachment 154910 [details]
Draw dotted borders with rounded dots

We've just rebaselined plenty of pixel tests. Does this affect any of them? How is this tested?

[Martin Leutelt]

I'm currently having a look into which tests would be affected or if there are any new tests needed. But I think at least some of the border related pixel tests would have to be rebaselined again if that patch was accepted.

[Martin Leutelt]

Created attachment 156274 [details]
Patch with updated expected results

[Bruno Abinader (history only)]

Comment on attachment 156274 [details]
Patch with updated expected results

Requesting "review" and "commit-queue" flags.

[Noam Rosenthal]

Comment on attachment 156274 [details]
Patch with updated expected results

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

Looks good, But GraphicsContext::drawLine is growing to epic proportions. Let's try to split the dashing etc. to separate functions?

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:376
> +        if (isVerticalLine) {
> +            p1.setY(p1.y() - width / 2);
> +            p2.setY(p2.y() + width / 2);
> +        } else {
> +            p1.setX(p1.x() - width / 2);
> +            p2.setX(p2.x() + width / 2);
> +        }

Can we wrap this with some function with a descriptive name?

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:415
> +            // Draw circles circles in the corners too.

circles circles

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:419
> +        if (style == DashedStroke) {
> +            capStyle = Qt::FlatCap;
> +            dashes << qreal(patWidth) / width << qreal(patWidth) / width;
> +
> +            if (isVerticalLine) {
> +                p->fillRect(FloatRect(p1.x() - width / 2, p1.y() - width, width, width), QColor(color));
> +                p->fillRect(FloatRect(p2.x() - width / 2, p2.y(), width, width), QColor(color));
> +            } else {
> +                p->fillRect(FloatRect(p1.x() - width, p1.y() - width / 2, width, width), QColor(color));
> +                p->fillRect(FloatRect(p2.x(), p2.y() - width / 2, width, width), QColor(color));
> +            }
> +        }
> +
> +        // As per css spec a dotted stroke should be made of circles.
> +        if (style == DottedStroke) {
> +            capStyle = Qt::RoundCap;
> +            // The actual length of one line element can not be set to zero and at 0.1 the dots
> +            // are still slightly elongated. Setting it to 0.01 will make it look like the
> +            // line endings are being stuck together, close enough to look like a circle.
> +            // For the distance of the line elements we subtract the small amount again.
> +            qreal lineElementLength = 0.01;
> +            dashes << lineElementLength << qreal(2 * patWidth) / width - lineElementLength;
> +
> +            // Draw circles circles in the corners too.
> +            p->setPen(Qt::NoPen);
> +            p->setBrush(QColor(color));
> +            p->drawEllipse(p1.x() - width / 2, p1.y() - width / 2, width, width);
> +            p->drawEllipse(p2.x() - width / 2, p2.y() - width / 2, width, width);

Can we wrap this in a couple of functions with descriptive names?

[Martin Leutelt]

Created attachment 157643 [details]
Patch with updated expected results

New patch with requested changes and new expected results.

[Noam Rosenthal]

Comment on attachment 157643 [details]
Patch with updated expected results

Great, much more readable now!

[WebKit Review Bot]

Comment on attachment 157643 [details]
Patch with updated expected results

Clearing flags on attachment: 157643

Committed r125285: <http://trac.webkit.org/changeset/125285>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.