Bug 144167

Summary: Simple line layout: Wrong text offsetting when range does not start from the first renderer.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, koivisto, rniwa, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-mavericks
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Patch
none
Patch none

Description zalan 2015-04-24 15:52:31 PDT 
<div>foobar1<br>foobar2<br>foobar3<br>foobar4<br>foobar5</div>
1. multiple renderers are present in the render flow.
2. Render flow uses simple line layout.
3. Range start node is not set to the first node in the flow.
When text is called on Range, TextIterator returns the wrong selection.
Comment 1 zalan 2015-04-24 15:53:31 PDT 
rdar://problem/20639857
Comment 2 zalan 2015-04-24 16:09:24 PDT 
Created attachment 251583 [details]
Patch
Comment 3 Simon Fraser (smfr) 2015-04-24 16:20:34 PDT 
Comment on attachment 251583 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:369
> +    // Calculate the text offset for simple lines.
> +    Node* node = m_node->previousSibling();
> +    while (node) {
> +        RenderObject* renderer = node->renderer();
> +        if (renderer && is<RenderText>(renderer))
> +            m_previousTextLengthInFlow += downcast<RenderText>(renderer)->textLength();
> +        node = node->previousSibling();
> +    }

I think this should happen later when we know we're hitting a SLL code path.
Comment 4 zalan 2015-04-24 16:31:02 PDT 
Created attachment 251588 [details]
Patch
Comment 5 Simon Fraser (smfr) 2015-04-24 16:35:21 PDT 
Comment on attachment 251588 [details]
Patch

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

> LayoutTests/fast/text/range-text-with-simple-line-layout.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>
Comment 6 Build Bot 2015-04-24 16:49:44 PDT 
Comment on attachment 251588 [details]
Patch

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

New failing tests:
editing/deleting/delete-blockquote-large-offsets.html
fast/spatial-navigation/snav-iframe-no-focusable-content.html
editing/style/bold-with-dom-changes.html
fast/parser/script-modify-page-outer.html
fast/css/pseudo-empty-display-none.html
editing/inserting/caret-position.html
accessibility/aria-labelledby-with-descendants.html
fast/files/file-reader-file-url.html
fast/xmlhttprequest/xmlhttprequest-no-file-access.html
fast/files/file-reader-sandbox-iframe.html
Comment 7 Build Bot 2015-04-24 16:49:47 PDT 
Created attachment 251593 [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
Comment 8 Build Bot 2015-04-24 17:04:30 PDT 
Comment on attachment 251588 [details]
Patch

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

New failing tests:
editing/deleting/delete-blockquote-large-offsets.html
accessibility/aria-labelledby-with-descendants.html
fast/parser/script-modify-page-outer.html
fast/css/pseudo-empty-display-none.html
fast/files/file-reader-sandbox-iframe.html
fast/files/file-reader-file-url.html
fast/xmlhttprequest/xmlhttprequest-no-file-access.html
Comment 9 Build Bot 2015-04-24 17:04:34 PDT 
Created attachment 251596 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 10 zalan 2015-04-27 09:56:27 PDT 
Created attachment 251748 [details]
Patch
Comment 11 Darin Adler 2015-04-27 09:58:50 PDT 
Comment on attachment 251588 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:509
> +    Node* node = firstTextNodeInRange.previousSibling();
> +    while (node) {

Should be a for loop, not a while loop.

> Source/WebCore/editing/TextIterator.cpp:512
> +        if (renderer && is<RenderText>(renderer))
> +            textOffset += downcast<RenderText>(renderer)->textLength();

What about a <br> or any other element that is treated as text by the text iterator? Just adding the literal text lengths of RenderText can’s be sufficient can it?
Comment 12 zalan 2015-04-27 10:02:59 PDT 
(In reply to comment #11)
> Comment on attachment 251588 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251588&action=review
> 
> > Source/WebCore/editing/TextIterator.cpp:509
> > +    Node* node = firstTextNodeInRange.previousSibling();
> > +    while (node) {
> 
> Should be a for loop, not a while loop.
> 
Indeed. Thanks!

> > Source/WebCore/editing/TextIterator.cpp:512
> > +        if (renderer && is<RenderText>(renderer))
> > +            textOffset += downcast<RenderText>(renderer)->textLength();
> 
> What about a <br> or any other element that is treated as text by the text
> iterator? Just adding the literal text lengths of RenderText can’s be
> sufficient can it?
This is only used by simple line layout code path, so other text elements should not be included yet. (<br> does not have a length, only position)
Comment 13 zalan 2015-04-27 11:04:46 PDT 
Created attachment 251759 [details]
Patch
Comment 14 WebKit Commit Bot 2015-04-27 13:20:27 PDT 
Comment on attachment 251759 [details]
Patch

Clearing flags on attachment: 251759

Committed r183413: <http://trac.webkit.org/changeset/183413>
Comment 15 WebKit Commit Bot 2015-04-27 13:20:32 PDT 
All reviewed patches have been landed.  Closing bug.