Bug 33247 - Upwards cursor movement incorrect when previous block ends with <br>
: Upwards cursor movement incorrect when previous block ends with <br>
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-01-05 18:28 PST by
Modified: 2011-08-05 16:41 PST (History)


Attachments
Patch (4.78 KB, patch)
2010-02-11 22:05 PST, Tony Chang
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.16 KB, patch)
2010-02-21 17:13 PST, Tony Chang
no flags Review Patch | Details | Formatted Diff | Diff
fixes the bug (9.39 KB, patch)
2011-08-05 14:08 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
rebaselined the test (10.03 KB, patch)
2011-08-05 15:13 PST, Ryosuke Niwa
tony: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-01-05 18:28:31 PST
Normally, if you press the up arrow, the cursor moves to somewhere close to being right above where you were.  However, if the previous line is a block and ends in a br, the cursor always goes to the end of the line instead.

See http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cdiv%20contentEditable%3E%3Cp%3Efirst%20line.%20blah%20blah%20blah.%20This%20ends%20in%20a%20br.%3Cbr%2F%3E%3C%2Fp%3E%3Cp%3Esecond%20line.%20Put%20your%20cursor%20here%20%5B%20%5D.%20Now%20hit%20up.%3C%2Fp%3E%3Cp%3EAnother%20line%20line.%20This%20does%20not%20have%20a%20br.%3C%2Fp%3E%3Cp%3Etry%20again%20from%20down%20here%20%5B%20%5D%20and%20hit%20up.%3C%2Fp%3E%3C%2Fdiv%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1 for an example.

This seems to happen whenever the <br> doesn't cause a linebreak - that is, if the <p>'s in the example above are replaced with <spans>, this does not happen.

Seen in Safari 4.0.4 and Chrome 4.0.266.0.
------- Comment #1 From 2010-02-11 22:05:05 PST -------
Created an attachment (id=48625) [details]
Patch
------- Comment #2 From 2010-02-11 22:06:38 PST -------
(In reply to comment #1)
> Created an attachment (id=48625) [details] [details]
> Patch

In theory I should have to do a similar check in nextLinePosition, but I couldn't come up with a test case where we have an inline element that takes up no space at the beginning of a block like a <br/> right before a </p>.
------- Comment #3 From 2010-02-19 13:40:35 PST -------
(From update of attachment 48625 [details])
Maybe this check should move into a function with a nice name?
((o->isText() && toRenderText(o)->linesBoundingBox().height())
 593                     || (o->isBox() && toRenderBox(o)->borderBoundingBox().height())) {

like canHaveCursor(o)?
------- Comment #4 From 2010-02-21 17:13:53 PST -------
Created an attachment (id=49179) [details]
Patch
------- Comment #5 From 2010-02-21 17:14:36 PST -------
(From update of attachment 49179 [details])
Good idea.  Pulled out a function named canHaveCursor.
------- Comment #6 From 2010-03-05 13:45:28 PST -------
(From update of attachment 49179 [details])
OK.
------- Comment #7 From 2010-03-05 23:28:35 PST -------
(From update of attachment 49179 [details])
Clearing flags on attachment: 49179

Committed r55613: <http://trac.webkit.org/changeset/55613>
------- Comment #8 From 2010-03-05 23:28:39 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #9 From 2011-08-05 11:52:40 PST -------
This is still broken.  To reproduce the bug, simply add <!DOCTYPE html> to the test added by r55613.
------- Comment #10 From 2011-08-05 14:08:37 PST -------
Created an attachment (id=103110) [details]
fixes the bug
------- Comment #11 From 2011-08-05 15:05:58 PST -------
(From update of attachment 103110 [details])
Attachment 103110 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9306985

New failing tests:
editing/execCommand/move-selection-back-line-rtl.html
------- Comment #12 From 2011-08-05 15:13:35 PST -------
Created an attachment (id=103120) [details]
rebaselined the test
------- Comment #13 From 2011-08-05 16:27:05 PST -------
(From update of attachment 103120 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=103120&action=review

> LayoutTests/editing/execCommand/move-selection-back-line-rtl.html:6
> +<p>Move the caret from [ ] in the second line to [ ] in the first line.</p>

It would be nice if this description were more specific like the old test description (what to do and what should happen).

> LayoutTests/editing/execCommand/move-selection-back-line-strict.html:1
> +<div contentEditable="true" style="font-family: monospace;">

Did you mean to add a <!DOCTYPE html> here?
------- Comment #14 From 2011-08-05 16:30:23 PST -------
(From update of attachment 103120 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=103120&action=review

Thanks for the review.

>> LayoutTests/editing/execCommand/move-selection-back-line-rtl.html:6
>> +<p>Move the caret from [ ] in the second line to [ ] in the first line.</p>
> 
> It would be nice if this description were more specific like the old test description (what to do and what should happen).

Will fix.

>> LayoutTests/editing/execCommand/move-selection-back-line-strict.html:1
>> +<div contentEditable="true" style="font-family: monospace;">
> 
> Did you mean to add a <!DOCTYPE html> here?

Huh that's odd.  Apparently svn diff didn't pick up my change to this file.  Will fix.
------- Comment #15 From 2011-08-05 16:41:57 PST -------
Committed r92526: <http://trac.webkit.org/changeset/92526>