Bug 64140

Summary: REGRESSION: Pressing return in a particular document sends the cursor to the end of the document
Product: WebKit Reporter: Justin Garcia <justin.garcia>
Component: HTML EditingAssignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: enrica, leviw, rniwa, sullivan, wcarss
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch simon.fraser: review+

Justin Garcia
Reported 2011-07-07 18:01:43 PDT
Open the following test case in Safari: <body contentEditable="true"><b>place the cursor between these two lines<br><br>and press return</b><br></body> and place the cursor between the two lines and press return. The cursor moves to the end of the document. <rdar://problem/9737491>
Attachments
Patch (6.61 KB, patch)
2011-07-16 17:17 PDT, Enrica Casucci 		simon.fraser: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Justin Garcia
Comment 1 2011-07-07 18:04:34 PDT
Using a WebKit nightly build from r90560 on Mac OS 10.6.7. Does not reproduce with Safari 5.0.5 on the same OS.
Annie Sullivan
Comment 2 2011-07-07 18:09:56 PDT
This sounds really similar to bug 61594, but I can reproduce this test case and not the one in that bug. It is probably in the same codepath (InsertParagraphSeparatorCommand).
Enrica Casucci
Comment 3 2011-07-16 16:11:34 PDT
I'm not sure exactly where this regressed. I suspect http://trac.webkit.org/changeset/83247 where the cloning of the hierarchy was removed. The bug seems to reproduce always when the insertion point is over a BR element inside an inline. I've implemented a patch that detects when the insertion point is a BR element and simply adds another BR element, without trying to create another block and move stuff around. I'll run all the regression tests to see what breaks. The approach is promising, because it produces a much simpler markup.
Enrica Casucci
Comment 4 2011-07-16 16:20:38 PDT
Only one test is currently failing, but the visual result is correct. The failure is due to the different markup being generated.
Enrica Casucci
Comment 5 2011-07-16 17:17:15 PDT
Created attachment 101105 [details] Patch
Enrica Casucci
Comment 6 2011-07-16 17:24:25 PDT
This patch will probably break the test I've updated the results for in other platforms. I will fix the results as they become available.
Simon Fraser (smfr)
Comment 7 2011-07-16 17:24:28 PDT
Comment on attachment 101105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101105&action=review > Source/WebCore/ChangeLog:9 > + The fix consists in detecting that the insertion point is a break element and simply insert another insert -> inserting > Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:307 > + if (visiblePos.deepEquivalent().anchorNode()->renderer()->isBR()) { Is anchorNode() guaranteed to have a renderer?
Enrica Casucci
Comment 8 2011-07-16 17:28:45 PDT
(In reply to comment #7) > (From update of attachment 101105 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101105&action=review > > > Source/WebCore/ChangeLog:9 > > + The fix consists in detecting that the insertion point is a break element and simply insert another > > insert -> inserting > > > Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:307 > > + if (visiblePos.deepEquivalent().anchorNode()->renderer()->isBR()) { > > Is anchorNode() guaranteed to have a renderer? Yes, because is from a visibleposition. thanks for the review!
Enrica Casucci
Comment 9 2011-07-16 17:34:34 PDT
Committed revision 91158.
Ryosuke Niwa
Comment 10 2011-07-16 17:48:59 PDT
Comment on attachment 101105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101105&action=review >> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:307 >> + if (visiblePos.deepEquivalent().anchorNode()->renderer()->isBR()) { > > Is anchorNode() guaranteed to have a renderer? Are you sure we we need to exit early both when the insertion position is before BR and after BR? What if visiblePos was before another block element such as hr? Shouldn't be also exit early in the case? I'm not convinced that we should be exiting early here if visiblePos was after BR.
Enrica Casucci
Comment 11 2011-07-16 18:10:06 PDT
(In reply to comment #10) > (From update of attachment 101105 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101105&action=review > > >> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:307 > >> + if (visiblePos.deepEquivalent().anchorNode()->renderer()->isBR()) { > > > > Is anchorNode() guaranteed to have a renderer? > > Are you sure we we need to exit early both when the insertion position is before BR and after BR? What if visiblePos was before another block element such as hr? Shouldn't be also exit early in the case? I'm not convinced that we should be exiting early here if visiblePos was after BR. If we are at the beginning of another block but not on a BR, then we don't exit early. if the markup is <b>one<br><br>two</br> and the insertion point is on "two" we don't exit early.
Ryosuke Niwa
Comment 12 2011-07-16 18:57:02 PDT
(In reply to comment #11) > (In reply to comment #10) > > Are you sure we we need to exit early both when the insertion position is before BR and after BR? What if visiblePos was before another block element such as hr? Shouldn't be also exit early in the case? I'm not convinced that we should be exiting early here if visiblePos was after BR. > If we are at the beginning of another block but not on a BR, then we don't exit early. But we could still be anchored at BR in those cases, right? e.g. if we had <b>one<br><br>two</br>, we can have (two, PositionIsOffsetInAnchor, 0) but we might as well as have (second br, PositionIsAfterAnchor).
Ryosuke Niwa
Comment 13 2011-07-18 01:12:49 PDT
Chromium rebaseline: http://trac.webkit.org/changeset/91172 GTK / Qt rebaselines: http://trac.webkit.org/changeset/91175
Note You need to log in before you can comment on or make changes to this bug.