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+

Description Mario Sanchez Prada 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.
Comment 1 Mario Sanchez Prada 2011-03-01 10:48:39 PST
Created attachment 84250 [details]
Patch proposal + Layout test

Attaching patch to fix this issue
Comment 2 Martin Robinson 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.
Comment 3 Mario Sanchez Prada 2011-03-01 10:55:59 PST
Adding Ryosuke Niwa to CC as suggested by Martin Robinson and Xan Lopez.
Comment 4 Ryosuke Niwa 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.
Comment 5 Ryosuke Niwa 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.
Comment 6 Mario Sanchez Prada 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
Comment 7 Mario Sanchez Prada 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
Comment 8 Levi Weintraub 2011-03-29 10:28:02 PDT
This seems like the right fix to me.
Comment 9 Enrica Casucci 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.
Comment 10 Mario Sanchez Prada 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
Comment 11 Eric Seidel (no email) 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;
}
Comment 12 Eric Seidel (no email) 2011-04-26 16:28:23 PDT
Comment on attachment 84250 [details]
Patch proposal + Layout test

This change seems reasonable to me.
Comment 13 WebKit Commit Bot 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
Comment 14 WebKit Commit Bot 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
Comment 15 Mario Sanchez Prada 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.
Comment 16 Mario Sanchez Prada 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!
Comment 17 Eric Seidel (no email) 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.
Comment 18 Eric Seidel (no email) 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.
Comment 19 Mario Sanchez Prada 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.
Comment 20 Eric Seidel (no email) 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().
Comment 21 Eric Seidel (no email) 2011-05-31 08:29:59 PDT
(I suspect that was just a rename.)
Comment 22 Mario Sanchez Prada 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.
Comment 23 Ryosuke Niwa 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.
Comment 24 Ryosuke Niwa 2011-06-01 20:45:15 PDT
Created attachment 95712 [details]
converted the test to a script test
Comment 25 Ryosuke Niwa 2011-06-01 20:45:43 PDT
Comment on attachment 95712 [details]
converted the test to a script test

Wrong bug.
Comment 26 Ryosuke Niwa 2011-06-01 20:46:14 PDT
Comment on attachment 95435 [details]
Patch proposal + Layout test

Restore my r-.
Comment 27 Ryosuke Niwa 2011-06-14 15:20:07 PDT
Created attachment 97171 [details]
fixes the bug
Comment 28 Mario Sanchez Prada 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!
Comment 29 Eric Seidel (no email) 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.
Comment 30 Ryosuke Niwa 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).
Comment 31 Ryosuke Niwa 2011-06-15 13:42:06 PDT
Created attachment 97358 [details]
Added blockDirectionPointInLine
Comment 32 Eric Seidel (no email) 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.
Comment 33 Ryosuke Niwa 2011-06-16 12:11:38 PDT
Committed r89056: <http://trac.webkit.org/changeset/89056>