Bug 33247 - Upwards cursor movement incorrect when previous block ends with <br>
Summary: Upwards cursor movement incorrect when previous block ends with <br>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-05 18:28 PST by Julie Parent
Modified: 2011-08-05 16:41 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Julie Parent 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 Tony Chang 2010-02-11 22:05:05 PST
Created attachment 48625 [details]
Patch
Comment 2 Tony Chang 2010-02-11 22:06:38 PST
(In reply to comment #1)
> Created an attachment (id=48625) [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 Eric Seidel (no email) 2010-02-19 13:40:35 PST
Comment on attachment 48625 [details]
Patch

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 Tony Chang 2010-02-21 17:13:53 PST
Created attachment 49179 [details]
Patch
Comment 5 Tony Chang 2010-02-21 17:14:36 PST
Comment on attachment 49179 [details]
Patch

Good idea.  Pulled out a function named canHaveCursor.
Comment 6 Eric Seidel (no email) 2010-03-05 13:45:28 PST
Comment on attachment 49179 [details]
Patch

OK.
Comment 7 WebKit Commit Bot 2010-03-05 23:28:35 PST
Comment on attachment 49179 [details]
Patch

Clearing flags on attachment: 49179

Committed r55613: <http://trac.webkit.org/changeset/55613>
Comment 8 WebKit Commit Bot 2010-03-05 23:28:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Ryosuke Niwa 2011-08-05 11:52:40 PDT
This is still broken.  To reproduce the bug, simply add <!DOCTYPE html> to the test added by r55613.
Comment 10 Ryosuke Niwa 2011-08-05 14:08:37 PDT
Created attachment 103110 [details]
fixes the bug
Comment 11 WebKit Review Bot 2011-08-05 15:05:58 PDT
Comment on attachment 103110 [details]
fixes the bug

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 Ryosuke Niwa 2011-08-05 15:13:35 PDT
Created attachment 103120 [details]
rebaselined the test
Comment 13 Tony Chang 2011-08-05 16:27:05 PDT
Comment on attachment 103120 [details]
rebaselined the test

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 Ryosuke Niwa 2011-08-05 16:30:23 PDT
Comment on attachment 103120 [details]
rebaselined the test

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 Ryosuke Niwa 2011-08-05 16:41:57 PDT
Committed r92526: <http://trac.webkit.org/changeset/92526>