Bug 144167

Summary:

Simple line layout: Wrong text offsetting when range does not start from the first renderer.

Product:

WebKit

Reporter:

alan <zalan>

Component:

Layout and Rendering

Assignee:

alan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, koivisto, rniwa, simon.fraser

Priority:

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

alan

Reported 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.

Attachments
Patch (6.72 KB, patch) 2015-04-24 16:09 PDT, alan	no flags	Details Formatted Diff Diff
Patch (6.75 KB, patch) 2015-04-24 16:31 PDT, alan	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-mavericks (734.52 KB, application/zip) 2015-04-24 16:49 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (719.67 KB, application/zip) 2015-04-24 17:04 PDT, Build Bot	no flags	Details
Patch (6.86 KB, patch) 2015-04-27 09:56 PDT, alan	no flags	Details Formatted Diff Diff
Patch (6.84 KB, patch) 2015-04-27 11:04 PDT, alan	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

alan

Comment 1 2015-04-24 15:53:31 PDT

rdar://problem/20639857

alan

Comment 2 2015-04-24 16:09:24 PDT

Created attachment 251583 [details] Patch

Simon Fraser (smfr)

Comment 3 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.

alan

Comment 4 2015-04-24 16:31:02 PDT

Created attachment 251588 [details] Patch

Simon Fraser (smfr)

Comment 5 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>

Build Bot

Comment 6 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

Build Bot

Comment 7 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

Build Bot

Comment 8 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

Build Bot

Comment 9 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

alan

Comment 10 2015-04-27 09:56:27 PDT

Created attachment 251748 [details] Patch

Darin Adler

Comment 11 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?

alan

Comment 12 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)

alan

Comment 13 2015-04-27 11:04:46 PDT

Created attachment 251759 [details] Patch

WebKit Commit Bot

Comment 14 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>

WebKit Commit Bot

Comment 15 2015-04-27 13:20:32 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.