| Summary: | Moving caret up or down skips lines when there's a non-editable line | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
| Component: | HTML Editing | Assignee: | 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: |
|
||||||||
Created attachment 140580 [details]
proposal patch
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. 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. (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. Created attachment 142386 [details]
Fixes the bug
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? 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. (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. Comment on attachment 142386 [details]
Fixes the bug
OK.
Comment on attachment 142386 [details] Fixes the bug (In reply to comment #9) > (From update of attachment 142386 [details]) > OK. Thanks for the review :) Comment on attachment 142386 [details] Fixes the bug Clearing flags on attachment: 142386 Committed r117392: <http://trac.webkit.org/changeset/117392> All reviewed patches have been landed. Closing bug. |
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