Bug 33247

Summary: Upwards cursor movement incorrect when previous block ends with <br>
Product: WebKit Reporter: Julie Parent <jparent>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dglazkov, enrica, justin.garcia, mitz, rniwa, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
Patch
none
Patch
none
fixes the bug
none
rebaselined the test tony: review+

Julie Parent
Reported 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.
Attachments
Patch (4.78 KB, patch)
2010-02-11 22:05 PST, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch (5.16 KB, patch)
2010-02-21 17:13 PST, Tony Chang 		no flags
DetailsFormatted DiffDiff
fixes the bug (9.39 KB, patch)
2011-08-05 14:08 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
rebaselined the test (10.03 KB, patch)
2011-08-05 15:13 PDT, Ryosuke Niwa 		tony: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Tony Chang
Comment 1 2010-02-11 22:05:05 PST
Created attachment 48625 [details] Patch
Tony Chang
Comment 2 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>.
Eric Seidel (no email)
Comment 3 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)?
Tony Chang
Comment 4 2010-02-21 17:13:53 PST
Created attachment 49179 [details] Patch
Tony Chang
Comment 5 2010-02-21 17:14:36 PST
Comment on attachment 49179 [details] Patch Good idea. Pulled out a function named canHaveCursor.
Eric Seidel (no email)
Comment 6 2010-03-05 13:45:28 PST
Comment on attachment 49179 [details] Patch OK.
WebKit Commit Bot
Comment 7 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>
WebKit Commit Bot
Comment 8 2010-03-05 23:28:39 PST
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 9 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.
Ryosuke Niwa
Comment 10 2011-08-05 14:08:37 PDT
Created attachment 103110 [details] fixes the bug
WebKit Review Bot
Comment 11 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
Ryosuke Niwa
Comment 12 2011-08-05 15:13:35 PDT
Created attachment 103120 [details] rebaselined the test
Tony Chang
Comment 13 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?
Ryosuke Niwa
Comment 14 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.
Ryosuke Niwa
Comment 15 2011-08-05 16:41:57 PDT
Committed r92526: <http://trac.webkit.org/changeset/92526>
Note You need to log in before you can comment on or make changes to this bug.