Summary: | Consider padding and border when looking for the next/previous line position | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mario Sanchez Prada <mario> | ||||||||||||||||
Component: | HTML Editing | Assignee: | 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
Mario Sanchez Prada
2011-03-01 10:30:53 PST
Created attachment 84250 [details]
Patch proposal + Layout test
Attaching patch to fix this issue
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.
Adding Ryosuke Niwa to CC as suggested by Martin Robinson and Xan Lopez. (In reply to comment #3) > Adding Ryosuke Niwa to CC as suggested by Martin Robinson and Xan Lopez. 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. (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 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 This seems like the right fix to me. (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. (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 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;
}
Comment on attachment 84250 [details]
Patch proposal + Layout test
This change seems reasonable to me.
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 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
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. (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! Comment on attachment 91904 [details]
Patch proposal + Layout test
Your ChangeLog looks confused. You have mutiple nested entries. There should only be one entry.
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.
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. 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(). (I suspect that was just a rename.) (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. 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. Created attachment 95712 [details]
converted the test to a script test
Comment on attachment 95712 [details]
converted the test to a script test
Wrong bug.
Comment on attachment 95435 [details]
Patch proposal + Layout test
Restore my r-.
Created attachment 97171 [details]
fixes the bug
(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! 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. (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). Created attachment 97358 [details]
Added blockDirectionPointInLine
Comment on attachment 97358 [details]
Added blockDirectionPointInLine
The patch looks fine. But the name blockDirectionPointInLine is meaningless to me.
Committed r89056: <http://trac.webkit.org/changeset/89056> |