Bug 25526

Summary:

[Gtk] Additional support is needed for caret browsing

Product:

WebKit

Reporter:

Joanmarie Diggs <jdiggs>

Component:

Accessibility

Assignee:

Xan Lopez <xan.lopez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, agl, apinheiro, eric, jmalonzo, justin.garcia, mario, tonikitoo, walker.willie, webkit.review.bot, xan.lopez

Priority:

Keywords:

Gtk

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Bug Depends on:

49143

Bug Blocks:

25531

Attachments:

Description	Flags
caretrefactor.patch	eric: review-
homeendcaret.patch	none
textcaretmoved.patch	none
textcaretv2.patch	none
textselectionchanged.patch	none
caretextra.patch	eric: review-
caret-cleanup.diff	none
Move to Beginning/End of the document + Layout test	none
Move to Beginning/End of the document + Layout test	xan.lopez: review+

Joanmarie Diggs

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

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Xan Lopez

Comment 1 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?

Xan Lopez

Comment 2 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?

Joanmarie Diggs

Comment 3 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. :-)

Joanmarie Diggs

Comment 4 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....

Xan Lopez

Comment 5 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.

Xan Lopez

Comment 6 2009-05-04 10:31:02 PDT

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

Xan Lopez

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

Xan Lopez

Comment 8 2009-05-05 03:51:38 PDT

Created attachment 30015 [details] homeendcaret.patch This adds support for Home and End with their modifiers.

Xan Lopez

Comment 9 2009-05-05 09:58:26 PDT

Created attachment 30022 [details] textcaretmoved.patch Emit AtkText::text-caret-moved when the selection changes.

Xan Lopez

Comment 10 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.

Xan Lopez

Comment 11 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.

Gustavo Noronha (kov)

Comment 12 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 =).

Gustavo Noronha (kov)

Comment 13 2009-05-06 07:03:43 PDT

Comment on attachment 30045 [details] textselectionchanged.patch And here goes text-selection-changed.

Joanmarie Diggs

Comment 14 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!!

Joanmarie Diggs

Comment 15 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.

Brent Fulgham

Comment 16 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?

Xan Lopez

Comment 17 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...

Jan Alonzo

Comment 18 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.

Eric Seidel (no email)

Comment 19 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.

Xan Lopez

Comment 20 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.

Xan Lopez

Comment 21 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.

Eric Seidel (no email)

Comment 22 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.

Eric Seidel (no email)

Comment 23 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.

Adam Barth

Comment 24 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?

Holger Freyther

Comment 25 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.

Xan Lopez

Comment 26 2009-07-24 01:13:02 PDT

Assign the bug to myself.

Eric Seidel (no email)

Comment 27 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.

Eric Seidel (no email)

Comment 28 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).

Xan Lopez

Comment 29 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.

Xan Lopez

Comment 30 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 :)

Eric Seidel (no email)

Comment 31 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.

Xan Lopez

Comment 32 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.

Eric Seidel (no email)

Comment 33 2009-10-06 09:35:48 PDT

Comment on attachment 40506 [details] caret-cleanup.diff My mistake. Looks fine.

Xan Lopez

Comment 34 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).

Joanmarie Diggs

Comment 35 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?)

Xan Lopez

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

Joanmarie Diggs

Comment 37 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!

Mario Sanchez Prada

Comment 38 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.

Mario Sanchez Prada

Comment 39 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.

Mario Sanchez Prada

Comment 40 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.

Xan Lopez

Comment 41 2010-12-13 03:48:43 PST

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

Mario Sanchez Prada

Comment 42 2010-12-13 07:54:29 PST

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

Antonio Gomes

Comment 43 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.

WebKit Review Bot

Comment 44 2010-12-13 09:05:48 PST

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

Mario Sanchez Prada

Comment 45 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.

Note You need to log in before you can comment on or make changes to this bug.