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: | P2 | Keywords: | Gtk | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||
Bug Depends on: | 49143 | ||||||||||||||||||||||
Bug Blocks: | 25531 | ||||||||||||||||||||||
Attachments: |
|
Description
Joanmarie Diggs
2009-05-03 13:49:02 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? (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? (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. :-) (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.... 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.
CCing Justin so he can hopefully have a look at the patch. (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. Created attachment 30015 [details]
homeendcaret.patch
This adds support for Home and End with their modifiers.
Created attachment 30022 [details]
textcaretmoved.patch
Emit AtkText::text-caret-moved when the selection changes.
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.
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 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 on attachment 30045 [details]
textselectionchanged.patch
And here goes text-selection-changed.
> 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!! 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. 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? (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... (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 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.
(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. 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 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 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.
We should either decide to land these or remove them from the queue. Any guidance on which is best? Comment on attachment 30045 [details]
textselectionchanged.patch
This patch got landed, clear review to make it drop out of the review queue.
Assign the bug to myself. Comment on attachment 30015 [details]
homeendcaret.patch
Based on above comments this was already landed (unsure what revision?). Clearing r+ flag.
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). (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. 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 on attachment 40506 [details]
caret-cleanup.diff
This is adding Editor commands which are exposed to JavaScript, so this needs LayoutTests.
(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 on attachment 40506 [details]
caret-cleanup.diff
My mistake. Looks fine.
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). (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?) (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 (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! (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. 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. Created attachment 76092 [details]
Move to Beginning/End of the document + Layout test
This is the right patch to be reviewed, sorry.
Comment on attachment 76092 [details]
Move to Beginning/End of the document + Layout test
Woot!
Committed r73918: <http://trac.webkit.org/changeset/73918> 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. http://trac.webkit.org/changeset/73918 might have broken GTK Linux 64-bit Debug (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. |