81490 – Moving caret up or down skips lines when there's a non-editable line

RESOLVED FIXED 81490

Moving caret up or down skips lines when there's a non-editable line

https://bugs.webkit.org/show_bug.cgi?id=81490

Summary Moving caret up or down skips lines when there's a non-editable line

Ryosuke Niwa

Reported 2012-03-18 21:59:03 PDT

Open the following page and try to move caret up or down from line 2. WebKit skips lines and move to the beginning of the end of the editable region. <!DOCTYPE html> <html style="background:none transparent scroll repeat 0 0;"> <head> <title> Repro Keyboard Skips Line</title> </head> <body contenteditable=true style="height: 100%;" tabindex=1> Line 1 editable <div contenteditable=false style="background: red"> Uneditable div 1 </div> Line 2 editable <div contenteditable=false style="background: red"> Uneditable div 2 </div> Line 3 editable </body> </html> http://crbug.com/118695

Attachments
proposal patch (7.43 KB, patch) 2012-05-07 13:50 PDT, Yi Shen	no flags	Details Formatted Diff Diff
Fixes the bug (8.74 KB, patch) 2012-05-16 18:24 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Yi Shen

Comment 1 2012-05-07 13:50:37 PDT

Created attachment 140580 [details] proposal patch

Ryosuke Niwa

Comment 2 2012-05-07 14:03:43 PDT

Comment on attachment 140580 [details] proposal patch View in context: https://bugs.webkit.org/attachment.cgi?id=140580&action=review Thanks for tackling this bug but r- because we also need to fix previousRootInlineBox and nextRootInlineBox. As I have attempted, ideally, we would like to merge previousRootInlineBox and nextRootInlineBox with code in previousLinePosition and nextLinePosition before fixing this bug. > LayoutTests/editing/selection/move-by-line-006-expected.txt:6 > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS Please make sure the test output shows what we're testing.

Yi Shen

Comment 3 2012-05-07 14:15:36 PDT

Thanks for the review :) Niwa, is there any plan to fix the cr-linux failure for Bug 81593. I saw it blocked several editing bugs, which is not good :( (In reply to comment #2) > (From update of attachment 140580 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140580&action=review > > Thanks for tackling this bug but r- because we also need to fix previousRootInlineBox and nextRootInlineBox. > > As I have attempted, ideally, we would like to merge previousRootInlineBox and nextRootInlineBox with code in previousLinePosition and nextLinePosition before fixing this bug. > > > LayoutTests/editing/selection/move-by-line-006-expected.txt:6 > > +PASS > > +PASS > > +PASS > > +PASS > > +PASS > > +PASS > > Please make sure the test output shows what we're testing.

Ryosuke Niwa

Comment 4 2012-05-07 14:24:26 PDT

(In reply to comment #3) > Thanks for the review :) Niwa, is there any plan to fix the cr-linux failure for Bug 81593. I saw it blocked several editing bugs, which is not good :( Unfortunately, I don't have access to cr-linux bot but I'm intending to re-do the refactoring.

Ryosuke Niwa

Comment 5 2012-05-16 18:24:28 PDT

Created attachment 142386 [details] Fixes the bug

Eric Seidel (no email)

Comment 6 2012-05-16 18:45:36 PDT

Comment on attachment 142386 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=142386&action=review > Source/WebCore/editing/visible_units.cpp:106 > + while (nextNode && inSameLine(firstPositionInOrBeforeNode(nextNode), visiblePosition)) How much more expensive is this version of the check? (and do we care?) > LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt:90 > +" abc def AAA AAA hij AAA AAA uvw xyz "[1, 5, 8, 12, 16, 20, 24, 28, 32, 36], <DIV>[0], <DIV>[0], "AAA kj AAA mn opq AAA AAA"[3, 6, 10, 13, 17, 21, 25] FAIL expected: [" abc def AAA AAA hij AAA AAA uvw xyz "[ 1, 5, 8, 12, 16, 20, 24, 28, 32, 36, ]"AAA kj AAA mn opq AAA AAA"[ 3, 6, 10, 13, 17, 21, 25] The two div[0] in a row look suspicious... is that intended?

Ryosuke Niwa

Comment 7 2012-05-16 18:49:30 PDT

Comment on attachment 142386 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=142386&action=review >> LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt:90 >> +" abc def AAA AAA hij AAA AAA uvw xyz "[1, 5, 8, 12, 16, 20, 24, 28, 32, 36], <DIV>[0], <DIV>[0], "AAA kj AAA mn opq AAA AAA"[3, 6, 10, 13, 17, 21, 25] FAIL expected: [" abc def AAA AAA hij AAA AAA uvw xyz "[ 1, 5, 8, 12, 16, 20, 24, 28, 32, 36, ]"AAA kj AAA mn opq AAA AAA"[ 3, 6, 10, 13, 17, 21, 25] > > The two div[0] in a row look suspicious... is that intended? We have 3 empty lines in this test, and we're supposed to skip them all. The current behavior is that we don't skip the first empty line but skip the second and third empty lines. After this patch, we don't skip first and second empty lines but skip the last empty line. I don't think the new behavior is necessarily worse.

Ryosuke Niwa

Comment 8 2012-05-16 18:55:34 PDT

(In reply to comment #6) > (From update of attachment 142386 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142386&action=review > > > Source/WebCore/editing/visible_units.cpp:106 > > + while (nextNode && inSameLine(firstPositionInOrBeforeNode(nextNode), visiblePosition)) > > How much more expensive is this version of the check? (and do we care?) It is significantly slower but I don't see a way around it other than re-writing the function to talk on render tree instead of DOM tree. The last time xji tried it, we ended up introducing a security vulnerability but I suppose we can take that route the performance of this function matters. Also, this function is only called when the user is trying to caret forward or backward between lines and AccessibilityObject::lineForPosition. I don't think the accessibility code should be using this function anyway per FIXME in the code.

Eric Seidel (no email)

Comment 9 2012-05-16 19:09:14 PDT

Comment on attachment 142386 [details] Fixes the bug OK.

Ryosuke Niwa

Comment 10 2012-05-16 19:10:02 PDT

Comment on attachment 142386 [details] Fixes the bug (In reply to comment #9) > (From update of attachment 142386 [details]) > OK. Thanks for the review :)

WebKit Review Bot

Comment 11 2012-05-16 21:09:03 PDT

Comment on attachment 142386 [details] Fixes the bug Clearing flags on attachment: 142386 Committed r117392: <http://trac.webkit.org/changeset/117392>

WebKit Review Bot

Comment 12 2012-05-16 21:09:09 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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component HTML Editing

Assignee

Ryosuke Niwa

Reported

2012-03-18 21:59 PDT

Modified

2012-05-16 21:09 PDT History

CC List

8 users Show

URL

Keywords HasReduction

Depends on

81593

Blocks

Dependencies

tree graph