Bug 25676 - Problems navigating by caret in links whose text wraps onto subsequent lines
: Problems navigating by caret in links whose text wraps onto subsequent lines
Status: RESOLVED FIXED
: WebKit
Accessibility
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
: Gtk
: 31920
: 25531
  Show dependency treegraph
 
Reported: 2009-05-10 14:14 PST by
Modified: 2010-12-06 22:04 PST (History)


Attachments
aforementioned test case (295 bytes, text/html)
2009-05-10 14:15 PST, Joanmarie Diggs (irc: joanie)
no flags Details
Proposed patch (603 bytes, patch)
2009-12-16 05:57 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (3.39 KB, patch)
2009-12-16 11:56 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch (3.47 KB, patch)
2009-12-17 03:13 PST, Alejandro G. Castro
darin: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch (3.41 KB, patch)
2010-02-09 03:30 PST, Alejandro G. Castro
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-05-10 14:14:14 PST
Steps to reproduce:

1. Open the (to be) attached test case.

2. Resize the window so that the first line ends with the word "span".

3. Position the caret to the left of the first link and use Right Arrow to move through the text of the first paragraph.

Expected results: Right Arrow would continue to move the caret one character at a time.

Actual results: Right Arrow works as expected until the end of the first line is reached. Pressing Right Arrow from the end of the line causes the caret to jump to the end of the link.

4. Having arrowed past the first link, use Left Arrow to move the caret one character at a time in the opposite direction.

Expected results: Left Arrow would continue to move the caret one character at a time.

Actual results: Left Arrow works as expected until the beginning of the second line is reached. Pressing Left Arrow from the beginning of the second line causes the caret to jump to the end of the link.

Note: If you perform the above steps on the link in the second paragraph instead, things work as expected. The difference is that the second paragraph has a line break inserted at the end of "span".
------- Comment #1 From 2009-05-10 14:15:36 PST -------
Created an attachment (id=30167) [details]
aforementioned test case
------- Comment #2 From 2009-10-12 12:13:15 PST -------
Apparently the patch in bug 13736 is causing the problem, I'll comment in that bug and try to check how to fix the problem completely.
------- Comment #3 From 2009-12-16 05:57:22 PST -------
Created an attachment (id=44967) [details]
Proposed patch

This patch fix the problem, it is another regression caused by the search ahead patch. Now I have to check how to add a test to the patch before asking for review.
------- Comment #4 From 2009-12-16 05:58:33 PST -------
I forgot to mention you can check bug 31920 for more information about the regression.
------- Comment #5 From 2009-12-16 10:45:40 PST -------
New dependency for this bug is the bug 32612.
------- Comment #6 From 2009-12-16 11:56:22 PST -------
Created an attachment (id=45000) [details]
Proposed patch

This patch requires the patch in the bug 32612 in order to run the test correctly. I'll not ask for review until we have that patch landed.
------- Comment #7 From 2009-12-17 03:13:43 PST -------
Created an attachment (id=45052) [details]
Proposed patch

I've decided to remove the !box condition because actually it is apparently the wrong way to check if the candidate is the last one. Not review until we have the patch of the bug 32612 in the repository.
------- Comment #8 From 2009-12-18 01:41:00 PST -------
Now the patch with the new property have landed I've set r? and added darin to the bug so he can check it.
------- Comment #9 From 2009-12-20 10:03:16 PST -------
Just tried the patch. Awesome. Thanks so much for doing this Alex!
------- Comment #10 From 2010-01-26 23:18:51 PST -------
This is not really GTK specific at all, changing the bug title.
------- Comment #11 From 2010-01-31 16:02:54 PST -------
It would be awesome if Alex's patch could be reviewed and committed. It's really important for users who require the ability to navigate content via the keyboard. Thanks!
------- Comment #12 From 2010-02-01 21:54:09 PST -------
CC'ing a few more people who might feel comfortable reviewing.
------- Comment #13 From 2010-02-02 08:18:09 PST -------
(From update of attachment 45052 [details])
This change has no effect outside caret navigation? The function being changed is not just used in caret navigation.

If the change does have other effects, then it should be straightforward to add a cross-platform test case showing that effect.
------- Comment #14 From 2010-02-02 08:29:29 PST -------
I was able to test on Mac OS X by adding "style='-webkit-user-modify: read-write'" to the body element. Note that just setting the contenteditable attribute will not work, because it changes the line breaking style.
------- Comment #15 From 2010-02-02 09:50:50 PST -------
(From update of attachment 45052 [details])
review- because this all-platforms fix needs an all-platforms test. It's fine if you want to keep the GTK-specific accessibility-based test too, but the identical test can be done based on Mitz’s suggestion in a way that will work on all platforms.
------- Comment #16 From 2010-02-09 03:30:38 PST -------
Created an attachment (id=48400) [details]
Proposed patch

I've moved the test and added the style property, it fails before the applying the patch and works after it. I've removed the a11y specific test because basically both were checking the same point.

Thanks for the comments.

