Bug 25526 – [Gtk] Additional support is needed for caret browsing

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

[Joanmarie Diggs]

(This is a spin-off bug from Bug 16135 comment 25)

* For the caret navigation implemented in Bug 16135 there do not seem to be any caret-moved events emitted to inform assistive technologies of the movement taking place. As a result, ATs don't know that there's anything to present to the user.

* Need support for Home, End, Ctrl+Home, Ctrl+End

* Need (additional) support for selection via Shift + the above. Interestingly
enough, this seems to have been implemented once something is already selected.
(i.e. if you Shift + Right, Shift+Home, Shift+End, Shift+Ctrl+Home, and
Shift+Ctrl+End start working as expected. They just fail when nothing is
already selected.)

* In addition to the caret-moved events, I'm not seeing text-selection-changed
events when selecting/unselecting text.

* What are your thoughts/plans w.r.t. emitting focus: events when focus
changes? I am seeing the object:state-changed:focused events now (yea!) but not
focus: events. To be honest, I'm not sure how critical they are. I'm just used
to seeing them in other apps. I'll leave it to Will to comment on that.

[Xan Lopez]

(In reply to comment #0)
> (This is a spin-off bug from Bug 16135 comment 25)
> 

I'm going to start with this bug from the list of Orca blockers, yell at me if I should start with a different one.


> * What are your thoughts/plans w.r.t. emitting focus: events when focus
> changes? I am seeing the object:state-changed:focused events now (yea!) but not
> focus: events. To be honest, I'm not sure how critical they are. I'm just used
> to seeing them in other apps. I'll leave it to Will to comment on that.
> 

Hum, I'm emitting 'focus-event' and 'state-change:focused' from the same place, so I guess 'focus-event' is not what you have mind. Can you point me to the doc of the signal that is missing?

[Xan Lopez]

(In reply to comment #0)
> * Need (additional) support for selection via Shift + the above. Interestingly
> enough, this seems to have been implemented once something is already selected.
> (i.e. if you Shift + Right, Shift+Home, Shift+End, Shift+Ctrl+Home, and
> Shift+Ctrl+End start working as expected. They just fail when nothing is
> already selected.)
> 

BTW, I'm not sure I get what you mean here. Shitf+Home,End, etc won't work right, but the combinations with the direction keys should be working correctly I think. At least they work when nothing is selected here. Can you clarify this?

[Joanmarie Diggs]

(In reply to comment #2)
> BTW, I'm not sure I get what you mean here. Shitf+Home,End, etc won't work
> right, but the combinations with the direction keys should be working correctly
> I think. At least they work when nothing is selected here. Can you clarify
> this?

Sorry. Yes, the direction keys (Left, Right, Up, Down) work as expected (given normal text). Similarly, Shift + those four direction keys cause text to be selected, as expected. As for the rest:

1. With no text selected, try pressing Home, End, Control+Home, Control+End. Results: The caret does not move.

2. *With no text selected*, try Shift+Home, Shift+End, Shift+Control+Home, Shift+Control+End. Results: The caret does not move and no text is selected.

BUT

3. Use, say, Shift+Right Arrow to select the next character from within the middle of a line. Having done so, try Shift+Home, Shift+End, Shift+Control+Home, Shift+Control+End. Results: All four of these commands suddenly start working as expected. :-)

[Joanmarie Diggs]

(In reply to comment #1)
> I'm going to start with this bug from the list of Orca blockers, yell at me if
> I should start with a different one.

This is a good place to start, but thanks for asking. :-)

> Hum, I'm emitting 'focus-event' and 'state-change:focused' from the same place,

Indeed you are. I should've looked at your patch before I asked my question. My bad.

> so I guess 'focus-event' is not what you have mind.

I'm afraid I'm now stumped too. When I look at this doc

http://library.gnome.org/devel/atk/unstable/AtkObject.html#AtkObject-focus-event

it does seem that focus-event is exactly what I have in mind. The thing is, when I use Accerciser's event monitor and choose "focus" as the event of interest, Tabbing around links and form controls does not cause any events to be displayed by Accerciser. Yet, when I choose object->state-changed and Tab around, events are displayed.

So....

When you choose focus as the event type in Accerciser, are you seeing events emitted as expected? (In which case I need to figure out what's up on my box.) Otherwise, I wonder why what you've done is not causing the desired/expected results....

[Xan Lopez]

Created attachment 29992 [details]
caretrefactor.patch

Before adding the new features I'm refactoring the key handling code. I've now removed the special case for caret browsing in GTK+ and do everything in the crossplatform part of the code, which should be better for everyone.

[Xan Lopez]

CCing Justin so he can hopefully have a look at the patch.

[Xan Lopez]

(In reply to comment #4)
> > So....
> 
> When you choose focus as the event type in Accerciser, are you seeing events
> emitted as expected? (In which case I need to figure out what's up on my box.)
> Otherwise, I wonder why what you've done is not causing the desired/expected
> results....
> 

Yes, the same happens here. I guess there's something missing for this to work, we'll have to figure it out.

[Xan Lopez]

Created attachment 30015 [details]
homeendcaret.patch

This adds support for Home and End with their modifiers.

[Xan Lopez]

Created attachment 30022 [details]
textcaretmoved.patch

Emit AtkText::text-caret-moved when the selection changes.

[Xan Lopez]

Created attachment 30044 [details]
textcaretv2.patch

Rename our SelectionController to SelectionControllerGtk.cpp to avoid complaints by the linker, and only emit the signal if the wrapper object is actually an instance of AtkText.

[Xan Lopez]

Created attachment 30045 [details]
textselectionchanged.patch

Implement AtkText::text-selection-changed. I believe with this we now cover all the issues raised in the first comment.

[Gustavo Noronha (kov)]

Comment on attachment 30044 [details]
textcaretv2.patch

Looks correct. I was about to ask for text-selection-changed, but then I saw the second patch =).

[Gustavo Noronha (kov)]

Comment on attachment 30045 [details]
textselectionchanged.patch

And here goes text-selection-changed.

[Joanmarie Diggs]

> Created an attachment (id=29992) [review]
> caretrefactor.patch

From just a testing/usage standpoint, this seems to be working nicely.

> Created an attachment (id=30015) [review]
> homeendcaret.patch
> 
> This adds support for Home and End with their modifiers.

Ditto for this one. Awesome!!

[Joanmarie Diggs]

I just tested the new text-caret-moved and text-selection-changed events. Nice!

I'm seeing a couple of issues that I'll comment on here. If you want either filed as new bugs, please let me know.

1. When selecting text, text-caret-moved events continue to be issued for the offset at which the selection began rather than the current offset (i.e. the selection end).

2. When selecting text across object boundaries, the text-selection-changed events which are emitted are emitted for the object in which the selection started, which might not be the object containing the most recent selection. For instance, given:

    This <a href="foo.html">is</a> a test.

If I start selection somewhere in "This" and Shift+Right through to "test," I see a text-selection-changed event for each press of Shift+Right, but all of them are for the accessible which contains "This" rather than the link and subsequent text.

[Brent Fulgham]

It's not clear to me from the above what (if anything) should be landed.  Clearly textcaretv2.patch and textselectionchanged.patch are fine, but what about the caretrefactor.patch and homeendcaret.patch?  Is this only partially approved?

[Xan Lopez]

(In reply to comment #16)
> It's not clear to me from the above what (if anything) should be landed. 
> Clearly textcaretv2.patch and textselectionchanged.patch are fine, but what
> about the caretrefactor.patch and homeendcaret.patch?  Is this only partially
> approved?
> 

Well, someone has to review them. They improve the cross-platform support for caret browsing mode, so none of the GTK+ reviewers feel comfortable approving that kind of change. I've pinged several people about it several times without luck so far...

[Jan Alonzo]

(In reply to comment #17)
> (In reply to comment #16)
> > It's not clear to me from the above what (if anything) should be landed. 
> > Clearly textcaretv2.patch and textselectionchanged.patch are fine, but what
> > about the caretrefactor.patch and homeendcaret.patch?  Is this only partially
> > approved?
> > 
> 
> Well, someone has to review them. They improve the cross-platform support for
> caret browsing mode, so none of the GTK+ reviewers feel comfortable approving
> that kind of change. I've pinged several people about it several times without
> luck so far...

Xan, are these two patches needed to get textcaret and textselectionchanged patches to work? I'm thinking move these two cross-platform refactorings in a separate bug for now and retain/improve what we have in EditorClientGtk so we can move forward with this bug.

[Eric Seidel (no email)]

Comment on attachment 29992 [details]
caretrefactor.patch

executeMoveParagraphBackward and executeMoveParagraphForward implementations seem flipped.

What does Caret Browsing mean?

Seems this should be a static function:
bool caretBrowsing = frame->settings() && frame->settings()->caretBrowsingEnabled();

bool caretBrowsingEnabled(frame)

Seems this should be a nicely-named constant somewhere:
static_cast<EditorCommandSource>(0)

Or there should be a different version of enabledInEditableText which doesn't take a source.

How does one test this?  We need at least a manual test if not a LayoutTest.

[Xan Lopez]

(In reply to comment #19)
> (From update of attachment 29992 [details] [review])
> executeMoveParagraphBackward and executeMoveParagraphForward implementations
> seem flipped.

Yeah, silly me.

> 
> What does Caret Browsing mean?

It means the hability to browse web pages only using the keyboard, thanks to a special mode in the browser called, you guessed, caret browsing mode. It's of great importance for accessibility, and Gecko's implementation is specified here: http://www.mozilla.org/access/keyboard/proposal

> 
> Seems this should be a static function:
> bool caretBrowsing = frame->settings() &&
> frame->settings()->caretBrowsingEnabled();
> 
> bool caretBrowsingEnabled(frame)

Sure.

> 
> Seems this should be a nicely-named constant somewhere:
> static_cast<EditorCommandSource>(0)
> 
> Or there should be a different version of enabledInEditableText which doesn't
> take a source.

I think at least for this patch keeping the dummy constant would be better, we can further refactor it in follow-up patches.

> 
> How does one test this?  We need at least a manual test if not a LayoutTest.
> 

OK, I can provide at the very least a manual test.

[Xan Lopez]

Created attachment 31409 [details]
caretextra.patch

Fixes the points raised by Eric.

The manual test is really basic, because a) I'm not sure how to enable caret browsing from tests (in a crossplatform way) b) GTK+ port does not have eventSender right now, so I couldn't really test this anyway. I hope this will be enough while I figure out those problems.

[Eric Seidel (no email)]

Comment on attachment 31409 [details]
caretextra.patch

Since you're adding two new execCommands we need to test those.  Please add a LayoutTest which uses execCommand() to test these new commands.

Otherwise I think the patch looks fine.

[Eric Seidel (no email)]

Comment on attachment 30015 [details]
homeendcaret.patch

This part looks fine.  We need tests for this though.  You should add a way to turn on Caret browsing from DRT and then check that these edit commands are enabled...

Sicne you already have other patches on this bug blocked by lack of tests, I'll r+ this simple one and you can add more tests as part of those.

[Adam Barth]

We should either decide to land these or remove them from the queue.  Any guidance on which is best?

[Holger Freyther]

Comment on attachment 30045 [details]
textselectionchanged.patch

This patch got landed, clear review to make it drop out of the review queue.

[Xan Lopez]

Assign the bug to myself.

[Eric Seidel (no email)]

Comment on attachment 30015 [details]
homeendcaret.patch

Based on above comments this was already landed (unsure what revision?).  Clearing r+ flag.

[Eric Seidel (no email)]

If patches have already been landed from this bug, do we still need it open?  Ideally we do one change per bug (to not confuse the humans or tools).

[Xan Lopez]

(In reply to comment #28)
> If patches have already been landed from this bug, do we still need it open? 
> Ideally we do one change per bug (to not confuse the humans or tools).

There are still patches to land, pending on doing the tests for them.

[Xan Lopez]

Created attachment 40506 [details]
caret-cleanup.diff

OK, spliting this a bit.

This just moves the code in EditorClientGtk.cpp to WebCore, but does not change anything else. This code is only used when caret browse mode is enabled, which only the GTK+ port does, so it should be totally safe (ie, no behavior change).

The extra editorCommands that are missing will be added when I add the dumpEditorCommands stuff to our DRT, so that I'm able to test them :)

[Eric Seidel (no email)]

Comment on attachment 40506 [details]
caret-cleanup.diff

This is adding Editor commands which are exposed to JavaScript, so this needs LayoutTests.

[Xan Lopez]

(In reply to comment #31)
> (From update of attachment 40506 [details])
> This is adding Editor commands which are exposed to JavaScript, so this needs
> LayoutTests.

It does not add any new command, it just allows those useful in caret browsing mode to be used when that is enabled.

[Eric Seidel (no email)]

Comment on attachment 40506 [details]
caret-cleanup.diff

My mistake.  Looks fine.

[Xan Lopez]

Comment on attachment 40506 [details]
caret-cleanup.diff

Landed as r49193, clearing flags. I'll leave this open since there's still one thing to land (the two new editor commands).

[Joanmarie Diggs]

(In reply to comment #34)
> (From update of attachment 40506 [details])
> Landed as r49193, clearing flags. I'll leave this open since there's still one
> thing to land (the two new editor commands).

Ping. :-) (Aren't home and end done and just in need of review and commit?)

[Xan Lopez]

(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 40506 [details] [details])
> > Landed as r49193, clearing flags. I'll leave this open since there's still one
> > thing to land (the two new editor commands).
> 
> Ping. :-) (Aren't home and end done and just in need of review and commit?)

We need features in DRT we don't have atm to be able to land this, see comment #30

[Joanmarie Diggs]

(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > (From update of attachment 40506 [details] [details] [details])
> > > Landed as r49193, clearing flags. I'll leave this open since there's still one
> > > thing to land (the two new editor commands).
> > 
> > Ping. :-) (Aren't home and end done and just in need of review and commit?)
> 
> We need features in DRT we don't have atm to be able to land this, see comment #30

Ugh. I'm working on Orca's script for Epiphany and could really, really use this final bit of the fix. Pretty please with sugar on top? :-) 

Jokes aside, it looks like it's been over six months -- since the last ping. :-( If there's some way this could be addressed, it would be awesome. Thanks!

[Mario Sanchez Prada]

(In reply to comment #30)
> Created an attachment (id=40506) [details]
> caret-cleanup.diff
> 
> OK, spliting this a bit.
> 
> This just moves the code in EditorClientGtk.cpp to WebCore, but does not change anything else. This 
> code is only used when caret browse mode is enabled, which only the GTK+ port does, so it should be 
> totally safe (ie, no behavior change).
> 
> The extra editorCommands that are missing will be added when I add the dumpEditorCommands stuff to 
> our DRT, so that I'm able to test them :)

I'd like to start tackling this issue asap but I'm a bit clueless when it comes to understanding what you mean with "the extra editorCommands" and "the dumpEditorCommands stuff" :-)

Could you please give a little bit more of detail then about:

  - Which ones are those "extra editorCommands", I assume thos are Ctrl+Home and Ctrl+End, but could you please confirm that? (based on mozilla's proposal I assume expected behaviour for those is moving the caret to the beginning/end of the current document, right?)

  - What that "dumpEditorCommands stuff" is about? This is my biggest doubt

Perhaps that way I could take the lead on this bug and trying to fix it & provide the missing bits in DRT so we could create a layout test for it and land everything together.

Thanks in advance.

[Mario Sanchez Prada]

Created attachment 76080 [details]
Move to Beginning/End of the document + Layout test

(In reply to comment #38)
> [...]
>   - Which ones are those "extra editorCommands", I assume thos are Ctrl+Home
> and Ctrl+End, but could you please confirm that? (based on mozilla's proposal I
> assume expected behaviour for those is moving the caret to the beginning/end
> of the current document, right?)
>
>   - What that "dumpEditorCommands stuff" is about? This is my biggest doubt

Attaching a new patch that would implement these last bits making the most of this new "dumpEditorCommands" tool we now have in DRT.

[Mario Sanchez Prada]

Created attachment 76092 [details]
Move to Beginning/End of the document + Layout test

This is the right patch to be reviewed, sorry.

[Xan Lopez]

Comment on attachment 76092 [details]
Move to Beginning/End of the document + Layout test

Woot!

[Mario Sanchez Prada]

Committed r73918: <http://trac.webkit.org/changeset/73918>

[Antonio Gomes]

I am about to make Qt also support properly caret browsing, at least for testing proposes. See bug 50536. It would be great if we did not add any further Gtk specific caret browsing tests after it lands.

  Test: platform/gtk/editing/selection/caret-mode-document-begin-end.html

Also you named the bug  "[GTK]" and it just touches cross platform code. Please also avoid it.

[WebKit Review Bot]

http://trac.webkit.org/changeset/73918 might have broken GTK Linux 64-bit Debug

[Mario Sanchez Prada]

(In reply to comment #43)
> I am about to make Qt also support properly caret browsing, at least for testing proposes. See bug 50536. It would be great if we did not add any further Gtk specific caret browsing tests after it lands.
> 
>   Test: platform/gtk/editing/selection/caret-mode-document-begin-end.html

I just filed bug 50942 for taking care of bug as soon as we get integrated all the related patches currently pending on review (bug 25533 and bug 27048).

> Also you named the bug  "[GTK]" and it just touches cross platform code. Please also avoid it.

I'll try not to put that prefix and use the Gtk keyword instead.