Bug 178232

Summary: Share logic in InlineTextBox to compute selection rect
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Layout and RenderingAssignee: Daniel Bates <dbates>
Status: REOPENED ---    
Severity: Normal CC: bfulgham, buildbot, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=138913
https://bugs.webkit.org/show_bug.cgi?id=135631
Bug Depends on: 178611, 178671    
Bug Blocks: 175784, 178613    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
zalan: review+
Work-in-progress
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 none

Description Daniel Bates 2017-10-12 14:12:10 PDT
Currently each paint routine in InlineTextBox computes the selection rect it will paint using the same algorithm or an algorithm that varies slightly. The slight variations represent simplifications of the algorithm that are specific to what is being painted. For example, InlineTextBox::paintDocumentMarker() takes advantage of the fact that the marker decoration is an underline such that it does not concern itself with overlapping backgrounds as InlineTextBox::paintSelection() needs to do. Therefore, InlineTextBox::paintDocumentMarker() can assume the height of the marker is the height of selected text in the box ignoring overlap (call InlineTextBox::selectionHeight()). In contrast, InlineTextBox::paintSelection() must compute the height taking into account the height of any selected text in the last line of the proceeding block (using RootInlineBox::selectionTopAdjustedForPrecedingBlock()) so as make the illusion that the background for all of the selected text on the page was painted in one continuous paint operation as opposed to being painted piecewise. We should share more code.
Comment 1 Radar WebKit Bug Importer 2017-10-12 14:12:27 PDT
<rdar://problem/34963452>
Comment 2 Daniel Bates 2017-10-12 14:38:18 PDT
Created attachment 323564 [details]
Patch
Comment 3 Daniel Bates 2017-10-12 14:40:34 PDT
Created attachment 323566 [details]
Patch
Comment 4 Daniel Bates 2017-10-12 15:09:30 PDT
Created attachment 323575 [details]
Patch
Comment 5 Daniel Bates 2017-10-12 15:52:58 PDT
Comment on attachment 323575 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:677
> +    context.fillRect(snapRectToDevicePixelsWithWritingDirection(selectionRect, renderer().document().deviceScaleFactor(), direction() == LTR), c);

Will substitute isLeftToRightDirection() for "direction() == LTR" before landing.

> Source/WebCore/rendering/InlineTextBox.cpp:699
> +    context.fillRect(snapRectToDevicePixelsWithWritingDirection(selectionRect, renderer().document().deviceScaleFactor(), direction() == LTR), color);

Ditto.
Comment 6 zalan 2017-10-19 11:08:36 PDT
Comment on attachment 323575 [details]
Patch

This patch could use a bit of 'auto'.
Comment 7 Daniel Bates 2017-10-19 11:55:35 PDT
Committed r223699: <https://trac.webkit.org/changeset/223699>
Comment 8 Daniel Bates 2017-10-23 10:28:23 PDT
Reverted r223699 for reason:

Caused regressions with right-to-left text selection and painting of markers in flipped writing mode and in overlapping lines. Will investigate offline.

Committed r223840: <https://trac.webkit.org/changeset/223840>
Comment 9 Daniel Bates 2017-10-23 11:08:08 PDT
Created attachment 324568 [details]
Work-in-progress

Incorporates the source code change from the patch for bug #178611, attachment #324477 [details].
Comment 10 Build Bot 2017-10-23 12:09:08 PDT
Comment on attachment 324568 [details]
Work-in-progress

Attachment 324568 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4960534

New failing tests:
platform/mac/fast/dom/character-index-for-point.html
fast/repaint/selection-ruby-rl.html
editing/spelling/spelling-markers-in-overlapping-lines.html
editing/spelling/spelling-markers-in-overlapping-lines-large-font.html
fast/dom/Range/getClientRects.html
Comment 11 Build Bot 2017-10-23 12:09:10 PDT
Created attachment 324574 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Build Bot 2017-10-23 12:14:15 PDT
Comment on attachment 324568 [details]
Work-in-progress

Attachment 324568 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4960488

New failing tests:
platform/mac/fast/dom/character-index-for-point.html
fast/repaint/selection-ruby-rl.html
editing/spelling/spelling-markers-in-overlapping-lines.html
fast/dom/Range/getClientRects.html
editing/spelling/spelling-markers-in-overlapping-lines-large-font.html
Comment 13 Build Bot 2017-10-23 12:14:19 PDT
Created attachment 324575 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 14 Build Bot 2017-10-23 18:39:56 PDT
Comment on attachment 324568 [details]
Work-in-progress

Attachment 324568 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4964592

New failing tests:
fast/repaint/selection-ruby-rl.html
fast/dom/Range/getClientRects.html
Comment 15 Build Bot 2017-10-23 18:39:57 PDT
Created attachment 324625 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6