1370496 - [linux] [osk] on screen keyboard does not hide when entry lose focus

Status: NEW

(Core :: Disability Access APIs, defect, P3)

Description

[Jan Horak [:jhorak]]

On screen keyboard remains open even after the entry element has lost focus.
It is because the atk_object_notify_state_change(atkObj, ATK_STATE_FOCUSED, false) is not called on the blur event (there's no blur event handler in atk code for now, only focus[1]).

This been found during debugging bug 789038.

[1] http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/accessible/atk/AccessibleWrap.cpp#1247

Comment 1

[Jan Horak [:jhorak]]

Attachment: Bug 1370496 - Hide on screen keyboard when input lose focus :surkov

This patch modifies focus manager to fire an EVENT_BLUR and handling it in atk implementation because
ATM the atk library is not notified of entry losing focus, so the on screen keyboard does not hide.

Review commit: https://reviewboard.mozilla.org/r/146248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146248/

Comment 2

[Jan Horak [:jhorak]]

Hm, failing a lot of test, will have to look into it. Anyway, is this approach fine or not?

Comment 3

[Jan Horak [:jhorak]]

Comment on attachment 8874852 [details]
Bug 1370496 - Hide on screen keyboard when input lose focus :surkov

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146248/diff/1-2/

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Eitan can you look at this one?

Comment 5

[Eitan Isaacson [:eeejay]]

This looks right to me. I think adding EVENT_BLUR instead of amending other events is good because it won't disrupt other platforms.

Another approach you can take in the focus tracker. Instead of listening for DOM blur, you can send EVENT_BLUR for the stored focused accessible before updating it and sending EVENT_FOCUS for the new one. The only pitfall would be dispatching blur events on dead objects, since a typical case for a DOM focus is when the previously focused element was destroyed.

From looking at the Caribou code[1] (which I may have wrote!), it looks like it only waits for focus events on certain roled accessibles. What if it hid the keyboard on inversely matched events (ie. a focus event on a non-editable accessible)?

I think we should add this in Gecko because it is how AT-SPI apps should behave, but just in case Caribou could be robustified..


1. https://git.gnome.org/browse/caribou/tree/daemon/daemon.vala#n83

Comment 6

[Eitan Isaacson [:eeejay]]

Alex might have some feedback about this approach as well. He has more context around FocusManager, etc.

Comment 7

[alexander :surkov (:asurkov)]

DOM blur event doesn't cover all cases, since DOM focus is a subset of user focus. I find that 'fire unfocus on a previous item right before a new item grabs the focus' is a more workable idea.

Having said that, I suppose there's no such a thing as unfocus event aka atk_object_notify_state_change(atkObj, ATK_STATE_FOCUSED, false), because if something gets unfocused, then something else has to acquire the focus.

I don't know what ATK says though, and if we have to unfocus a thing before focusing another one, then it's fine with me.

Joanie, what do you think on it?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Jan Horak from comment #2)
> Hm, failing a lot of test, will have to look into it. Anyway, is this
> approach fine or not?

Jan, thank you for working on this bug. I feel bad that the bug got slept off our radar; so if it ever happens, then please make sure to draw our attention by setting need-info? (for example to a triage owner).

Also I agree that providing a patch is the best way to get a thing fixed, however sometimes it is a good idea to run your approach through reviewers before working on a patch, just to make sure a chosen approach is optimal.

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #7)
> Having said that, I suppose there's no such a thing as unfocus event aka
> atk_object_notify_state_change(atkObj, ATK_STATE_FOCUSED, false), because if
> something gets unfocused, then something else has to acquire the focus.
> 

Other ATK clients do send a state change event when focused state is unset.

Comment 10

[Joanmarie Diggs]

What Eitan said. :) (i.e. there is indeed such an event)

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Joanmarie Diggs from comment #10)
> What Eitan said. :) (i.e. there is indeed such an event)

If blur then blur, it's totally fine with me :)

on implementation: I would consider to add AccFocusEvent class that has a field of related target which is set to previous unfocused element, since the blur event feels artificial. You'd need to implement a focus event coalescence for it (make sure that a related target is the oldest one), and make some magic for proxies.

Comment 12

[Jan Horak [:jhorak]]

