55481 – Consider padding and border when looking for the next/previous line position

RESOLVED FIXED 55481

Consider padding and border when looking for the next/previous line position

https://bugs.webkit.org/show_bug.cgi?id=55481

Summary Consider padding and border when looking for the next/previous line position

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-	Details Formatted Diff Diff
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-	Details Formatted Diff Diff
Patch proposal + Layout test (14.15 KB, patch) 2011-05-31 08:22 PDT, Mario Sanchez Prada	no flags	Details Formatted Diff Diff
converted the test to a script test (8.06 KB, patch) 2011-06-01 20:45 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
fixes the bug (7.04 KB, patch) 2011-06-14 15:20 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Added blockDirectionPointInLine (7.72 KB, patch) 2011-06-15 13:42 PDT, Ryosuke Niwa	eric: review+	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware PC

OS Linux

Product WebKit

Component HTML Editing

Assignee

Ryosuke Niwa

Reported

2011-03-01 10:30 PST

Modified

2011-06-16 12:11 PDT History

CC List

11 users Show

URL

Keywords

Depends on

Blocks

25533 59435 62743

Dependencies

tree graph