Bug 45375 - [Regression][Gtk] Left and Right Arrows no longer function when caret browsing is enabled
Summary: [Regression][Gtk] Left and Right Arrows no longer function when caret browsin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2010-09-08 02:10 PDT by Joanmarie Diggs
Modified: 2010-09-24 08:00 PDT (History)
6 users (show)

See Also:


Attachments
Patch to fix this issue (8.39 KB, patch)
2010-09-14 04:57 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch to fix this issue + Layout test (13.55 KB, patch)
2010-09-21 09:51 PDT, Mario Sanchez Prada
cfleizach: review-
Details | Formatted Diff | Diff
Patch to fix this issue + Layout test (21.97 KB, patch)
2010-09-22 02:27 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs 2010-09-08 02:10:35 PDT
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.
Comment 1 Mario Sanchez Prada 2010-09-14 04:57:33 PDT
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)
Comment 2 Joanmarie Diggs 2010-09-14 06:10:34 PDT
(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. :-(
Comment 3 Xan Lopez 2010-09-20 02:52:33 PDT
This used to work. It broke because we don't have any layout tests for this stuff. Perhaps it's time to do that?
Comment 4 Mario Sanchez Prada 2010-09-21 09:51:21 PDT
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! :-)
Comment 5 chris fleizach 2010-09-21 14:37:12 PDT
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
Comment 6 Xan Lopez 2010-09-21 22:14:33 PDT
(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).
Comment 7 Xan Lopez 2010-09-21 22:15:43 PDT
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?
Comment 8 Mario Sanchez Prada 2010-09-22 02:27:24 PDT
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.
Comment 9 chris fleizach 2010-09-22 10:00:41 PDT
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
Comment 10 Mario Sanchez Prada 2010-09-23 01:38:44 PDT
(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.
Comment 11 Mario Sanchez Prada 2010-09-23 01:41:22 PDT
Adding Chris Fleizach to CC
Comment 12 chris fleizach 2010-09-23 08:48:41 PDT
Comment on attachment 68353 [details]
Patch to fix this issue + Layout test

rme
Comment 13 WebKit Commit Bot 2010-09-23 09:21:06 PDT
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>
Comment 14 WebKit Commit Bot 2010-09-23 09:21:12 PDT
All reviewed patches have been landed.  Closing bug.