Bug 55481

Summary: Consider padding and border when looking for the next/previous line position
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, enrica, eric, hyatt, leviw, logingx, mitz, mrobinson, rniwa, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 25533, 59435, 62743    
Attachments:
Description Flags
Patch proposal + Layout test
eric: review+, commit-queue: commit-queue-
Archive of layout-test-results from eseidel-cq-sf
none
Patch proposal + Layout test
eric: review-
Patch proposal + Layout test
none
converted the test to a script test
none
fixes the bug
none
Added blockDirectionPointInLine eric: review+

Mario Sanchez Prada
Reported 2011-03-01 10:30:53 PST
In WebCore/editing/visible_units.cpp, in nextLinePosition() and previousLinePosition() functions there are the following lines at some point: renderer->positionForPoint(IntPoint(x - absPos.x(), root->lineTop())); ...the problem with this line is that it doesn't take into consideration that objects RootInlineBoxes could have a non-zero padding and/or border so you could be sometimes looking for the wrong point when trying to target the line above or below the current object. Instead, root->selectionTop() should be used, which does take those "extra" values into account. See https://bugs.webkit.org/show_bug.cgi?id=25533#c10 for an example of problems happening because of this: when having links rendered with 1px width border at the bottom (border-bottom: 1px), and having the caret positioned inside that element, pressing down to move the caret to the line below will result on keeping the caret in the wrong place, since the lineTop() won't take into account that border at the bottom and will still point to the current line, not the one below. I've just seen this in the GTK port with caret browsing enabled, so I'm not sure whether this is reproducible in other ports, but I swear the problem is there. I'm marking this bug as blocking bug 25533 since it directly impacts its resolution (as it's a more general bug in the way that it tries to fix "wrong vertical navigation"). Attaching a patch + layout test write after committing this report.
Attachments
Patch proposal + Layout test (11.92 KB, patch)
2011-03-01 10:48 PST, Mario Sanchez Prada 		eric: review+
commit-queue: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from eseidel-cq-sf (205.17 KB, application/zip)
2011-04-26 22:13 PDT, WebKit Commit Bot 		no flags
Details
Patch proposal + Layout test (15.61 KB, patch)
2011-05-02 03:12 PDT, Mario Sanchez Prada 		eric: review-
DetailsFormatted DiffDiff
Patch proposal + Layout test (14.15 KB, patch)
2011-05-31 08:22 PDT, Mario Sanchez Prada 		no flags
DetailsFormatted DiffDiff
converted the test to a script test (8.06 KB, patch)
2011-06-01 20:45 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
fixes the bug (7.04 KB, patch)
2011-06-14 15:20 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Added blockDirectionPointInLine (7.72 KB, patch)
2011-06-15 13:42 PDT, Ryosuke Niwa 		eric: review+
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Mario Sanchez Prada
Comment 1 2011-03-01 10:48:39 PST
Created attachment 84250 [details] Patch proposal + Layout test Attaching patch to fix this issue
Martin Robinson
Comment 2 2011-03-01 10:52:03 PST
Comment on attachment 84250 [details] Patch proposal + Layout test This looks sane to me, but I'm not familiar enough with the editing code to do a review for this one. It might be better to try to devise a platform-independent layout test for this one too.
Mario Sanchez Prada
Comment 3 2011-03-01 10:55:59 PST
Adding Ryosuke Niwa to CC as suggested by Martin Robinson and Xan Lopez.
Ryosuke Niwa
Comment 4 2011-03-01 21:52:14 PST
(In reply to comment #3) > Adding Ryosuke Niwa to CC as suggested by Martin Robinson and Xan Lopez.
Ryosuke Niwa
Comment 5 2011-03-01 21:54:05 PST
Comment on attachment 84250 [details] Patch proposal + Layout test View in context: https://bugs.webkit.org/attachment.cgi?id=84250&action=review > Source/WebCore/editing/visible_units.cpp:571 > - return renderer->positionForPoint(IntPoint(x - absPos.x(), root->lineTop())); > + return renderer->positionForPoint(IntPoint(x - absPos.x(), root->selectionTop())); I can't review this patch as I don't know the difference between lineTop and selectionTop. I'm hoping that mitz, hyatt, smfr, or darin can review this change. Also cc enrica & leviw since they are more familiar with the rendering engine than I am.
Mario Sanchez Prada
Comment 6 2011-03-02 02:42:08 PST
(In reply to comment #5) > (From update of attachment 84250 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84250&action=review > > > Source/WebCore/editing/visible_units.cpp:571 > > - return renderer->positionForPoint(IntPoint(x - absPos.x(), root->lineTop())); > > + return renderer->positionForPoint(IntPoint(x - absPos.x(), root->selectionTop())); > > I can't review this patch as I don't know the difference between lineTop and > selectionTop. I'm hoping that mitz, hyatt, smfr, or darin can review this > change. Also cc enrica & leviw since they are more familiar with the > rendering engine than I am. As far as can extract from the implementation, and having in mind the CSS box model, lineTop() just gives you the Y coordinate (relative to the containing block) of the top boundary for the "net" content of the box, without considering anything else in the CSS box model like padding and border, and that impacts the accuracy of the calculations passed to positionForPoint() when the current box does indeed have non-zero padding or border. In the other hand selectionTop() does take into account padding and border, so the calculations passed to positionForPoint(), when trying to "hit" another box in the next/previous line, are correct. What is not so clear to me, is whether this is the right patch to fix the whole issue. Perhaps there are some extra calculations/considerations I should be taking into account here and that could be missing, of course. Just trying to explain a bit better the rationale behind the current patch
Mario Sanchez Prada
Comment 7 2011-03-07 09:56:56 PST
Not willing to put any pressure on this one, but it would be great if we could have it reviewed soon so we could ship a version of webkitgtk shipping this fix along with the release of GNOME 3.0 [1], since it's very important for caret navigation. Hope you understand. Thanks! [1] https://live.gnome.org/TwoPointNinetyone/#Schedule
Levi Weintraub
Comment 8 2011-03-29 10:28:02 PDT
This seems like the right fix to me.
Enrica Casucci
Comment 9 2011-03-31 10:38:56 PDT
(In reply to comment #7) > Not willing to put any pressure on this one, but it would be great if we could have it reviewed soon so we could ship a version of webkitgtk shipping this fix along with the release of GNOME 3.0 [1], since it's very important for caret navigation. > > Hope you understand. Thanks! > > [1] https://live.gnome.org/TwoPointNinetyone/#Schedule Your change looks fine to me, but I would really like mitz or smfr to look at this, since they are more familiar with the rendering code than I am.
Mario Sanchez Prada
Comment 10 2011-04-12 01:48:37 PDT
(In reply to comment #9) > (In reply to comment #7) > > Not willing to put any pressure on this one, but it would be great if we > > could have it reviewed soon so we could ship a version of webkitgtk shipping > > this fix along with the release of GNOME 3.0 [1], since it's very important > > for caret navigation. > > > > Hope you understand. Thanks! > > > > [1] https://live.gnome.org/TwoPointNinetyone/#Schedule > > Your change looks fine to me, but I would really like mitz or smfr to look at > this, since they are more familiar with the rendering code than I am. It's been almost two weeks since the last comment and, even though we're already out of time for the GNOME 3.0 (since it's finally out [1], yay!), it would be great if we could get this integrated at some point, since it's a problem affecting cursor navigation under some scenarios, and not only in the GTK port, as far as I know... Having said that, I must say there's no big urgency either, this is just yet another ping, specially thinking of Dan and Simon, as per Enrica's comments Thanks [1] http://www.gnome.org
Eric Seidel (no email)
Comment 11 2011-04-26 16:27:40 PDT
Comment on attachment 84250 [details] Patch proposal + Layout test int lineTop() const { return m_lineTop; } int RootInlineBox::selectionTop() const { int selectionTop = m_lineTop; if (m_hasAnnotationsBefore) selectionTop -= !renderer()->style()->isFlippedLinesWritingMode() ? computeOverAnnotationAdjustment(m_lineTop) : computeUnderAnnotationAdjustment(m_lineTop); if (renderer()->style()->isFlippedLinesWritingMode()) return selectionTop; int prevBottom = prevRootBox() ? prevRootBox()->selectionBottom() : block()->borderBefore() + block()->paddingBefore(); if (prevBottom < selectionTop && block()->containsFloats()) { // This line has actually been moved further down, probably from a large line-height, but possibly because the // line was forced to clear floats. If so, let's check the offsets, and only be willing to use the previous // line's bottom if the offsets are greater on both sides. int prevLeft = block()->logicalLeftOffsetForLine(prevBottom, false); int prevRight = block()->logicalRightOffsetForLine(prevBottom, false); int newLeft = block()->logicalLeftOffsetForLine(selectionTop, false); int newRight = block()->logicalRightOffsetForLine(selectionTop, false); if (prevLeft > newLeft || prevRight < newRight) return selectionTop; } return prevBottom; }
Eric Seidel (no email)
Comment 12 2011-04-26 16:28:23 PDT
Comment on attachment 84250 [details] Patch proposal + Layout test This change seems reasonable to me.
WebKit Commit Bot
Comment 13 2011-04-26 22:13:47 PDT
Comment on attachment 84250 [details] Patch proposal + Layout test Rejecting attachment 84250 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2 Last 500 characters of output: ests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ............ 766.54s total testing time 23390 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 16 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8511270
WebKit Commit Bot
Comment 14 2011-04-26 22:13:49 PDT
Created attachment 91231 [details] Archive of layout-test-results from eseidel-cq-sf The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: eseidel-cq-sf Port: Mac Platform: Mac OS X 10.6.6
Mario Sanchez Prada
Comment 15 2011-05-02 03:12:04 PDT
Created attachment 91904 [details] Patch proposal + Layout test (In reply to comment #14) > Created an attachment (id=91231) [details] > Archive of layout-test-results from eseidel-cq-sf > > The attached test failures were seen while running run-webkit-tests on the commit-queue. > Bot: eseidel-cq-sf Port: Mac Platform: Mac OS X 10.6.6 Sorry, forgot to skip platforms not supporting caret browsing mode.
Mario Sanchez Prada
Comment 16 2011-05-31 07:32:11 PDT
(In reply to comment #15) > Created an attachment (id=91904) [details] > Patch proposal + Layout test > > (In reply to comment #14) > > Created an attachment (id=91231) [details] [details] > > Archive of layout-test-results from eseidel-cq-sf > > > > The attached test failures were seen while running run-webkit-tests on the commit-queue. > > Bot: eseidel-cq-sf Port: Mac Platform: Mac OS X 10.6.6 > > Sorry, forgot to skip platforms not supporting caret browsing mode. A month has passed since this comment so I guess it's safe pinging again... :-) Eric, would you mind taking a look to it? It's basically the same patch than before, but skipping layout tests in the platforms where it won't obviously work. Thanks!
Eric Seidel (no email)
Comment 17 2011-05-31 08:00:26 PDT
Comment on attachment 91904 [details] Patch proposal + Layout test Your ChangeLog looks confused. You have mutiple nested entries. There should only be one entry.
Eric Seidel (no email)
Comment 18 2011-05-31 08:01:45 PDT
Comment on attachment 91904 [details] Patch proposal + Layout test I assume this is only visible when using caret browsing mode? Your patch no longer applies so I can't see the context, to see if these functions are caret-browsing-only.
Mario Sanchez Prada
Comment 19 2011-05-31 08:22:58 PDT
Created attachment 95435 [details] Patch proposal + Layout test (In reply to comment #17) > (From update of attachment 91904 [details]) > Your ChangeLog looks confused. You have mutiple nested entries. > There should only be one entry. Oops... sorry, it's fixed now. (In reply to comment #18) > (From update of attachment 91904 [details]) > I assume this is only visible when using caret browsing mode? Your patch no > longer applies so I can't see the context, to see if these functions are > caret-browsing-only. The patch applies now.
Eric Seidel (no email)
Comment 20 2011-05-31 08:29:37 PDT
Comment on attachment 95435 [details] Patch proposal + Layout test View in context: https://bugs.webkit.org/attachment.cgi?id=95435&action=review > Source/WebCore/editing/visible_units.cpp:576 > + return renderer->positionForPoint(IntPoint(x - absPos.x(), root->selectionTop())); This line was last touched by hyatt, reviewed by mitz, as part of bug 20329 where it was changed from topOverflow() to lineTop().
Eric Seidel (no email)
Comment 21 2011-05-31 08:29:59 PDT
(I suspect that was just a rename.)
Mario Sanchez Prada
Comment 22 2011-05-31 08:50:26 PDT
(In reply to comment #20) > (From update of attachment 95435 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95435&action=review > > > Source/WebCore/editing/visible_units.cpp:576 > > + return renderer->positionForPoint(IntPoint(x - absPos.x(), root->selectionTop())); > > This line was last touched by hyatt, reviewed by mitz, as part of bug 20329 > where it was changed from topOverflow() to lineTop(). I'm not sure that is a related issue. The current bug is about being able to "find" the next/previous line for a given VisiblePosition and the fact that the padding and border is currently ignored is causing the trouble, so that's why I think selectionTop/Bottom() are the right way to go. But of course, can't swear on my life :-) It could be good then, I guess, if mitz and/or hyatt (both already in CC) took a look to it before actually committing the changeset to the repo.
Ryosuke Niwa
Comment 23 2011-06-01 20:22:27 PDT
Comment on attachment 95435 [details] Patch proposal + Layout test View in context: https://bugs.webkit.org/attachment.cgi?id=95435&action=review > LayoutTests/editing/selection/caret-mode-vertical-navigation-with-links.html:37 > + shouldBe("getSelection().baseOffset", "10"); > + shouldBe("getSelection().anchorNode.nodeValue", "'A long paragraph with some '"); > + eventSender.keyDown("downArrow"); > + shouldBe("getSelection().baseOffset", "3"); > + shouldBe("getSelection().anchorNode.nodeValue", "'them'"); This test doesn't work because depending on font metrics of a port/platform, the text is wrapped differently.
Ryosuke Niwa
Comment 24 2011-06-01 20:45:15 PDT
Created attachment 95712 [details] converted the test to a script test
Ryosuke Niwa
Comment 25 2011-06-01 20:45:43 PDT
Comment on attachment 95712 [details] converted the test to a script test Wrong bug.
Ryosuke Niwa
Comment 26 2011-06-01 20:46:14 PDT
Comment on attachment 95435 [details] Patch proposal + Layout test Restore my r-.
Ryosuke Niwa
Comment 27 2011-06-14 15:20:07 PDT
Created attachment 97171 [details] fixes the bug
Mario Sanchez Prada
Comment 28 2011-06-15 01:07:33 PDT
(In reply to comment #27) > Created an attachment (id=97171) [details] > fixes the bug It definitely looks better than my previous patch. Thanks for taking care of this, Ryosuke!
Eric Seidel (no email)
Comment 29 2011-06-15 08:42:42 PDT
Comment on attachment 97171 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=97171&action=review > Source/WebCore/editing/visible_units.cpp:576 > + return renderer->positionForPoint(IntPoint(x - absPos.x(), max(root->lineTop(), root->selectionTop()))); Should RootLineBox have a function which returns this max() value? I'm not sure what it would be called. But it seems like a good candidate for a nicely named function.
Ryosuke Niwa
Comment 30 2011-06-15 10:50:09 PDT
(In reply to comment #29) > (From update of attachment 97171 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97171&action=review > > > Source/WebCore/editing/visible_units.cpp:576 > > + return renderer->positionForPoint(IntPoint(x - absPos.x(), max(root->lineTop(), root->selectionTop()))); > > Should RootLineBox have a function which returns this max() value? I'm not sure what it would be called. But it seems like a good candidate for a nicely named function. How about blockDirectionPointInLine ? (we shouldn't call it y, top, etc... because those concepts break down in vertical writing modes).
Ryosuke Niwa
Comment 31 2011-06-15 13:42:06 PDT
Created attachment 97358 [details] Added blockDirectionPointInLine
Eric Seidel (no email)
Comment 32 2011-06-16 11:00:10 PDT
Comment on attachment 97358 [details] Added blockDirectionPointInLine The patch looks fine. But the name blockDirectionPointInLine is meaningless to me.
Ryosuke Niwa
Comment 33 2011-06-16 12:11:38 PDT
Committed r89056: <http://trac.webkit.org/changeset/89056>
Note You need to log in before you can comment on or make changes to this bug.