Bug 81490

Summary: Moving caret up or down skips lines when there's a non-editable line
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cshu, darin, enrica, max.hong.shen, tonikitoo, webkit.review.bot, xji
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 81593    
Bug Blocks:    
Attachments:
Description Flags
proposal patch
none
Fixes the bug none

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
DetailsFormatted DiffDiff
Fixes the bug (8.74 KB, patch)
2012-05-16 18:24 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
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.