Bug 64140 - REGRESSION: Pressing return in a particular document sends the cursor to the end of the document
Summary: REGRESSION: Pressing return in a particular document sends the cursor to the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-07-07 18:01 PDT by Justin Garcia
Modified: 2011-07-18 01:12 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.61 KB, patch)
2011-07-16 17:17 PDT, Enrica Casucci
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 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>
Comment 1 Justin Garcia 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.
Comment 2 Annie Sullivan 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).
Comment 3 Enrica Casucci 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.
Comment 4 Enrica Casucci 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.
Comment 5 Enrica Casucci 2011-07-16 17:17:15 PDT
Created attachment 101105 [details]
Patch
Comment 6 Enrica Casucci 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.
Comment 7 Simon Fraser (smfr) 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?
Comment 8 Enrica Casucci 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!
Comment 9 Enrica Casucci 2011-07-16 17:34:34 PDT
Committed revision 91158.
Comment 10 Ryosuke Niwa 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.
Comment 11 Enrica Casucci 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.
Comment 12 Ryosuke Niwa 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).
Comment 13 Ryosuke Niwa 2011-07-18 01:12:49 PDT
Chromium rebaseline: http://trac.webkit.org/changeset/91172
GTK / Qt rebaselines: http://trac.webkit.org/changeset/91175