GNOME Bugzilla – Bug 756371
Accessible event spam in Overview
Last modified: 2016-02-24 12:05:51 UTC
Created attachment 313036 [details] pyatspi accessible-event listener Steps to reproduce: 1. Launch the attached accessible-event listener in a terminal 2. Press Super 3. Sit there and do nothing for a while Expected results: There would not be a continuous flow of object:state-changed:showing events from nameless panels Actual results: I get around 150-300/second object:state-changed:showing events as a result of doing absolutely nothing. While the above is not the typical use case, something in GNOME Shell is being pretty spammy with accessible events in Overview.
Created attachment 313037 [details] result of performing the steps in the opening report Proof that I'm not making this up. ;)
What's new about this bug ? I personally think it's huge bug that makes Gnome hard to use for a blind people. The overview panel is so slow (more than the reasonable 1 second). Could you change the priority to important/urgent ?
(In reply to Joanmarie Diggs (IRC: joanie) from comment #0) > Actual results: I get around 150-300/second object:state-changed:showing > events as a result of doing absolutely nothing. I see it spamming for a bit when I enter the overview, or, after I enter it, if I move the pointer around hovering different widgets. I don't see a continuous spam. In any case, it seems to me that the problem lies in how clutter's a11y layer notifies clutter actor state changes in: https://git.gnome.org/browse/clutter/tree/clutter/cally/cally-actor.c#n997 At least in an application like gnome-shell, that composites many different clutter actors for a single logical widget, this means we'll emit plenty of phony atk object state changes as we show/hide things which happens quite a bit in the overview. I'm not sure how to best fix this without possibly breaking a11y for other clutter applications that might be relying on this behavior though. CCing Emmanuele to get his opinion.
[ Adding Alejandro to the Cc list. ] The idea that Alejandro and I had at the time was that toolkits like Mx and St would subclass CallyActor to implement their own thing; that's also why Cally is part of the public API. Nobody really took time to do that for St, though. Ideally, when the shell or St implements a container it should also override the get_accessible() vfunc, and provide the correct Cally subclass that avoids changing a ton of state. We cannot really do that from Clutter because the API is generic, at that level.
(In reply to Emmanuele Bassi (:ebassi) from comment #4) > [ Adding Alejandro to the Cc list. ] Hi, and thanks. > > The idea that Alejandro and I had at the time was that toolkits like Mx and > St would subclass CallyActor to implement their own thing; that's also why > Cally is part of the public API. Nobody really took time to do that for St, > though. Well, in fact, I did that a long time ago. It is part of the current accessibility support for gnome-shell. Probably you didn't find it because in opposite to clutter, that a11y support are on different source files, on St are on the same source files. For example, StWidgetAccessible is at st-widget.c: https://git.gnome.org/browse/gnome-shell/tree/src/st/st-widget.c#n2510 > Ideally, when the shell or St implements a container it should also override > the get_accessible() vfunc, and provide the correct Cally subclass that > avoids changing a ton of state. get_accessible is already overriden at StWidget: https://git.gnome.org/browse/gnome-shell/tree/src/st/st-widget.c#n2548 > We cannot really do that from Clutter because the API is generic, at that > level. And even if we need some extra API from Clutter, most of the work needs to be done at gnome-shell. And now answering Rui Matos: (In reply to Rui Matos from comment #3) > (In reply to Joanmarie Diggs (IRC: joanie) from comment #0) > At least in an application like gnome-shell, that composites many different > clutter actors for a single logical widget, this means we'll emit plenty of > phony atk object state changes as we show/hide things which happens quite a > bit in the overview. Yes, the a11y support of gnome-shell is creating an object for all clutter objects, even if they are not UI objects, so most of the a11y tree are in fact irrelevant to a11y needs. One of my TODOs was getting a real accessible tree. But, when the support was added, this was just "an issue", so the priority was on getting everything needed implemented. So now that the situation is a problem, a braindump of what I thought on the moment: other toolkits (specifically Mac accessibility) mark explicitly which objects are part of the accessibility tree. Take a look to diagrams 3-2 and 3-3 here: https://developer.apple.com/library/mac/documentation/Accessibility/Conceptual/AccessibilityMacOSX/OSXAXmodel.html#//apple_ref/doc/uid/TP40001078-CH208-TPXREF101 So what I wanted to implement on St was add a new boolean to the a11y stuff that StWidget already have: https://git.gnome.org/browse/gnome-shell/tree/src/st/st-widget.c#n76 Something like "bool ignore_accessibility". So if for an object, ignore_accessibility is set, it would be ignored, and not included on the accessibility tree. My doubts was how to implement it taking into account the grandchildren, and how to ensure that the accessibility objects are not created when calling the container-like methods of AtkObject. But as I mentioned, I didn't think too much on that, I just knew that I wanted "what cocoa already have" and a early draft on mind. > I'm not sure how to best fix this without possibly breaking a11y for other > clutter applications that might be relying on this behavior though. It is possible that some support would be needed on Clutter, but most work would be needed to be done on StWidget (so gnome-shell).
(In reply to Alejandro Piñeiro Iglesias (IRC: infapi00) from comment #5) > Well, in fact, I did that a long time ago. It is part of the current > accessibility support for gnome-shell. Probably you didn't find it because > in opposite to clutter, that a11y support are on different source files, on > St are on the same source files. For example, StWidgetAccessible is at > st-widget.c: > > https://git.gnome.org/browse/gnome-shell/tree/src/st/st-widget.c#n2510 Sure, but what I meant was: every time there is a complex container, with multiple moving parts, inside the Shell and it subclasses from ClutterActor or from a StWidget, then the Shell should also implement the corresponding accessible object, instead of relying on the default implementation. > > We cannot really do that from Clutter because the API is generic, at that > > level. > > And even if we need some extra API from Clutter, most of the work needs to > be done at gnome-shell. Indeed. [ … ] The idea of API entry points like: /* TRUE if the actor class has an accessible state that can change */ clutter_actor_class_set_has_accessible_state (class, boolean); /* Equivalent to the GtkWidget methods */ clutter_actor_class_set_accessible_role (class, role); clutter_actor_class_set_accessible_type (class, gtype); for subclasses of ClutterActor would be fine by me.
(In reply to Emmanuele Bassi (:ebassi) from comment #6) > (In reply to Alejandro Piñeiro Iglesias (IRC: infapi00) from comment #5) > > > Well, in fact, I did that a long time ago. It is part of the current > > accessibility support for gnome-shell. Probably you didn't find it because > > in opposite to clutter, that a11y support are on different source files, on > > St are on the same source files. For example, StWidgetAccessible is at > > st-widget.c: > > > > https://git.gnome.org/browse/gnome-shell/tree/src/st/st-widget.c#n2510 > > Sure, but what I meant was: every time there is a complex container, with > multiple moving parts, inside the Shell and it subclasses from ClutterActor > or from a StWidget, then the Shell should also implement the corresponding > accessible object, instead of relying on the default implementation. Ah ok, sorry for the misunderstanding. Thanks for the explanation. > > > We cannot really do that from Clutter because the API is generic, at that > > > level. > > > > And even if we need some extra API from Clutter, most of the work needs to > > be done at gnome-shell. > > Indeed. > > [ … ] > > The idea of API entry points like: > > /* TRUE if the actor class has an accessible state that can change */ > clutter_actor_class_set_has_accessible_state (class, boolean); > > /* Equivalent to the GtkWidget methods */ > clutter_actor_class_set_accessible_role (class, role); > clutter_actor_class_set_accessible_type (class, gtype); > > for subclasses of ClutterActor would be fine by me. Thanks for the offering. But having said so, most of the UI objects on gnome-shell are StWidgets (well, at least they were, correct me if Im wrong). So as I mentioned, I think that it would be better to try to deal with this issue on gnome-shell, and scalate to clutter only if needed.
(In reply to Rui Matos from comment #3) > (In reply to Joanmarie Diggs (IRC: joanie) from comment #0) > > Actual results: I get around 150-300/second object:state-changed:showing > > events as a result of doing absolutely nothing. > > I see it spamming for a bit when I enter the overview, or, after I enter it, > if I move the pointer around hovering different widgets. I don't see a > continuous spam. I was wrong here. I do see continuous spam if I run the script on a terminal window (instead of through ssh). In fact I see continuous spam if any thumbnailed window is updating its contents while the overview is visible. This particular issue comes from clutter_clone_paint() which temporarily maps the cloned actor to paint it and immediately unmaps it again. Obviously this causes all the ATK_STATE_SHOWING spam.
We should avoid changing the ATK state if the actor was mapped by a clone, since the clone should be the one authoritative source for the ATK object.
Created attachment 321983 [details] [review] cally: Avoid clone spamming state changes Clones may generate a temporary map/unmap on their source when painting; this, in turn, will generate unnecessary ATK state changes.
Review of attachment 321983 [details] [review]: ::: clutter/cally/cally-actor.c @@ +1024,3 @@ + if (clutter_actor_is_in_clone_paint (actor)) + return; + The comment seems to suggest under that condition it shouldn't emit any state change at all. So why not moving the if to the beginning of the _real_notify_clutter method and filter all events, and not just ATK_STATE_SHOWING?
(In reply to Emmanuele Bassi (:ebassi) from comment #10) > Created attachment 321983 [details] [review] [review] > cally: Avoid clone spamming state changes > > Clones may generate a temporary map/unmap on their source when painting; > this, in turn, will generate unnecessary ATK state changes. This doesn't seem to be enough because the cloned actor might have children which themselves will be temporarily mapped but aren't marked with in_clone_paint. Perhaps they should be marked to begin with?
(In reply to Alejandro Piñeiro Iglesias (IRC: infapi00) from comment #11) > Review of attachment 321983 [details] [review] [review]: > > ::: clutter/cally/cally-actor.c > @@ +1024,3 @@ > + if (clutter_actor_is_in_clone_paint (actor)) > + return; > + > > The comment seems to suggest under that condition it shouldn't emit any > state change at all. So why not moving the if to the beginning of the > _real_notify_clutter method and filter all events, and not just > ATK_STATE_SHOWING? I'd rather keep the check near its use. The other conditions are: * visible: this is not triggered by the Clone paint, as it's something that can only be done by the developer * reactive: this is not triggered by the Clone paint, as it's something that can only be done by the developer, and it's only related to event handling
(In reply to Rui Matos from comment #12) > (In reply to Emmanuele Bassi (:ebassi) from comment #10) > > Created attachment 321983 [details] [review] [review] [review] > > cally: Avoid clone spamming state changes > > > > Clones may generate a temporary map/unmap on their source when painting; > > this, in turn, will generate unnecessary ATK state changes. > > This doesn't seem to be enough because the cloned actor might have children > which themselves will be temporarily mapped but aren't marked with > in_clone_paint. Perhaps they should be marked to begin with? True; is_in_clone_paint() should also check for a sub-graph of the scene being cloned. We have a in_cloned_branch value that we propagate to children, so is_in_clone_paint() should: * check if in_clone_paint is true * recurse upwards until parent.in_cloned_branch == 0: * check if in_clone_paint is true I'll prepare a patch.
(In reply to Emmanuele Bassi (:ebassi) from comment #13) > (In reply to Alejandro Piñeiro Iglesias (IRC: infapi00) from comment #11) > > Review of attachment 321983 [details] [review] [review] [review]: > > > > ::: clutter/cally/cally-actor.c > > @@ +1024,3 @@ > > + if (clutter_actor_is_in_clone_paint (actor)) > > + return; > > + > > > > The comment seems to suggest under that condition it shouldn't emit any > > state change at all. So why not moving the if to the beginning of the > > _real_notify_clutter method and filter all events, and not just > > ATK_STATE_SHOWING? > > I'd rather keep the check near its use. > > The other conditions are: > > * visible: this is not triggered by the Clone paint, as it's something that > can only be done by the developer > * reactive: this is not triggered by the Clone paint, as it's something that > can only be done by the developer, and it's only related to > event handling Ok.
Created attachment 322009 [details] [review] actor: Expand is_in_clone_paint() check The function should return true not only if the actor is being painted by a ClutterClone, but also if it's inside a sub-graph being painted by a ClutterClone.
With both patches, the event spam seems fixed now.
Attachment 321983 [details] pushed as 2b0ff2f - cally: Avoid clone spamming state changes Attachment 322009 [details] pushed as c867e9d - actor: Expand is_in_clone_paint() check Feel free to re-open the bug if there are other issues.