Bug 88000 – Block selection gaps and inline selection heights are not properly pixel snapped

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

[Levi Weintraub]

Chromium Bug: http://code.google.com/p/chromium/issues/detail?id=130249

The page referenced in the above bug shows the affect when selecting text with sub-pixel enabled: http://www.spiegel.de/politik/ausland/hollande-erwaegt-militaereinsatz-gegen-assad-in-syrien-a-835846.html

[Levi Weintraub]

Created attachment 145133 [details]
Patch

[Levi Weintraub]

Whew... cr-linux has had quite the slow day today! Anyhow, pinging one Eric Seidel :)

[WebKit Review Bot]

Comment on attachment 145133 [details]
Patch

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

New failing tests:
editing/style/create-block-for-style-006.html
editing/style/block-style-003.html
editing/style/create-block-for-style-011.html
editing/selection/3690703-2.html
editing/style/table-selection.html
editing/style/create-block-for-style-010.html
editing/style/create-block-for-style-005.html
editing/style/create-block-for-style-012.html
editing/selection/3690719.html
editing/selection/3690703.html
fast/sub-pixel/selection/selection-gaps-at-fractional-offsets.html

[WebKit Review Bot]

Created attachment 145198 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Levi Weintraub]

Comment on attachment 145133 [details]
Patch

Looked through the results, and it appears that while this fixed MacOS, we're still in trouble on Linux. I'll loop around tomorrow with a new patch.

[Levi Weintraub]

Updating the bug now that I believe I've resolved all issues (patch forthcoming)

[Levi Weintraub]

Created attachment 145383 [details]
Patch

[Levi Weintraub]

Ping :)

[Levi Weintraub]

Created attachment 145650 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 145650 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:-3407
> +    LayoutUnit rootBlockLogicalRight = min(inlineDirectionOffset(rootBlock, offsetFromRootBlock) + floorToInt(logicalLeft), min(logicalRightSelectionOffset(rootBlock, logicalTop), logicalRightSelectionOffset(rootBlock, logicalTop + logicalHeight)));
>      LayoutUnit rootBlockLogicalWidth = rootBlockLogicalRight - rootBlockLogicalLeft;
>      if (rootBlockLogicalWidth <= ZERO_LAYOUT_UNIT)
>          return LayoutRect();
>  
>      LayoutRect gapRect = rootBlock->logicalRectToPhysicalRect(rootBlockPhysicalPosition, LayoutRect(rootBlockLogicalLeft, rootBlockLogicalTop, rootBlockLogicalWidth, logicalHeight));
> -    alignSelectionRectToDevicePixels(gapRect);

It'll be great if you could explain that the significance here is that floorToInt is applied before computing rootBlockLogicalRight.

> Source/WebCore/rendering/RenderLayer.h:955
> -    LayoutRect m_blockSelectionGapsBounds;
> +    IntRect m_blockSelectionGapsBounds;

Should we also change the type of "rect" local variable in repaintBlockSelectionGaps?

[Ryosuke Niwa]

Comment on attachment 145650 [details]
Patch

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

r=me once my points are addressed :)

> Source/WebCore/ChangeLog:14
> +        We were also unintentionally upcasting LayoutUnits to floats in InlineTextBox's selection painting
> +        routine. Now we're properly rounding.

It would be great to explain why the previous approach was wrong so that other people don't have to figure it out by themselves.

Here's a nice joke that can go along with it:

In one of undergraduate math. classes at Berkeley, a professor was proving a theorem.
In the proof, he said "X is obviously Y, and therefore Z" where X, Y, and Z were some formal statements.
Students were puzzled because it wasn't obvious at all that Y followed X so one of them asked the professor why it was obvious.
"because...", says the professor; he then paused for a second and asked himself "Hm... wait, why is that...".
He spent the next 5 minutes thinking about it without a word, and then concluded his answer by "uh huh! It IS obvious"
without explaining why it was obvious to students.
The only thing students understood was that they never have to prove Y follows X in his class.

[Levi Weintraub]

Created attachment 145827 [details]
Patch for landing

[Levi Weintraub]

Comment on attachment 145827 [details]
Patch for landing

Clearing flags on attachment: 145827

Committed r119528: <http://trac.webkit.org/changeset/119528>

[Levi Weintraub]

All reviewed patches have been landed.  Closing bug.

The test is failing in OS X Lion: <http://build.webkit.org/results/Lion%20Debug%20(Tests)/r119532%20(7368)/fast/sub-pixel/selection/selection-gaps-at-fractional-offsets-pretty-diff.html>

Filed bug 88382.