Bug 25526 - [Gtk] Additional support is needed for caret browsing
Summary: [Gtk] Additional support is needed for caret browsing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Xan Lopez
URL:
Keywords: Gtk
Depends on: 49143
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-05-03 13:49 PDT by Joanmarie Diggs
Modified: 2010-12-13 10:35 PST (History)
11 users (show)

See Also:


Attachments
caretrefactor.patch (19.41 KB, patch)
2009-05-04 10:27 PDT, Xan Lopez
eric: review-
Details | Formatted Diff | Diff
homeendcaret.patch (7.21 KB, patch)
2009-05-05 03:51 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
textcaretmoved.patch (4.42 KB, patch)
2009-05-05 09:58 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
textcaretv2.patch (4.52 KB, patch)
2009-05-05 23:17 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
textselectionchanged.patch (2.54 KB, patch)
2009-05-06 02:18 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
caretextra.patch (20.95 KB, patch)
2009-06-17 04:53 PDT, Xan Lopez
eric: review-
Details | Formatted Diff | Diff
caret-cleanup.diff (19.00 KB, patch)
2009-10-02 04:01 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Move to Beginning/End of the document + Layout test (11.26 KB, patch)
2010-12-09 10:09 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Move to Beginning/End of the document + Layout test (12.42 KB, patch)
2010-12-09 10:40 PST, Mario Sanchez Prada
xan.lopez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs 2009-05-03 13:49:02 PDT
(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.
Comment 1 Xan Lopez 2009-05-04 02:15:16 PDT
(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?
Comment 2 Xan Lopez 2009-05-04 09:29:50 PDT
(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?
Comment 3 Joanmarie Diggs 2009-05-04 09:55:34 PDT
(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. :-)
Comment 4 Joanmarie Diggs 2009-05-04 10:13:18 PDT
(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....
Comment 5 Xan Lopez 2009-05-04 10:27:49 PDT
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.
Comment 6 Xan Lopez 2009-05-04 10:31:02 PDT
CCing Justin so he can hopefully have a look at the patch.
Comment 7 Xan Lopez 2009-05-04 10:32:03 PDT
(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.

Comment 8 Xan Lopez 2009-05-05 03:51:38 PDT
Created attachment 30015 [details]
homeendcaret.patch

This adds support for Home and End with their modifiers.
Comment 9 Xan Lopez 2009-05-05 09:58:26 PDT
Created attachment 30022 [details]
textcaretmoved.patch

Emit AtkText::text-caret-moved when the selection changes.
Comment 10 Xan Lopez 2009-05-05 23:17:40 PDT
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.
Comment 11 Xan Lopez 2009-05-06 02:18:32 PDT
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.
Comment 12 Gustavo Noronha (kov) 2009-05-06 07:03:25 PDT
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 =).
Comment 13 Gustavo Noronha (kov) 2009-05-06 07:03:43 PDT
Comment on attachment 30045 [details]
textselectionchanged.patch

And here goes text-selection-changed.
Comment 14 Joanmarie Diggs 2009-05-06 20:24:40 PDT
> 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!!
Comment 15 Joanmarie Diggs 2009-05-06 23:03:24 PDT
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.
Comment 16 Brent Fulgham 2009-06-10 10:33:08 PDT
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?
Comment 17 Xan Lopez 2009-06-10 10:35:52 PDT
(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...
Comment 18 Jan Alonzo 2009-06-12 15:28:37 PDT
(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.

Comment 19 Eric Seidel (no email) 2009-06-15 13:49:30 PDT
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.
Comment 20 Xan Lopez 2009-06-15 13:58:06 PDT
(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.
Comment 21 Xan Lopez 2009-06-17 04:53:53 PDT
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.
Comment 22 Eric Seidel (no email) 2009-06-18 17:12:00 PDT
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.
Comment 23 Eric Seidel (no email) 2009-06-21 01:49:16 PDT
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.
Comment 24 Adam Barth 2009-07-17 00:11:07 PDT
We should either decide to land these or remove them from the queue.  Any guidance on which is best?
Comment 25 Holger Freyther 2009-07-17 00:19:06 PDT
Comment on attachment 30045 [details]
textselectionchanged.patch

This patch got landed, clear review to make it drop out of the review queue.
Comment 26 Xan Lopez 2009-07-24 01:13:02 PDT
Assign the bug to myself.
Comment 27 Eric Seidel (no email) 2009-08-13 14:48:15 PDT
Comment on attachment 30015 [details]
homeendcaret.patch

Based on above comments this was already landed (unsure what revision?).  Clearing r+ flag.
Comment 28 Eric Seidel (no email) 2009-08-13 14:48:56 PDT
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).
Comment 29 Xan Lopez 2009-08-26 00:06:19 PDT
(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.
Comment 30 Xan Lopez 2009-10-02 04:01:09 PDT
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 :)
Comment 31 Eric Seidel (no email) 2009-10-06 08:53:55 PDT
Comment on attachment 40506 [details]
caret-cleanup.diff

This is adding Editor commands which are exposed to JavaScript, so this needs LayoutTests.
Comment 32 Xan Lopez 2009-10-06 08:57:24 PDT
(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.
Comment 33 Eric Seidel (no email) 2009-10-06 09:35:48 PDT
Comment on attachment 40506 [details]
caret-cleanup.diff

My mistake.  Looks fine.
Comment 34 Xan Lopez 2009-10-06 10:32:37 PDT
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).
Comment 35 Joanmarie Diggs 2010-01-05 18:41:50 PST
(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?)
Comment 36 Xan Lopez 2010-01-19 09:04:54 PST
(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
Comment 37 Joanmarie Diggs 2010-09-03 08:32:12 PDT
(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!
Comment 38 Mario Sanchez Prada 2010-11-12 02:50:42 PST
(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.
Comment 39 Mario Sanchez Prada 2010-12-09 10:09:31 PST
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.
Comment 40 Mario Sanchez Prada 2010-12-09 10:40:47 PST
Created attachment 76092 [details]
Move to Beginning/End of the document + Layout test

This is the right patch to be reviewed, sorry.
Comment 41 Xan Lopez 2010-12-13 03:48:43 PST
Comment on attachment 76092 [details]
Move to Beginning/End of the document + Layout test

Woot!
Comment 42 Mario Sanchez Prada 2010-12-13 07:54:29 PST
Committed r73918: <http://trac.webkit.org/changeset/73918>
Comment 43 Antonio Gomes 2010-12-13 08:45:17 PST
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.
Comment 44 WebKit Review Bot 2010-12-13 09:05:48 PST
http://trac.webkit.org/changeset/73918 might have broken GTK Linux 64-bit Debug
Comment 45 Mario Sanchez Prada 2010-12-13 10:35:19 PST
(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.