After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 668366 - Required a way to add accessible states to a generic container
Required a way to add accessible states to a generic container
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on: 667432
Blocks: 670719
 
 
Reported: 2012-01-20 18:35 UTC by Alejandro Piñeiro Iglesias (IRC: infapi00)
Modified: 2012-03-15 19:08 UTC
See Also:
GNOME target: 3.4
GNOME version: 3.3/3.4


Attachments
Adds add_accessible_state/remove_state to shell-generic container (8.93 KB, patch)
2012-01-20 18:47 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
needs-work Details | Review
Use new API on the ui code to add CHECKED state (875 bytes, patch)
2012-01-20 18:47 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
accepted-commit_now Details | Review
Add a way to add accessible states to StWidget (6.87 KB, patch)
2012-02-02 13:57 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Adding checked state on the popup menu items (1.50 KB, patch)
2012-02-02 13:58 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
needs-work Details | Review
Add a way to add accessible states to StWidget (6.87 KB, patch)
2012-02-27 16:45 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review
Add a way to add accessible states to StWidget (5.56 KB, patch)
2012-02-29 00:10 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Adding checked state on the popup menu items (1.99 KB, patch)
2012-02-29 13:02 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review
Add a way to add accessible states to StWidget (5.88 KB, patch)
2012-03-05 19:18 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
committed Details | Review
Adding checked state on the popup menu items (1.97 KB, patch)
2012-03-10 00:35 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
committed Details | Review
Adding checked state on the Activities button (1.03 KB, patch)
2012-03-10 01:09 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
committed Details | Review

Description Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-01-20 18:35:32 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.
Comment 1 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-01-20 18:47:12 UTC
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.
Comment 2 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-01-20 18:47:55 UTC
Created attachment 205721 [details] [review]
Use new API on the ui code to add CHECKED state
Comment 3 Owen Taylor 2012-01-25 00:17:20 UTC
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)
Comment 4 Owen Taylor 2012-01-25 17:35:19 UTC
Review of attachment 205721 [details] [review]:

Looks good contigent on the other patch
Comment 5 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-02-02 13:57:09 UTC
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.
Comment 6 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-02-02 13:58:54 UTC
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.
Comment 7 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-02-27 16:45:40 UTC
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 8 Dan Winship 2012-02-28 17:50:11 UTC
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 9 Dan Winship 2012-02-28 17:55:58 UTC
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)
Comment 10 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-02-29 00:10:41 UTC
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
Comment 11 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-02-29 00:17:34 UTC
(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.
Comment 12 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-02-29 13:02:26 UTC
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
Comment 13 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-03-05 19:18:34 UTC
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 14 Dan Winship 2012-03-09 15:23:59 UTC
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 15 Dan Winship 2012-03-09 15:25:03 UTC
Comment on attachment 209023 [details] [review]
Add a way to add accessible states to StWidget

looks good
Comment 16 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-03-10 00:35:46 UTC
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.
Comment 17 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-03-10 01:09:02 UTC
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.