Bug 16135 – [GTK] Support caret browsing

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

[Alp Toker]

Caret browsing is an accessibility feature that allows in-page navigation similar to content editable, displaying an insertion caret which can be moved with the arrow keys. However, the content is not actually editable, and when the caret is in a hyperlink, the hyperlink gets a focus ring and hitting enter causes it to get activated.

This is enabled in Firefox by hitting F7, for example.

[Alp Toker]

GtkTextView also supports a caret mode:

  http://library.gnome.org/devel/gtk/2.12/GtkTextView.html

<snip>

gtk_text_view_set_cursor_visible ()

void                gtk_text_view_set_cursor_visible    (GtkTextView *text_view,
                                                         gboolean setting);

Toggles whether the insertion point is displayed. A buffer with no editable text probably shouldn't have a visible cursor, so you may want to turn the cursor off.

</snip>

This should probably be the inspiration for the API we choose.

[Alp Toker]

Created attachment 23778 [details]
Caret navigation (proof of concept)

This patch enables navigation using a blinking caret without interfering with other editing capabilities. What needs work:

1) Make this into a configurable web setting for WebSettings/WebPreferences instead of hijacking AXObjectCache::enableEnhancedUserInterfaceAccessibility() (which we may later be able to obsolete depending on Mac a11y requirements).

2) Focus elements when they're entered.

3) Caret sometimes get 'trapped' in a node. Should integrate more closely with SelectionController/FocusController

[Alp Toker]

Created attachment 24982 [details]
WIP caret navigation support

This patch is against WebKit r38232 (but should apply to later revisions).

To try it out:

$ WEBKIT_CARET=1 Programs/GtkLauncher

There are some caret browsing changes in this patch that it will probably make sense to enable in the non-caret-browsing mode too rather than making them conditional.

Known issues:

1) Caret sometimes gets stuck and needs a tab or up/down to get going again.
2) Patch breaks focusing into a WebView by tabbing; easy to fix.

It's worth doing a search in the bug tracker for 'caret' since issues like (1) have often already been noticed and reported by users of WebKit's HTML-editing mode which shares code with the feature in this patch.

The code is portable and should work in other WebKit ports (Mac, Win, Qt, Wx, Chrome) but it may depend on some recent focus changes that are currently only in the GTK+ port that the other ports may need to pick up if they want to switch on this feature. It may also require modifications to the EditorClient in those ports to work correctly.

I could do with feedback from

1) WebKit developers familiar with the focus and editing code. Are we on the right track here?
2) People who use or are familiar with caret browsing in Mozilla. Does this do what you need? What's missing and what could work better?
3) Other port maintainers. Does this Setting work in your WebKit port?

Cheers

[Luca Ferretti]

Just a couple of links.

Mozilla Keyboard Navigation
http://www.mozilla.org/access/keyboard/proposal

GNOME Caret Navigation Mode
http://library.gnome.org/users/gnome-access-guide/stable/keynav-52.html

[Xan Lopez]

Created attachment 27035 [details]
0001-Update-alp-s-patch-to-trunk.patch

Updated Alp's patch to trunk, as some the code and some assumptions had been changed in trunk. I've also removed all 'bool caretBrowsing = true;' hardcodings, and try to check the setting everywhere.

Other than that the same bugs and way of testing he mentions still apply, and it would still be nice to get some feedback.

[Xan Lopez]

Created attachment 27073 [details]
Caret browsing v3

I started refactoring the code in EditorClientGTK to handle properly events with caret browsing (most keys but up/down/left/right would not work in caret mode, for example), and halfway through realized I was basically getting to do what was already done in the Windows port. So I took that code and modified it to handle differently up/down/left/right keys in caret mode instead.

With this the keys works as expected in both caret and non-caret mode, and I haven't been able to reproduce the "getting stuck in a node" bug either (but I couldn't say if it's gone for good).

[Xan Lopez]

Just for the record, I went through http://www.mozilla.org/access/keyboard/proposal:

