Bug 123236 – Gaps between underlines in adjacent underlined text runs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Don't round horizontal underline extents

[Myles C. Maxfield]

Created attachment 215013 [details]
Patch

[Myles C. Maxfield]

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

[Radar WebKit Bug Importer]

<rdar://problem/15305380>

[Darin Adler]

Comment on attachment 215013 [details]
Patch

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

Looks like a good fix.

> Source/WebCore/ChangeLog:27
> +        There are two pieces to this change. The first piece is in
> +        InlineTextBox::paint(). When we were computing the bounding
> +        box for the InlineTextBox, we were putting the floating-point
> +        extents we received from the InlineBox into a LayoutSize. If
> +        a LayoutSize were fixed-point with sufficient precision, this
> +        would be okay, but right now a LayoutSize is simply an int,
> +        meaning that we were clamping the underline boundaries to
> +        integral values. Instead, we should be using the same unit
> +        throughout all of this code, which means that the bounding
> +        box should use floats instead.
> +
> +        In addition, inside GraphicsContext::drawLineForText(), we are
> +        rounding the underline to pixel boundaries so that it appears
> +        very crisp on the screen. However, this does not play well with
> +        these fractional underline extents. We should change the
> +        rounding mode to perform floating point addition before rounding,
> +        rather than rounding position and width before adding them
> +        together. Note that this will not blend the pixel in question
> +        between two colors; instead, whichever run contains the center
> +        of the disputed pixel will win.

Next time, please write a much smaller comment, and do it per-function.

> Source/WebCore/ChangeLog:32
> +        (WebCore::GraphicsContext::drawLineForText): Second part (See above)

I would say something like: “Round each end of the underline to a pixel boundary.”

> Source/WebCore/ChangeLog:34
> +        (WebCore::InlineTextBox::paint): First part (See above)

I would say something like: “Don’t convert to a LayoutSize just to convert to a FloatSize.”

> LayoutTests/ChangeLog:14
> +        This test contains two adjacent text runs. The first one has a
> +        x-position of 21.3 with a width of 44.4. The second one has a
> +        x-position of (21.3 + 44.4 =) 65.7. If we naively rounded these
> +        to pixel coordinates, the left run would be rounded to a
> +        x-position of 21 with a width of 44, so it would extend to pixel
> +        65. However, the next run's x-position would be rounded to 66,
> +        so there is a single pixel gap between the supposedly adjacent run.
> +        This test makes sure that the above scenerio does not occur,
> +        and that this "gap" pixel is filled in.

This comment is also too long, and more importantly, it would be much more appropriate in the test rather than in the change log.

> LayoutTests/ChangeLog:17
> +        * platform/mac/fast/css3-text/css3-text-decoration/no-gap-between-two-rounded-textboxes-expected.png: Added.

Could we make this a reference test somehow? Maybe use a canvas to draw the underline?

[WebKit Commit Bot]

Comment on attachment 215013 [details]
Patch

Rejecting attachment 215013 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 215013, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/11168023

[Simon Fraser (smfr)]

Comment on attachment 215013 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Don't round horizontal underline extents

This actually doesn't describe the user-visible symptom (underline gaps).

>> LayoutTests/ChangeLog:17
>> +        * platform/mac/fast/css3-text/css3-text-decoration/no-gap-between-two-rounded-textboxes-expected.png: Added.
> 
> Could we make this a reference test somehow? Maybe use a canvas to draw the underline?

Agreed, I don't think a ref test would be hard.

[Myles C. Maxfield]

Comment on attachment 215013 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Don't round horizontal underline extents
> 
> This actually doesn't describe the user-visible symptom (underline gaps).

Done.

>> Source/WebCore/ChangeLog:27
>> +        of the disputed pixel will win.
> 
> Next time, please write a much smaller comment, and do it per-function.

I'm not quite sure what you mean by "per-function", but I made it smaller. Do you mean that I should move it below after the bullet points?

>> Source/WebCore/ChangeLog:32
>> +        (WebCore::GraphicsContext::drawLineForText): Second part (See above)
> 
> I would say something like: “Round each end of the underline to a pixel boundary.”

Done.

>> Source/WebCore/ChangeLog:34
>> +        (WebCore::InlineTextBox::paint): First part (See above)
> 
> I would say something like: “Don’t convert to a LayoutSize just to convert to a FloatSize.”

Done.

>> LayoutTests/ChangeLog:14
>> +        and that this "gap" pixel is filled in.
> 
> This comment is also too long, and more importantly, it would be much more appropriate in the test rather than in the change log.

Done.

>>> LayoutTests/ChangeLog:17
>>> +        * platform/mac/fast/css3-text/css3-text-decoration/no-gap-between-two-rounded-textboxes-expected.png: Added.
>> 
>> Could we make this a reference test somehow? Maybe use a canvas to draw the underline?
> 
> Agreed, I don't think a ref test would be hard.

Done.

[Myles C. Maxfield]

Created attachment 215089 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 215089 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Fill in gaps in underlines between adjacent text runs
> +        https://bugs.webkit.org/show_bug.cgi?id=123236

Bug titles should describe problems, not fixes. "Gaps between adjacent underlined text runs"

> LayoutTests/ChangeLog:3
> +        Don't round horizontal underline extents

Still wrong.

[Myles C. Maxfield]

Created attachment 215092 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 215092 [details]
Patch

Rejecting attachment 215092 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 215092, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/10218151

[Myles C. Maxfield]

Created attachment 215094 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 215094 [details]
Patch

Perfect.

[Myles C. Maxfield]

\o/

[WebKit Commit Bot]

Comment on attachment 215094 [details]
Patch

Clearing flags on attachment: 215094

Committed r157948: <http://trac.webkit.org/changeset/157948>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.