Bug 45375 – [Regression][Gtk] Left and Right Arrows no longer function when caret browsing is enabled

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

[Joanmarie Diggs]

Steps to reproduce:

1. Launch Epiphany and view a page with a lot of text
2. Enable caret browsing
3. Arrow around

Expected results: Left, Right, Up, and Down would move the caret.

Actual results: Up and Down move the caret; Left and Right do not.

[Mario Sanchez Prada]

Created attachment 67538 [details]
Patch to fix this issue

Attaching patch that would fix this issue.

Basically it seems the problem was in how the Move[Word]{Forward|Backward} commands were acting when the WebCore::Editor::Command::isEnabled()  function was executed, that is, not taking into account whether caret browsing mode was enabled or not, as it was done for the Move[Word]{Left|Command}. So, using the enabledInEditableTextOrCaretBrowsing and enabledVisibleSelectionOrCaretBrowsing for those commands (instead of just enabledInEditableText and enabledVisibleSelection) would fix this issue.

Hence, now asking for review (and cq+ if approved) over this patch, which it's prioritary stuff for a11y support as it's blocking some things on Orca, I think (Joanmarie knows better about this)

[Joanmarie Diggs]

(In reply to comment #1)
> Attaching patch that would fix this issue.

Just gave it a spin. Works for me. Thanks!

> Hence, now asking for review (and cq+ if approved) over this patch, which it's prioritary stuff for a11y support as it's blocking some things on Orca, I think (Joanmarie knows better about this)

:-)

Basically, unlike with Gecko where Orca is forced to roll its own caret navigation due to Gecko brokenness, we're counting on WebKitGtk to do all of the "heavy lifting" navigation-wize. This makes Orca+WebKitGtk much more performant an option. It also means that without this fix, users cannot access text by character or by word; only by line. :-(

[Xan Lopez]

This used to work. It broke because we don't have any layout tests for this stuff. Perhaps it's time to do that?

[Mario Sanchez Prada]

Created attachment 68251 [details]
Patch to fix this issue + Layout test

(In reply to comment #3)
> This used to work. It broke because we don't have any layout tests for this stuff. Perhaps it's time to do that?

You command, master! :-)

[chris fleizach]

Comment on attachment 68251 [details]
Patch to fix this issue + Layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=68251&action=review

what about supporting more than word? i thought there was also line, page (maybe sentence) options available as well

> WebCore/ChangeLog:9
> +

is the patch considering doing something or doing something?
also end the sentence in a period

[Xan Lopez]

(In reply to comment #5)
> (From update of attachment 68251 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68251&action=review
> 
> what about supporting more than word? i thought there was also line, page (maybe sentence) options available as well

These are already supported, he's just fixing the ones that were missing the caret support for some reason (probably someone accidentally removed them when doing some refactoring).

[Xan Lopez]

Comment on attachment 68251 [details]
Patch to fix this issue + Layout test

>+  var target = document.getElementById("target");
>+  target.focus();

I don't think this is needed?

[Mario Sanchez Prada]

Created attachment 68353 [details]
Patch to fix this issue + Layout test

(In reply to comment #5)
> (From update of attachment 68251 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68251&action=review
> 
> what about supporting more than word? i thought there was also line, page (maybe sentence) options available as well

Good point. In this new patch I added support for MoveTo{Beginning|End}Of{Line|Sentence|Paragraph}, and as far as I could check, it was a nice addition. Thanks for pointing it out.

Also, extended the Layout test to check that new feature.

> > WebCore/ChangeLog:9
> > +
> 
> is the patch considering doing something or doing something?
> also end the sentence in a period

Done

On top of that, I've moved the tests from platform/gtk/accessibility to platform/gtk/editing/selection since those are not really a11y tests at all (just got confused because my work is usually a11y centered), and renamed them accordingly since they're no longer about left and right arrows, but also about home/end keys, for instance.

[chris fleizach]

Comment on attachment 68353 [details]
Patch to fix this issue + Layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=68353&action=review

> LayoutTests/platform/gtk/editing/selection/caret-mode-paragraph-keys-navigation.html:5
> +<script>

this pointer to js-test-style requires one ".."

the other two references require

"../../../../"

not sure if the js-test-pre stuff is getting loaded

> LayoutTests/platform/gtk/editing/selection/caret-mode-paragraph-keys-navigation.html:8
> +<script src="../../../../fast/js/resources/js-test-pre.js"></script>

as in here

> LayoutTests/platform/gtk/editing/selection/caret-mode-paragraph-keys-navigation.html:70
> +<script src="../../../../fast/js/resources/js-test-post.js"></script>

and here

[Mario Sanchez Prada]

(In reply to comment #9)
> (From update of attachment 68353 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68353&action=review
> 
> > LayoutTests/platform/gtk/editing/selection/caret-mode-paragraph-keys-navigation.html:5
> > +<script>
> 
> this pointer to js-test-style requires one ".."

I don't get this. It's precisely like that in the current patch:

   [...]
   <head>
   <link rel="stylesheet" href="../fast/js/resources/js-test-style.css">
   <script>
   [...]

> the other two references require
> 
> "../../../../"

Yep, and it's like that:

   [...]
   </script>
   <script src="../../../../fast/js/resources/js-test-pre.js"></script>
   </head>
   [...]
   </script>
   <script src="../../../../fast/js/resources/js-test-post.js"></script>
   </body>
   </html>
 
It was "../../.." when this test was inside platform/gtk/accessibility, but now it is under platform/gtk/editing/selection it's obvious we need an extra "../" :-)

> not sure if the js-test-pre stuff is getting loaded
>
> > LayoutTests/platform/gtk/editing/selection/caret-mode-paragraph-keys-navigation.html:8
> > +<script src="../../../../fast/js/resources/js-test-pre.js"></script>
> 
> as in here

Yes, it is being loaded. If it wasn't the description() and shouldBe() functions would be missing and whole text wouldn't work
 
> > LayoutTests/platform/gtk/editing/selection/caret-mode-paragraph-keys-navigation.html:70
> > +<script src="../../../../fast/js/resources/js-test-post.js"></script>
> 
> and here

Same for this.

[Mario Sanchez Prada]

Adding Chris Fleizach to CC

[chris fleizach]

Comment on attachment 68353 [details]
Patch to fix this issue + Layout test

rme

[WebKit Commit Bot]

Comment on attachment 68353 [details]
Patch to fix this issue + Layout test

Clearing flags on attachment: 68353

Committed r68149: <http://trac.webkit.org/changeset/68149>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.