- Everything in continuous navigation seems to work OK. We behave as if layout.word_select.eat_space_to_next_word were set to FALSE and layout.word_select.stop_at_punctuation to TRUE (Mozilla says it should be FALSE by default in !Windows)

- Everything in discontinuous navigation seems to work except (Ctrl+)Home/End. Probably those need to be special cased in EditorClient too for CaretBrowsing if we use the current approach.

- Text selection: seems to work in general but it gets 'stuck' in some situations.

- Control: in general forms are not auto-focused in caret mode, so it's a bit hard to test most of the stuff listed.

[Alp Toker]

Comment on attachment 27073 [details]
Caret browsing v3

Xan,

Thanks for picking this up.

I suppose the interpretKeyEvent() brings this in line with the other ports so that's fair enough.

The g_thread_init () change doesn't belong in this patch.

I think this is all good as long as it doesn't change behaviour on other ports. Maybe with a ChangeLog entry we can get this reviewed (I'd best not review my own code!)

Cheers

[Xan Lopez]

(In reply to comment #8)
> (From update of attachment 27073 [details] [review])
> The g_thread_init () change doesn't belong in this patch.

Right, it was needed for GtkLauncher to run though. I did it so Sun guys could test it and give feedback (but apparently they never got to it).

> 
> I think this is all good as long as it doesn't change behaviour on other ports.
> Maybe with a ChangeLog entry we can get this reviewed (I'd best not review my
> own code!)
> 
> Cheers
> 

I tried to be careful to make it so that the new code is never executed if the caret pref is not enabled, so it should be safe. I can add the ChangeLog if you think this could go in already, but there are still some quite important things missing (see previous comments).

[Willie Walker]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 27073 [details] [review] [review])
> > The g_thread_init () change doesn't belong in this patch.
> 
> Right, it was needed for GtkLauncher to run though. I did it so Sun guys could
> test it and give feedback (but apparently they never got to it).

Hey guys - thanks much for the work on this right now.  Yeah - we've been pretty busy working on some nasty problems and haven't had a chance to look at this yet.

I haven't yet Google'd for instructions on how to do so, but do you know of anyone who has had success building/using WebKit on OpenSolaris?  If they have tips/tricks/hints, it'd be great to not have to learn them on my own.

[Xan Lopez]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 27073 [details] [review] [review] [review])
> > > The g_thread_init () change doesn't belong in this patch.
> > 
> > Right, it was needed for GtkLauncher to run though. I did it so Sun guys could
> > test it and give feedback (but apparently they never got to it).
> 
> Hey guys - thanks much for the work on this right now.  Yeah - we've been
> pretty busy working on some nasty problems and haven't had a chance to look at
> this yet.
> 
> I haven't yet Google'd for instructions on how to do so, but do you know of
> anyone who has had success building/using WebKit on OpenSolaris?  If they have
> tips/tricks/hints, it'd be great to not have to learn them on my own.
> 

Hey, I really have no clue about that, although googling 'webkit gtk opensolaris' gives a few hits with people that apparently managed to build it without much trouble.

[Xan Lopez]

Created attachment 29645 [details]
basiccaret.patch

Implements basic caret browsing support in WebCore. Most of the code is just adding the setting and enabling the right codepaths (shared with editable mode) when needed. The changes in FocusController are needed for TAB to do the right thing when caret mode is enabled.

[Xan Lopez]

Created attachment 29646 [details]
gtksetting.patch

Add the caret mode setting to the GTK port.

[Xan Lopez]

Created attachment 29647 [details]
editorclientkeys.patch

Refactor key event handling in EditorClientGTK to work under caret mode.

[Gustavo Noronha (kov)]

Comment on attachment 29646 [details]
gtksetting.patch

Looks correct to me, I understand the first patch needs to go in prior to this one, but r=me when the first one gets reviewed.

[Gustavo Noronha (kov)]

Comment on attachment 29647 [details]
editorclientkeys.patch

