Bug 122365 – Include misspelling dot gap width when centering misspelling dots

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Include misspelling dot gap width when centering misspelling dots

[Myles C. Maxfield]

Created attachment 214881 [details]
Patch

[Myles C. Maxfield]

Created attachment 214897 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/15299813>

[Jon Lee]

Comment on attachment 214897 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        (WebCore::GraphicsContext::drawLineForDocumentMarker):

More explanation here please. Why is there a new (widthMod > 0) check? What is the new code doing to the offset? Why is it a floor()?

> LayoutTests/editing/spelling/centering-misspelling-dots-expected.txt:58
> +caret: position 14 of child 0 {#text} of child 3 {DIV} of body

Is having expected text useful for this test?

[Myles C. Maxfield]

Comment on attachment 214897 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        (WebCore::GraphicsContext::drawLineForDocumentMarker):
> 
> More explanation here please. Why is there a new (widthMod > 0) check? What is the new code doing to the offset? Why is it a floor()?

Done.

>> LayoutTests/editing/spelling/centering-misspelling-dots-expected.txt:58
>> +caret: position 14 of child 0 {#text} of child 3 {DIV} of body
> 
> Is having expected text useful for this test?

I don't think it's possible to run a pixel test without expected text. When I try, it says that the test failed due to missing results.

[Myles C. Maxfield]

Created attachment 214985 [details]
Patch

[Jon Lee]

Comment on attachment 214985 [details]
Patch

Provisional r=me.

[Simon Fraser (smfr)]

Comment on attachment 214985 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:-170
> -    // NOTE: Code here used to shift the underline to the left and increase the width
> -    // to make sure everything gets underlined, but that results in drawing out of
> -    // bounds (e.g. when at the edge of a view) and could make it appear that the
> -    // space between adjacent misspelled words was underlined.

I would like to see your testcase testing "the space between misspelled words".

[Myles C. Maxfield]

Created attachment 215004 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 215004 [details]
Patch

Clearing flags on attachment: 215004

Committed r157900: <http://trac.webkit.org/changeset/157900>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.