Bug 25676 – Problems navigating by caret in links whose text wraps onto subsequent lines

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

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".

[Joanmarie Diggs]

Created attachment 30167 [details]
aforementioned test case

[Alejandro G. Castro]

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.

[Alejandro G. Castro]

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.

[Alejandro G. Castro]

I forgot to mention you can check bug 31920 for more information about the regression.

[Alejandro G. Castro]

New dependency for this bug is the bug 32612.

[Alejandro G. Castro]

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.

[Alejandro G. Castro]

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.

[Alejandro G. Castro]

Now the patch with the new property have landed I've set r? and added darin to the bug so he can check it.

[Joanmarie Diggs]

Just tried the patch. Awesome. Thanks so much for doing this Alex!

[Xan Lopez]

This is not really GTK specific at all, changing the bug title.

[Joanmarie Diggs]

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!

[Adele Peterson]

CC'ing a few more people who might feel comfortable reviewing.

[Darin Adler]

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.

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.

[Darin Adler]

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.

[Alejandro G. Castro]

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+ :)

[Enrica Casucci]

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 @@
> [phone]  Alejandro G. Castro  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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 @@
> [phone]  Alejandro G. Castro  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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.

[Darin Adler]

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.

[Xan Lopez]

This was committed in r54566, closing.

[Antonio Gomes]

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.