> +    if (caretBrowsing) {
> +        switch (keyEvent->windowsVirtualKeyCode()) {
>              case VK_LEFT:
> -                frame->selection()->modify(kevent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> +                frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
>                          SelectionController::LEFT,
> -                        kevent->ctrlKey() ? WordGranularity : CharacterGranularity,
> +                        keyEvent->ctrlKey() ? WordGranularity : CharacterGranularity,
>                          true);
> -                break;
> +                return true;

There seems to be code very similar to this one here, btw: EventHandler::handleKeyboardSelectionMovement (in WebCore/page/EventHandler.cpp). This code seems to be called Perhaps we are doing some unneeded work?

> +    const PlatformKeyboardEvent* keyEvent = evt->keyEvent();
> +    if (!keyEvent /*|| keyEvent->isSystemKey()*/)  // do not treat this as text input if it's a system key event
> +        return false;

keyEvent->isSystemKey seems to be something used only by windows? We probably want to remove both comments here.
  
> -    // FIXME: Use GtkBindingSet instead of this hard-coded switch
> -    // http://bugs.webkit.org/show_bug.cgi?id=15911

We are now using GtkBindingSet in the webview, but we will be using the static table for editing? I am still trying to grasp how key handling works, but it seems to me like most things such as home/end/pageup/pagedown should be handled by that code you added to WebView, instead?

> +    // It's not quite clear whether clipboard shortcuts and Undo/Redo should be handled
> +    // in the application or in WebKit. We chose WebKit.
> +    { 'C',       CtrlKey,            "Copy"                                        },
> +    { 'V',       CtrlKey,            "Paste"                                       },
> +    { 'X',       CtrlKey,            "Cut"                                         },
> +    { 'A',       CtrlKey,            "SelectAll"                                   },
> +    { VK_INSERT, CtrlKey,            "Copy"                                        },
> +    { VK_DELETE, ShiftKey,           "Cut"                                         },
> +    { VK_INSERT, ShiftKey,           "Paste"                                       },
> +    { 'Z',       CtrlKey,            "Undo"                                        },
> +    { 'Z',       CtrlKey | ShiftKey, "Redo"                                        },
> +};
[...]
> +    Editor::Command command = frame->editor()->command(interpretKeyEvent(evt));
[...]
> +void EditorClient::handleKeyboardEvent(KeyboardEvent* event)
> +{
> +    if (handleEditingKeyboardEvent(event))
> +        event->setDefaultHandled();
>  }

An old commit seems to have moved cut/copy/paste/select-all from here to our WebView object (see http://trac.webkit.org/changeset/28386), so I think we should probably avoid handling them here? We should probably consider moving undo/redo, in the future, too.

[Xan Lopez]

(In reply to comment #16)
> (From update of attachment 29647 [details] [review])
> > +    if (caretBrowsing) {
> > +        switch (keyEvent->windowsVirtualKeyCode()) {
> >              case VK_LEFT:
> > -                frame->selection()->modify(kevent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> > +                frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> >                          SelectionController::LEFT,
> > -                        kevent->ctrlKey() ? WordGranularity : CharacterGranularity,
> > +                        keyEvent->ctrlKey() ? WordGranularity : CharacterGranularity,
> >                          true);
> > -                break;
> > +                return true;
> 
> There seems to be code very similar to this one here, btw:
> EventHandler::handleKeyboardSelectionMovement (in
> WebCore/page/EventHandler.cpp). This code seems to be called Perhaps we are
> doing some unneeded work?

Mmm, it seems to be conditionally called if some a11y setting is enabled, but I think it's not in our case. I guess you are talking about EventHandler::defaultKeyboardEventHandler here? I can look into this.

> 
> > +    const PlatformKeyboardEvent* keyEvent = evt->keyEvent();
> > +    if (!keyEvent /*|| keyEvent->isSystemKey()*/)  // do not treat this as text input if it's a system key event
> > +        return false;
> 
> keyEvent->isSystemKey seems to be something used only by windows? We probably
> want to remove both comments here.

You are right.

> 
> > -    // FIXME: Use GtkBindingSet instead of this hard-coded switch
> > -    // http://bugs.webkit.org/show_bug.cgi?id=15911
> 
> We are now using GtkBindingSet in the webview, but we will be using the static
> table for editing? I am still trying to grasp how key handling works, but it
> seems to me like most things such as home/end/pageup/pagedown should be handled
> by that code you added to WebView, instead?
> 

The motion keys in editor mode handle and modify the selection, while the stuff in the view scroll the view, so AFAICT they should not be handled by the same code. What could be done here, probably, is change the code to use GtkBinding too.

> > +    // It's not quite clear whether clipboard shortcuts and Undo/Redo should be handled
> > +    // in the application or in WebKit. We chose WebKit.
> > +    { 'C',       CtrlKey,            "Copy"                                        },
> > +    { 'V',       CtrlKey,            "Paste"                                       },
> > +    { 'X',       CtrlKey,            "Cut"                                         },
> > +    { 'A',       CtrlKey,            "SelectAll"                                   },
> > +    { VK_INSERT, CtrlKey,            "Copy"                                        },
> > +    { VK_DELETE, ShiftKey,           "Cut"                                         },
> > +    { VK_INSERT, ShiftKey,           "Paste"                                       },
> > +    { 'Z',       CtrlKey,            "Undo"                                        },
> > +    { 'Z',       CtrlKey | ShiftKey, "Redo"                                        },
> > +};
> [...]
> > +    Editor::Command command = frame->editor()->command(interpretKeyEvent(evt));
> [...]
> > +void EditorClient::handleKeyboardEvent(KeyboardEvent* event)
> > +{
> > +    if (handleEditingKeyboardEvent(event))
> > +        event->setDefaultHandled();
> >  }
> 
> An old commit seems to have moved cut/copy/paste/select-all from here to our
> WebView object (see http://trac.webkit.org/changeset/28386), so I think we
> should probably avoid handling them here? We should probably consider moving
> undo/redo, in the future, too.
> 

You are right, in this case it seems to be the same code. I'll check this too.

Thanks for the review!

[Xan Lopez]

Created attachment 29737 [details]
editorclientkeysv2.patch

OK, I've removed the systemKey commented code, and the handling of the clipboard functions from here.

The GtkBinding thing would be nice, but I don't see an issue way of doing it here, since for the bindings to work you need to go the parent class of the view in keypress, which happens quite a bit later than this.

Also, the code for up/down/etc in caretBrowsing here is actually different than the one in EventHandler, so I've left it here for now. We may unify it, but I think it should be done in a different patch.

[Gustavo Noronha (kov)]

Comment on attachment 29737 [details]
editorclientkeysv2.patch

(In reply to comment #18)
> The GtkBinding thing would be nice, but I don't see an issue way of doing it
> here, since for the bindings to work you need to go the parent class of the
> view in keypress, which happens quite a bit later than this.
> 
> Also, the code for up/down/etc in caretBrowsing here is actually different than
> the one in EventHandler, so I've left it here for now. We may unify it, but I
> think it should be done in a different patch.

Those sound sane to me. This one is go in my opinion, too. So r=me. Now let's find someone to get the first one looked at.

[Joanmarie Diggs]

I applied basiccaret.patch, gtksetting.patch, and editorclientkeyssv2.patch. I'm running it with:

WEBKIT_CARET=1 Programs/GtkLauncher

But I seem to be caret-less. I'm sure I'm missing something silly, but I'm not seeing it at the moment. Hints would be appreciated. :-)

Thanks!

[Xan Lopez]

I'm sorry!

You need to add this small snippet to the code for GtkLauncher, in WebKitTools/GtkLauncher/main.c:

+    WebKitWebSettings* settings = webkit_web_settings_new ();
+    g_object_set (settings,
+                  "enable-caret-browsing", g_getenv ("WEBKIT_CARET") ? TRUE : FALSE,
+                  NULL);
+    webkit_web_view_set_settings (web_view, settings);

I forgot about this because I use Epiphany for testing now, so I removed this from the patch (since it's not meant to be committed anyway).

I'll upload a patch with that now for testers.

[Xan Lopez]

Created attachment 29799 [details]
caretmodelauncher.patch

Enable caret mode in GtkLauncher with WEBKIT_CARET=1 ./GtkLauncher.

[Joanmarie Diggs]

(In reply to comment #22)
> Created an attachment (id=29799) [review]
> caretmodelauncher.patch
> 
> Enable caret mode in GtkLauncher with WEBKIT_CARET=1 ./GtkLauncher.
> 

Awesome, thanks! Works like a charm now.

In looking at this with Accerciser's event monitor, I'm not seeing any caret-moved events being emitted. Are you seeing them, or is that part of the work in progress?

[Xan Lopez]

(In reply to comment #23)
> In looking at this with Accerciser's event monitor, I'm not seeing any
> caret-moved events being emitted. Are you seeing them, or is that part of the
> work in progress?
> 

That's indeed still missing :)

[Joanmarie Diggs]

(In reply to comment #24)
> That's indeed still missing :)

No worries. Just checking. Some of the patches didn't apply cleanly to trunk (due to changes with spell checking iirc) so I had to do some hand-patching. I wanted to rule out the all-too-likely possibility of user error. :-)

Other observations, which I'll jot down here for now:

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

That said.... This is still awesome progress. Thanks again! And let me know if you want new bugs filed for any/all of the above.

[Justin Garcia]

Comment on attachment 29645 [details]
basiccaret.patch

+    bool caretBrowsing = settings() && settings()->caretBrowsingEnabled();
     bool shouldBlink = m_caretVisible
-        && selection()->isCaret() && selection()->isContentEditable();
+        && selection()->isCaret() && (selection()->isContentEditable() || caretBrowsing);

I was under the impression that we would not blink when caret browsing.  To provide a cue that the content is non editable.


+    if (!selection()->isNone() || !(isContentEditable() || caretBrowsing))

You don't need the ()s here.


         // If this is a change in selection resulting from a delete operation,
         // oldSelection may no longer be in the document.
-        if (closeTyping && oldSelection.isContentEditable() && oldSelection.start().node() && oldSelection.start().node()->inDocument()) {
+        if (closeTyping && (oldSelection.isContentEditable() || caretBrowsing)  && oldSelection.start().node() && oldSelection.start().node()->inDocument()) {
             VisiblePosition oldStart(oldSelection.visibleStart());

Do you really want to mark misspelled words if they are non-editable?  The user can't do anything about them.

[Justin Garcia]

Comment on attachment 29645 [details]
basiccaret.patch

also  we should have a bug on entering form elements when caret browsing or in editable content.  other than those things, r=me

[Xan Lopez]

(In reply to comment #26)
> (From update of attachment 29645 [details] [review])
> +    bool caretBrowsing = settings() && settings()->caretBrowsingEnabled();
>      bool shouldBlink = m_caretVisible
> -        && selection()->isCaret() && selection()->isContentEditable();
> +        && selection()->isCaret() && (selection()->isContentEditable() ||
> caretBrowsing);
> 
> I was under the impression that we would not blink when caret browsing.  To
> provide a cue that the content is non editable.
> 

Hum, it seems that we actually need this in order for the caret to be visible. Also, FWIW, in Gecko the caret actually blinks, most probably to make it easier (or possible) to figure out where it is.

> 
> +    if (!selection()->isNone() || !(isContentEditable() || caretBrowsing))
> 
> You don't need the ()s here.
>

Mmm, are you sure? You mean like !isContentEditable() || !caretBrowsing ? That's not really the same... 

[Justin Garcia]

> > +    if (!selection()->isNone() || !(isContentEditable() || caretBrowsing))
> > 
> > You don't need the ()s here.
> >
> 
> Mmm, are you sure? You mean like !isContentEditable() || !caretBrowsing ?
> That's not really the same... 

Oops my mistake I misread it.

[Xan Lopez]

So is the patch OK to go with the blinking bit on it?

[Xan Lopez]

(In reply to comment #30)
> So is the patch OK to go with the blinking bit on it?
> 

I'm going to go ahead and commit this, to be consistent with the de-facto standard. If you disagree we can work on this issue in future patches. Thanks for the review!