Btw, now that I have the committer account I do not need the cq+ :)
------- Comment #17 From 2010-02-09 08:55:38 PST -------
(From update of attachment 48400 [details])
> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 2d13e20..d9ea28b 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,16 @@
> +2010-02-09  Alejandro G. Castro  <alex@igalia.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Problems navigating by caret in links whose text wraps onto
> +        subsequent lines.
> +        https://bugs.webkit.org/show_bug.cgi?id=25676
> +
> +        Added tests showing the problem.
> +
> +        * fast/events/multiline-link-arrow-navigation.html
> +        * fast/events/multiline-link-arrow-navigation-expected.txt
> +
>  2010-02-09  Zoltan Herczeg  <zherczeg@inf.u-szeged.hu>
>  
>          Reviewed by Oliver Hunt.
> diff --git a/LayoutTests/fast/events/multiline-link-arrow-navigation-expected.txt b/LayoutTests/fast/events/multiline-link-arrow-navigation-expected.txt
> new file mode 100644
> index 0000000..6681480
> --- /dev/null
> +++ b/LayoutTests/fast/events/multiline-link-arrow-navigation-expected.txt
> @@ -0,0 +1,3 @@
> +This is a test of links which span multiple lines for various and sundry reasons.
> +
> +PASS
> diff --git a/LayoutTests/fast/events/multiline-link-arrow-navigation.html b/LayoutTests/fast/events/multiline-link-arrow-navigation.html
> new file mode 100644
> index 0000000..83d8441
> --- /dev/null
> +++ b/LayoutTests/fast/events/multiline-link-arrow-navigation.html
> @@ -0,0 +1,26 @@
> +<html><head>
> +<meta http-equiv="content-type" content="text/html; charset=UTF-8">
> +
> +<script>
> +if (window.layoutTestController) {
> +    layoutTestController.overridePreference("WebKitEnableCaretBrowsing", true);
> +    layoutTestController.dumpAsText();
> +}
> +
> +function runTest()
> +{
> +    var para = document.getElementById("para");
> +    window.getSelection().setPosition(para.childNodes.item(1).childNodes.item(0), 15);
> +    eventSender.keyDown("rightArrow");
> +    eventSender.keyDown("rightArrow");
> +
> +    document.getElementById("result").innerText = getSelection().baseOffset == 17 ? "PASS" : "FAIL";
> +}
> +</script>
> +
> +<title>Test</title>
> +</head><body onLoad="runTest();">
> +<p id="para" style="-webkit-user-modify: read-write; width: 250px; height: 100px; border: 1px solid blue;">This is a test of <a href="https://bug-25676-attachments.webkit.org/foo.html">links which span multiple lines</a> for various and sundry reasons.</p>
> +
> +<p id="result"></p>
> +</body></html>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 9fdf2cf..21487a7 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,15 @@
> +2010-02-09  Alejandro G. Castro  <alex@igalia.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Problems navigating by caret in links whose text wraps onto
> +        subsequent lines.
> +        https://bugs.webkit.org/show_bug.cgi?id=25676
> +
> +        We should not search ahead if we are not in the last element.
> +
> +        * dom/Position.cpp:
> +
>  2010-02-08  Dominic Cooney  <dominicc@google.com>
>  
>          Reviewed by Adam Barth.
> diff --git a/WebCore/dom/Position.cpp b/WebCore/dom/Position.cpp
> index 0126835..c0f6fa3 100644
> --- a/WebCore/dom/Position.cpp
> +++ b/WebCore/dom/Position.cpp
> @@ -1046,7 +1046,7 @@ void Position::getInlineBoxAndOffset(EAffinity affinity, TextDirection primaryDi
>  
>              candidate = box;
>          }
> -        if (candidate && !box && affinity == DOWNSTREAM) {
> +        if (candidate && candidate == textRenderer->lastTextBox() && affinity == DOWNSTREAM) {
>              box = searchAheadForBetterMatch(textRenderer);
>              if (box)
>                  caretOffset = box->caretMinOffset();

This looks good to me. I'm assuming you've run all the layout tests and found no errors.
------- Comment #18 From 2010-02-09 10:43:07 PST -------
(From update of attachment 48400 [details])
r=me

A more thorough test case would be good to have, but this one is fine for this bug fix.
------- Comment #19 From 2010-02-09 13:10:56 PST -------
This was committed in r54566, closing.
------- Comment #20 From 2010-12-06 22:04:10 PST -------
(From update of attachment 48400 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=48400&action=review

> LayoutTests/fast/events/multiline-link-arrow-navigation.html:6
> +    layoutTestController.overridePreference("WebKitEnableCaretBrowsing", true);

It turns out enabling caret browsing is not need at all here, since you are using -webkit-user-modify.

1) Mac and Win does not support toggling caret browsing on/off in DRT.
2) I tested both Qt, Gtk (ports that support enabling caret browsing) and both also worked without enabling it.
3) I assume chromium port, which also support enabling caret browsing in DRT, should also work.
4) In Qt and Gtk, if I keep enabling caret browsing and removed the -webkit-user-modify style, it also worked.

I propose we use the style approach only here instead of caret browsing, since it makes the test testable on all DRTs.