GNOME Bugzilla – Bug 668366
Required a way to add accessible states to a generic container
Last modified: 2012-03-15 19:08:28 UTC
At this moment, any AT can ask to an accessible about the state of an object (if active, focused, checked, etc). The accessible object creates a state set based on the API provided by the object. At this moment the accessibility support on gnome shell are based on expose the information from clutter actors. ShellGenericContainer is a workaround on gnome shell for the lack of GObject subclassing. That means that in several objects at the ui, custom objects are created based in delegation on the js code. That generic container provides some signals and methods in order to set the custom size (and others) on run-time. For the same reason, it would be required to add a generic way to provide accessible information. Specifically for this bug: a generic way to set accessible states. Example: check menu items on the popupmenu. That menu item is a generic container. Javascript code creates a generic container, and add some children (one label and a switcher). Then javascript code changes the state, changing the object style and so on. So the accessibility code at the clutter/st side can't access that information. Unless a generic way to add information is provided.
Created attachment 205719 [details] [review] Adds add_accessible_state/remove_state to shell-generic container First working solution. Adds a AtkStateSet to the generic container, and add public methods to add/remove the states. Anyway, there is something that I don't like to this patch. The accessible state change notification is done at those calls, asking for the accessible. That means that you always ask and modify the accessible object, no matter if the accessibility is on, or if the accessible is required at that moment (I'm right now at the hackfest, and one of the main topics is about not create accessible objects if not required). One solution could be add a signal to ShellGenericContainer for a state change, so the accessible object could connect to that signal. Anyway, I'm uploading this patch as a example of a working solution, and because I think that the controversial part of this patch are the modification of the API of that GenericContainer.
Created attachment 205721 [details] [review] Use new API on the ui code to add CHECKED state
Review of attachment 205719 [details] [review]: Various comments, mostly trivial ::: src/shell-generic-container.c @@ +20,3 @@ +#define ST_H_INSIDE 1 +#include <st/st-widget-accessible.h> +#undef ST_H_INSIDE Not OK, if something is needed out of ST, it should be in a public header @@ +39,3 @@ GHashTable *skip_paint; + + /* accessibility support see bug XXX */ Fix bug # @@ +254,3 @@ g_hash_table_destroy (self->priv->skip_paint); + if (self->priv->local_state_set != NULL) It can't be NULL @@ +402,3 @@ + * + * Ensure that the accessible object related to this object has @state + * as one of his #AtkState. It also notifies a state change if required Adds @state to the set of #AtkState for the container, if it is not already present. - The programmer doesn't care that there is an accessible object, basically an implementation detail - If state changes have notification than all code that changefs them has to notify them @@ +411,3 @@ + g_return_if_fail (SHELL_IS_GENERIC_CONTAINER (self)); + + if (!atk_state_set_contains_state (self->priv->local_state_set, state)) You don't need this check @@ +429,3 @@ + * Ensure that the accessible object related to this object doesn't + * have @state as one of his #AtkStateType. It also notifies a state + * change if required Removes @state from the set of #AtkState for the container, if present. @@ +438,3 @@ + g_return_if_fail (SHELL_IS_GENERIC_CONTAINER (self)); + + if (atk_state_set_contains_state (self->priv->local_state_set, state)) You don't need this check @@ +453,3 @@ +#define SHELL_GENERIC_CONTAINER_ACCESSIBLE(obj) \ + (G_TYPE_CHECK_INSTANCE_CAST ((obj), \ + SHELL_TYPE_GENERIC_CONTAINER_ACCESSIBLE, ShellGenericContainerAccessible)) Should conform to the style we use elsewhere and not be line-broken in an unusual way @@ +485,3 @@ + +static void shell_generic_container_accessible_class_init (ShellGenericContainerAccessibleClass *klass); +static void shell_generic_container_accessible_init (ShellGenericContainerAccessible *generic_container); This prototypes are included in G_DEFINE_TYPE @@ +490,3 @@ +static void shell_generic_container_accessible_initialize (AtkObject *obj, + gpointer data); +static AtkStateSet *shell_generic_container_accessible_ref_state_set (AtkObject *obj); General practice these days is to just order your definitions to avoid the need to have forward declarations @@ +520,3 @@ +/** + * + * We basically add the local states to the global state_set why "basically"? @@ +534,3 @@ + container = SHELL_GENERIC_CONTAINER (atk_gobject_accessible_get_object + (ATK_GOBJECT_ACCESSIBLE (obj))); + if (container == NULL) /* state defunct */ state defunct? @@ +541,3 @@ + + if (result == NULL) + result = parent_set; Is this the case where local_state_set is empty and parent_state_set is empty but not NULL? Can parent_set be NULL? ::: src/shell-generic-container.h @@ +52,3 @@ +/* accessibility support */ +void shell_generic_container_add_accessible_state (ShellGenericContainer *self, + AtkStateType state); parameter alignment (and below)
Review of attachment 205721 [details] [review]: Looks good contigent on the other patch
Created attachment 206635 [details] [review] Add a way to add accessible states to StWidget Fixed comments from previous revision. I also moved this to StWidget, as suggested by Dan Winship on IRC.
Created attachment 206636 [details] [review] Adding checked state on the popup menu items Previous patch only managed the state on the toggle function. But the checked value can be also set when the object is created or by setToggleState.
Created attachment 208503 [details] [review] Add a way to add accessible states to StWidget Due last changes on gnome-shell previous patch didn't apply with just an "git am"
Comment on attachment 208503 [details] [review] Add a way to add accessible states to StWidget Appears to address most of the comments from Owen's earlier review. >+ ACCESSIBLE_STATE_CHANGE, Is there any reason anyone other than the StWidgetAccessible implementation would ever want to connect to that signal? If not, it would make more sense to just have StWidget call a function on its accessible, rather than emitting a signal for the accessible to catch.
Comment on attachment 206636 [details] [review] Adding checked state on the popup menu items You need to deal with PopupSwitchMenuItem.setStatus() as well; when the switch has a textual status set, it doesn't matter what the switch setting is. (eg, if you have a wired device with no ethernet cable, it will show "cable unplugged" where the switch should be)
Created attachment 208650 [details] [review] Add a way to add accessible states to StWidget Removed the signal ACCESSIBLE_STATE_CHANGE as suggested on comment 9
(In reply to comment #9) > (From update of attachment 206636 [details] [review]) > You need to deal with PopupSwitchMenuItem.setStatus() as well; when the switch > has a textual status set, it doesn't matter what the switch setting is. (eg, if > you have a wired device with no ethernet cable, it will show "cable unplugged" > where the switch should be) True, I didn't realize that. But this also affects the patch related with the role that I uploaded on bug 667432 (this bug depends on this, also still being reviewed). This patch set the role to CHECK_MENU_ITEM. If the switch has a textual status set, it is not a CHECK_MENU_ITEM, but "just" a MENU_ITEM. I will update this patch as soon as I update the patch on the other bug.
Created attachment 208678 [details] [review] Adding checked state on the popup menu items Updated patch, based on comment 9 and comment 11. Addons on this patch: * Calling checkAccessibleState also in PopupSwitchMenuItem.setStatus() * checkAccessibleState takes into account the current role of the item (MENU_ITEM or CHECK_MENU_ITEM). Cleans the state set if it is a MENU_ITEM
Created attachment 209023 [details] [review] Add a way to add accessible states to StWidget Minor update: Setting a proper param style to all accessibility related methods on st-widget.h
Comment on attachment 208678 [details] [review] Adding checked state on the popup menu items >+ this.checkAccessibleState(); > this.actor.accessible_role = Atk.Role.CHECK_MENU_ITEM; you need to set accessible_role before calling checkAccessibleState, right?
Comment on attachment 209023 [details] [review] Add a way to add accessible states to StWidget looks good
Created attachment 209369 [details] [review] Adding checked state on the popup menu items (In reply to comment #14) > (From update of attachment 208678 [details] [review]) > >+ this.checkAccessibleState(); > > this.actor.accessible_role = Atk.Role.CHECK_MENU_ITEM; > > you need to set accessible_role before calling checkAccessibleState, right? Yes, updated patch.
Created attachment 209370 [details] [review] Adding checked state on the Activities button As Activities buttton is now a toggle button (see bug 667432 comment 17) it is also required to set when it is checked or not. In this case it is checked if the overview is being shown, and not checked if hide. So I used the overview showing/hiding signals to add/remove that state.