Bug 45375

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)

(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. :-(

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

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

(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 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?

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

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

Adding Chris Fleizach to CC

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

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>

All reviewed patches have been landed. Closing bug.