Bug 145393 – Subpixel rendering: Pixel crack in text selection of simple text in <textarea>.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

1. Open <https://bugs.webkit.org/enter_bug.cgi?product=WebKit>
2. Paste "Inspector doesn't do this. I have been aware of this " (with trailing space) into Desc.
3. Select the "this " at the end.

Notice the pixel crack between 'this' and ' '

[alan]

rdar://problem/19918941

[alan]

Created attachment 253755 [details]
Patch

[Build Bot]

Comment on attachment 253755 [details]
Patch

Attachment 253755 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5313444962107392

New failing tests:
platform/mac/editing/input/caret-primary-bidi.html

[Build Bot]

Created attachment 253758 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[alan]

Created attachment 253767 [details]
Patch

[alan]

Created attachment 253768 [details]
Patch

[Darin Adler]

Comment on attachment 253768 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Float to LayoutUnit conversion is lossy. To ensure that selection
> +        painting always lines up (snaps) properly, the calculated width needs to
> +        be adjusted by ceiling the float to the next LayoutUnit value.

What’s the default? To round instead of doing a ceil?

I find this change really mysterious. When would we need to do this? Why do we have a default conversion from float to LayoutUnit if it’s not always the right thing to do?

[alan]

Comment on attachment 253768 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        be adjusted by ceiling the float to the next LayoutUnit value.
> 
> What’s the default? To round instead of doing a ceil?
> 
> I find this change really mysterious. When would we need to do this? Why do we have a default conversion from float to LayoutUnit if it’s not always the right thing to do?

The default behavior is explicit floor through integer arithmetics. LayoutUnit has int m_value which in case of LayoutUnit(float floatValue) becomes m_value = floatValue * 64.
I assume the reason why we ended up with truncation was because of the potential performance impact (calling ceil() vs. integer arithmetics back then) and expect a few cases, truncated values just worked fine (we are talking about 1/64 of a pixel).
::fromFloatCeil() was introduced at the very beginning of the subpixel project when WebCore became LayoutUnit aware to compensate for the lossyness in cases where we have to ensure that the converted value is never smaller than the original (container sizing for example to prevent the content from getting wrapped). This is mainly the case for inline content as text measuring is float based and while transferring (width)values to block level, we have to make sure that the float -> LayoutUnit() conversion does not result in smaller container than its content's size.

[WebKit Commit Bot]

Comment on attachment 253768 [details]
Patch

Clearing flags on attachment: 253768

Committed r184970: <http://trac.webkit.org/changeset/184970>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This test fails on Windows, Gtk and Efl. Could you please update the results?

[alan]

(In reply to comment #11)
> This test fails on Windows, Gtk and Efl. Could you please update the results?
Thanks for the headsup.
https://trac.webkit.org/changeset/184986