GNOME Bugzilla – Bug 746670
GNOME Shell for Wayland fails to emit accessible window:activate events
Last modified: 2016-02-19 17:38:24 UTC
Created attachment 300159 [details] accessible-event listener Steps to reproduce: 1. Launch the attached accessible-event listener in a terminal 2. Use Alt+Tab, Ctrl+Alt+Tab, do a search, etc. Expected results: Each time gnome-shell became active, an accessible window:activate event would be printed in the terminal. Actual results: The expected results if you are not using gnome-shell for Wayland. If you are using gnome-shell for Wayland, however, these events are missing from gnome-shell. Impact: Orca is ignoring the subsequent, valid accessible events received from gnome-shell because Orca doesn't realize the user is in gnome-shell and not whatever app he/she just left.
*** Bug 746627 has been marked as a duplicate of this bug. ***
IRC chat (edited): mar 24 12:32:35 <ebassi> infapi00: hey mar 24 12:32:43 <infapi00> ebassi, hou mar 24 12:33:00 <infapi00> well, my ping is about that bug when using wayland mar 24 12:33:16 <infapi00> so it seems that the problem is this: mar 24 12:33:24 <infapi00> https://bugzilla.gnome.org/show_bug.cgi?id=746670 mar 24 12:33:35 <Services> Bug 746670: major, Normal, ---, gnome-shell-maint, NEW , GNOME Shell for Wayland fails to emit accessible window:activate events mar 24 12:33:40 <infapi00> but what I basically do to get those window:activate events mar 24 12:33:42 <infapi00> is forward mar 24 12:33:51 <infapi00> https://developer.gnome.org/clutter/stable/ClutterStage.html#ClutterStage-activate mar 24 12:34:11 <ebassi> infapi00: That's not really going to work for the Stage of the compositor mar 24 12:35:01 <infapi00> hmm mar 24 12:36:40 <infapi00> then not sure why it started to fail now, and now some time ago when we tested mar 24 12:36:57 <infapi00> ebassi, then I guess that in order to know when gnome-shell becames the active app mar 24 12:37:02 <ebassi> infapi00: You could have the shell emit that signal when it's switching between components mar 24 12:37:08 <infapi00> in opposite to gedit mar 24 12:37:22 <infapi00> we should use the shell or mutter mar 24 12:37:30 <infapi00> taking into account your last comment mar 24 12:37:34 <infapi00> your vote is for the shell mar 24 12:37:48 <ebassi> infapi00: I honestly have no idea; maybe one of the event handling refactoring killed that mar 24 12:38:48 <ebassi> in the X11 backend in Clutter, the active state is set/unset in response to FocusIn/FocusOut events mar 24 12:39:08 <ebassi> But I'm not entirely sure those get relayed to the compositor correctly mar 24 12:39:44 <ebassi> If it did work in the past it could be interesting to check why, but it could also be a rabbit hole mar 24 12:39:58 <infapi00> yes probably mar 24 12:40:30 <infapi00> the real question is if clutter can emit again those events on the compositor mar 24 12:40:31 <infapi00> and if not mar 24 12:40:49 <infapi00> forget stage:activate mar 24 12:41:07 <infapi00> forget forward stage:activate mar 24 12:41:13 <infapi00> and emit from the shell/mutter mar 24 12:41:24 <infapi00> probably as you suggested, the shell would be easier mar 24 12:41:47 <infapi00> anyway I guess that we should ask Jasper to confirm that mar 24 12:42:02 <ebassi> Those signals are fairly simple, and I think it's perfectly fine to emit them on the Stage instance from the shell when you key navigate to it mar 24 12:43:14 <infapi00> ebassi, ah so instead of forgetting state:activate mar 24 12:43:21 <infapi00> *stage:activate mar 24 12:43:30 <infapi00> getting them emitted from the shell mar 24 12:43:37 <infapi00> ? mar 24 12:44:50 <ebassi> Yes mar 24 12:44:56 <ebassi> That's one option mar 24 12:45:11 <ebassi> Otherwise, have the shell emit the accessibility events directly mar 24 12:45:14 <halfline> joanie: pong mar 24 12:45:24 <ebassi> I don't really have a good answer for you, I'm afraid :-) mar 24 12:46:14 <infapi00> ebassi, ok, thanks for the info mar 24 12:46:23 <infapi00> well, it seems that Jasper is not connected mar 24 12:46:33 <halfline> joanie: so i pasted the latest comments, and duped the bug mar 24 12:46:38 <ebassi> It's way too early for Jasper mar 24 12:46:56 <infapi00> so I will add this converstgation on the bug mar 24 12:46:58 <infapi00> and wait for him
So, in summary the options are: a) Getting gnome-shell emit stage:activate/deactivate on his Stage instance when gnome-shell becames the active application b) Getting gnome-shell emits the signal window:activate/deactivate on his CallyStage instance when gnome-shell becames the active application As far as I see, both would be really similar. Without thinking too much, Im right now somewhat biased to a) In any case, as said, it would be better to get the opinion of any gnome-shell maintainer.
Created attachment 300210 [details] [review] display: manually activate/deactivate stage when it takes/drops grab clutter currently never emits activated or deactivated signals on the stage object when using the EGL backend. Since the stage never gets activated, accessibility tools, like orca, don't work. This commit makes mutter take on the responsibility.
The above patch fixes things for me, but I feel like it's probably not really "right". I think a proper fix will need some changes in clutter to make _clutter_stage_update_state get called.
Created attachment 300211 [details] [review] display: manually activate/deactivate stage when it takes/drops grab clutter currently never emits activated or deactivated signals on the stage object when using the EGL backend. Since the stage never gets activated, accessibility tools, like orca, don't work. This commit makes mutter take on the responsibility.
The above patch is slightly better. Rather than emitting the signals directly, it uses clutter_stage_event to have clutter emit the signals for us.
Review of attachment 300211 [details] [review]: ::: src/core/display.c @@ +1421,3 @@ + event->stage_state.new_state = CLUTTER_STAGE_STATE_ACTIVATED; + + clutter_stage_event (stage, event); introduced a memleak here
Created attachment 300213 [details] [review] display: manually activate/deactivate stage when it takes/drops grab clutter currently never emits activated or deactivated signals on the stage object when using the EGL backend. Since the stage never gets activated, accessibility tools, like orca, don't work. This commit makes mutter take on the responsibility.
I should note while nicer than attachment 300210 [details] [review] , attachment 300213 [details] [review] still suffers from the same problem I alluded to in comment 5. normally the activation happens from the clutter backend via a call to _clutter_stage_update_state . Since we synthesized the event ourselves instead of calling _clutter_stage_update_state, stage->priv->current_state isn't going to be updated. I think, though, we could fix this in clutter, such that staging the event results in the state getting updated
Created attachment 300214 [details] [review] stage: update current_state from synthesized events Right now current_state can get out of synch with the actual state, if the actual state is poked directly with clutter_stage_event(). This commit makes sure that current_state is always kept up to date.
Review of attachment 300213 [details] [review]: sync_wayland_input_focus() is the right place to do this but doing it with an event like this seems wrong. Note how this can potentially result in several ClutterStage::activate emissions in a row since clutter_stage_event() doesn't check that the state actually changed. _clutter_stage_update_state() is the (internal) function that takes care of this. So, I think, the cleanest solution would be an extra API in clutter's evdev(^W mutter :-) backend that we could call here for it to call _clutter_stage_update_state(). ::: src/core/display.c @@ +1421,3 @@ + event->stage_state.new_state = CLUTTER_STAGE_STATE_ACTIVATED; + + clutter_stage_event (stage, event); event is still leaked here
some random, jumbled thoughts: - this really doesn't have much to do with evdev (or egl for that matter). It just so happens that the other windowing backends have the concept of windows, so they're able to update state implicitly in response to focus changes. Makes me think a backend specific api called by mutter probably isn't the right way to go. I mean maybe if we had access to the ClutterStageWindow it would make sense to use a backend specific api. in that case clutter_stage_eglnative_set_activated () would be the obvious way to go. But I also might not be fully grokking your proposal. - A keypoint, though, is this is all about the stage not evdev or egl. There are two pieces of StateState, in total: "fullscreen" and "activated". The former already has a clutter_stage_set_fullscreen () function. We could add a clutter_stage_set_activated () that on some backends would grab focus, and on egl would merely set state to STATE_ACTIVATED directly. - Another thought, we do have a ClutterStage subclass, MetaStage. So we could update ClutterStage to make the subclass able to control activated state. We could add a "activated" property to ClutterStage that MetaStage could override. activated/deactivated signals could get emitted on notify::activated changes (and the signals could potentially be deprecated in favor of the property) - We could also just sneak a update_state vfunc in the ClutterStage class struct and MetaStage could chain up to set the state.
I also want to say, there's a certain appeal to sticking within the confines of the existing api, since 3.16 / 1.22 is already out. It's certainly easier if mutter and clutter don't have to be upgraded in lockstep, though maybe not the end of the world.
(In reply to Ray Strode [halfline] from comment #14) > I also want to say, there's a certain appeal to sticking within the confines > of the existing api, since 3.16 / 1.22 is already out. It's certainly easier > if mutter and clutter don't have to be upgraded in lockstep, though maybe > not the end of the world. Right, and your patch works, so let's go with it.
(In reply to Rui Matos from comment #15) > Right, and your patch works, so let's go with it. Alright, we could potentially do something more invasive for 3.18, though I'd like to hear ebassi's take on how this should look, too. I've updated the patch a bit to avoid duplicate activate/deactivate signals, will reattach.
Created attachment 300489 [details] [review] wayland: manually activate/deactivate stage when taking/dropping grab clutter currently never emits activated or deactivated signals on the stage object when using the EGL backend. Since the stage never gets activated, accessibility tools, like orca, don't work. This commit makes mutter take on the responsibility.
Comment on attachment 300214 [details] [review] stage: update current_state from synthesized events We don't really need this near-term and medium-term we're probably going to make bigger changes, so marking obsolete.
Review of attachment 300489 [details] [review]: looks good, feel free to ignore the comment below ::: src/backends/meta-stage.c @@ +234,3 @@ + return; + + event = clutter_event_new (CLUTTER_STAGE_STATE); could be allocated on the stack to avoid the malloc
Attachment 300489 [details] pushed as 9f17c05 - wayland: manually activate/deactivate stage when taking/dropping grab
(didn't mean to close this)
Created attachment 300594 [details] [review] meta-stage: To change the stage state we need to set the event type -- Caused by my nit picking, sorry about that
Created attachment 300602 [details] [review] cally: Implement get_top_level_origin() for EGL and GDK backends Also check the windowing backend at runtime otherwise we would crash when running on !X11 backends when accessing the clutter_x11_* API. -- We also need this patch in clutter otherwise we'll crash...
(In reply to Rui Matos from comment #23) > Also check the windowing backend at runtime otherwise we would crash > when running on !X11 backends when accessing the clutter_x11_* API. That part was done a while ago, see http://git.gnome.org/browse/clutter/commit?id=38c4807230ad24.
Review of attachment 300602 [details] [review]: not needed
(In reply to Florian Müllner from comment #24) > That part was done a while ago, see > http://git.gnome.org/browse/clutter/commit?id=38c4807230ad24. That will teach me to update my checkouts more often
(In reply to Rui Matos from comment #25) > Review of attachment 300602 [details] [review] [review]: > > not needed Note that the commit only fixed the runtime check, it didn't actually implement the method for other backends.
Comment on attachment 300594 [details] [review] meta-stage: To change the stage state we need to set the event type Attachment 300594 [details] pushed as 939f7ce - meta-stage: To change the stage state we need to set the event type
Are we planning on including this for 3.16.1 or should we just advise Orca users to not use gnome-shell wayland?
We haven't branched for 3-16 yet, so everything that's currently in master will be included in 3.16.1 ...
Anything left here ?
No, this seems to be working fine.