(In reply to alexander :surkov from comment #11)
> (In reply to Joanmarie Diggs from comment #10)
> > What Eitan said. :) (i.e. there is indeed such an event)
> 
> If blur then blur, it's totally fine with me :)
> 
> on implementation: I would consider to add AccFocusEvent class that has a
> field of related target which is set to previous unfocused element, since
> the blur event feels artificial. You'd need to implement a focus event
> coalescence for it (make sure that a related target is the oldest one), and
> make some magic for proxies.

Um, I'll need some assistance there. Is there any similar implementation already? Like what should AccFocusEvent be inherited from. And where are these proxies exactly.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Jan Horak from comment #12)
> Um, I'll need some assistance there. Is there any similar implementation
> already? Like what should AccFocusEvent be inherited from.

this file [1] has number of examples

> And where are
> these proxies exactly.

I was referred to [2], you'd need to add a bunch of things like ProxyFocusEvent/RecvFocusEvent/etc. You may take StateChangeEvent as example.

[1] https://dxr.mozilla.org/mozilla-central/source/accessible/base/AccEvent.h
[2] https://dxr.mozilla.org/mozilla-central/source/accessible/atk/AccessibleWrap.cpp#1490

Comment 14

[Jan Horak [:jhorak]]

(In reply to alexander :surkov from comment #13)
> (In reply to Jan Horak from comment #12)
> > Um, I'll need some assistance there. Is there any similar implementation
> > already? Like what should AccFocusEvent be inherited from.
> 
> this file [1] has number of examples
> 
> > And where are
> > these proxies exactly.
> 
> I was referred to [2], you'd need to add a bunch of things like
> ProxyFocusEvent/RecvFocusEvent/etc. You may take StateChangeEvent as example.
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/accessible/base/AccEvent.h
> [2]
> https://dxr.mozilla.org/mozilla-central/source/accessible/atk/AccessibleWrap.
> cpp#1490

Thanks for the hints, but I'm still quite lost. Just to be sure we're on the same boat:
1. The EVENT_FOCUS is fired everytime user click some element.
2. The atkObj which is used in AccessibleWrap::HandleAccEvent is unique for each element. 
3. If I'd like send "blur event" I will have to call atk_object_notify_state_change(atkObj, ATK_STATE_FOCUSED, false) for the previous atkObj (here I presume [1]) which has received focus before current atkObj (if there was any).

How should I store the previous atkObj? Can't it be deleted before next EVENT_FOCUS?

What comes to my mind is to use some static variable (which is ugly to me), or is there some instrument to notify previous focused atkObj of the change? 

I find out my patch quite straightforward and hopefully harmless comparing to ideas I'm getting from your suggestions.

[1] http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/accessible/atk/AccessibleWrap.cpp#1246

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Jan Horak from comment #14)
> (In reply to alexander :surkov from comment #13)
> > (In reply to Jan Horak from comment #12)
> > > Um, I'll need some assistance there. Is there any similar implementation
> > > already? Like what should AccFocusEvent be inherited from.
> > 
> > this file [1] has number of examples
> > 
> > > And where are
> > > these proxies exactly.
> > 
> > I was referred to [2], you'd need to add a bunch of things like
> > ProxyFocusEvent/RecvFocusEvent/etc. You may take StateChangeEvent as example.
> > 
> > [1] https://dxr.mozilla.org/mozilla-central/source/accessible/base/AccEvent.h
> > [2]
> > https://dxr.mozilla.org/mozilla-central/source/accessible/atk/AccessibleWrap.
> > cpp#1490
> 
> Thanks for the hints, but I'm still quite lost. Just to be sure we're on the
> same boat:
> 1. The EVENT_FOCUS is fired everytime user click some element.
> 2. The atkObj which is used in AccessibleWrap::HandleAccEvent is unique for
> each element. 
> 3. If I'd like send "blur event" I will have to call
> atk_object_notify_state_change(atkObj, ATK_STATE_FOCUSED, false) for the
> previous atkObj (here I presume [1]) which has received focus before current
> atkObj (if there was any).
> 
> How should I store the previous atkObj? Can't it be deleted before next
> EVENT_FOCUS?
> 
> What comes to my mind is to use some static variable (which is ugly to me),
> or is there some instrument to notify previous focused atkObj of the change? 

FocusManager could keep a track of a last focused target, and then DispatchFocusEvent can do the rest.

> I find out my patch quite straightforward and hopefully harmless comparing
> to ideas I'm getting from your suggestions.

Your patch might look indeed simpler, however there's a bunch of concerns:
* as long as you depend on DOM blur event only, then you can't handle non DOM focus changes, for example, if the user navigates the menu items, then you don't reset focused state for unselected menu them.
* extra event type is introduced and thus one more events to process, you're right and it shouldn't be harmful, but technically we could deal with the focus event type only.
* the patch seems has an event coalescence bug, which is eCoalesceOfSameType will keep a latter event I think, while we should keep a first one; that's a case when the focus has changed twice during a single event queue processing.

So, if you could change FocusManager to track a last focused accessible, and fire a blur a11y event from DispatchFocusEvent, then it will resolve the first concern. If you introduce AccFocusEvent class, which keeps both focused and previously focused accessible, instead firing a blur event, then the second concern is resolved as well. Also you could incorporate event coalescence logic into AccFocusEvent, which would resolve 3d concern.

Comment 16

[Jan Horak [:jhorak]]

Attachment: blur-event-for-osk-hide.patch

I'd like to get some feedback on this, if this is the right direction. Thanks.

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 8911736 [details] [diff] [review]
blur-event-for-osk-hide.patch

Review of attachment 8911736 [details] [diff] [review]:
-----------------------------------------------------------------

yeah, it looks good, with the approach taken you don't have to care about event coalescence part, which is nice

Comment 18

[Jan Horak [:jhorak]]

There is one problem with the patch: when switching from the Firefox window with active OSK to the application which does not handle accessibility at all (like konsole from KDE project), the keyboard stays open. This does not happen when switching from GTK3 application to the konsole(ie. keyboard is hidden). That's because only blur event happens (according to the log) and the focus fire does not happen, so atk_object_notify_state_change(atkObj, ATK_STATE_FOCUSED, false) cannot be called for the previous atkObj. Should I leave it that way?

Comment 19

[Jan Horak [:jhorak]]

Comment on attachment 8874852 [details]
Bug 1370496 - Hide on screen keyboard when input lose focus :surkov

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146248/diff/2-3/

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Jan Horak from comment #18)
> There is one problem with the patch: when switching from the Firefox window
> with active OSK to the application which does not handle accessibility at
> all (like konsole from KDE project), the keyboard stays open. This does not
> happen when switching from GTK3 application to the konsole(ie. keyboard is
> hidden). That's because only blur event happens (according to the log) and
> the focus fire does not happen, so atk_object_notify_state_change(atkObj,
> ATK_STATE_FOCUSED, false) cannot be called for the previous atkObj. Should I
> leave it that way?

It feels like related but separate problem, I would file another bug to deal with it. It may be a small fix, btw, all you need is to fire 'unfocus' at window deactivation, see [1]

[1] https://dxr.mozilla.org/mozilla-central/source/accessible/atk/AccessibleWrap.cpp#1424