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
Product: WebKit
Classification: Unclassified
Component: Accessibility
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: Nobody
: Gtk
Depends on: 31920
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-05-10 14:14 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2010-12-06 22:04 PST (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2009-05-10 14:14:14 PDT
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 Joanmarie Diggs (irc: joanie) 2009-05-10 14:15:36 PDT
Created attachment 30167 [details]
aforementioned test case
Comment 2 Alejandro G. Castro 2009-10-12 12:13:15 PDT
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 Alejandro G. Castro 2009-12-16 05:57:22 PST
Created attachment 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 Alejandro G. Castro 2009-12-16 05:58:33 PST
I forgot to mention you can check bug 31920 for more information about the regression.
Comment 5 Alejandro G. Castro 2009-12-16 10:45:40 PST
New dependency for this bug is the bug 32612.
Comment 6 Alejandro G. Castro 2009-12-16 11:56:22 PST
Created attachment 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 Alejandro G. Castro 2009-12-17 03:13:43 PST
Created attachment 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 Alejandro G. Castro 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 Joanmarie Diggs (irc: joanie) 2009-12-20 10:03:16 PST
Just tried the patch. Awesome. Thanks so much for doing this Alex!
Comment 10 Xan Lopez 2010-01-26 23:18:51 PST
This is not really GTK specific at all, changing the bug title.
Comment 11 Joanmarie Diggs (irc: joanie) 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 Adele Peterson 2010-02-01 21:54:09 PST
CC'ing a few more people who might feel comfortable reviewing.
Comment 13 Darin Adler 2010-02-02 08:18:09 PST
Comment on attachment 45052 [details]
Proposed patch

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 mitz@webkit.org 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 Darin Adler 2010-02-02 09:50:50 PST
Comment on attachment 45052 [details]
Proposed patch

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 Alejandro G. Castro 2010-02-09 03:30:38 PST
Created attachment 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 Enrica Casucci 2010-02-09 08:55:38 PST
Comment on attachment 48400 [details]
Proposed patch

> 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 Darin Adler 2010-02-09 10:43:07 PST
Comment on attachment 48400 [details]
Proposed patch

r=me

A more thorough test case would be good to have, but this one is fine for this bug fix.
Comment 19 Xan Lopez 2010-02-09 13:10:56 PST
This was committed in r54566, closing.
Comment 20 Antonio Gomes 2010-12-06 22:04:10 PST
Comment on attachment 48400 [details]
Proposed patch

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.