Bug 88000 - Block selection gaps and inline selection heights are not properly pixel snapped
Summary: Block selection gaps and inline selection heights are not properly pixel snapped
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL: http://www.spiegel.de/politik/ausland...
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-31 12:08 PDT by Levi Weintraub
Modified: 2012-06-05 17:46 PDT (History)
7 users (show)

See Also:


Attachments
Patch (26.07 KB, patch)
2012-05-31 12:27 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (1.01 MB, application/zip)
2012-05-31 19:38 PDT, WebKit Review Bot
no flags Details
Patch (763.51 KB, patch)
2012-06-01 14:56 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (763.96 KB, patch)
2012-06-04 16:14 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch for landing (777.05 KB, patch)
2012-06-05 10:13 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2012-05-31 12:08:12 PDT
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
Comment 1 Levi Weintraub 2012-05-31 12:27:28 PDT
Created attachment 145133 [details]
Patch
Comment 2 Levi Weintraub 2012-05-31 19:11:01 PDT
Whew... cr-linux has had quite the slow day today! Anyhow, pinging one Eric Seidel :)
Comment 3 WebKit Review Bot 2012-05-31 19:38:02 PDT
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
Comment 4 WebKit Review Bot 2012-05-31 19:38:06 PDT
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
Comment 5 Levi Weintraub 2012-05-31 19:45:12 PDT
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.
Comment 6 Levi Weintraub 2012-06-01 14:44:35 PDT
Updating the bug now that I believe I've resolved all issues (patch forthcoming)
Comment 7 Levi Weintraub 2012-06-01 14:56:44 PDT
Created attachment 145383 [details]
Patch
Comment 8 Levi Weintraub 2012-06-01 18:49:48 PDT
Ping :)
Comment 9 Levi Weintraub 2012-06-04 16:14:39 PDT
Created attachment 145650 [details]
Patch
Comment 10 Ryosuke Niwa 2012-06-04 17:14:54 PDT
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?
Comment 11 Ryosuke Niwa 2012-06-04 18:34:36 PDT
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.
Comment 12 Levi Weintraub 2012-06-05 10:13:04 PDT
Created attachment 145827 [details]
Patch for landing
Comment 13 Levi Weintraub 2012-06-05 14:56:42 PDT
Comment on attachment 145827 [details]
Patch for landing

Clearing flags on attachment: 145827

Committed r119528: <http://trac.webkit.org/changeset/119528>
Comment 14 Levi Weintraub 2012-06-05 14:56:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 mitz 2012-06-05 17:46:20 PDT
Filed bug 88382.