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 636717 - [PATCH] Required accessibility support for StWidget
[PATCH] Required accessibility support for StWidget
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on:
Blocks: 626658 634016 640057
 
 
Reported: 2010-12-07 18:31 UTC by Alejandro Piñeiro Iglesias (IRC: infapi00)
Modified: 2011-01-20 14:55 UTC
See Also:
GNOME target: 3.0
GNOME version: ---


Attachments
New object StWidgetAccessible (12.74 KB, patch)
2010-12-07 18:34 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review
Expose the tooltip as the accessible description (1.99 KB, patch)
2010-12-07 18:35 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review
Updated patch after Dan Winship review (13.93 KB, patch)
2010-12-29 16:09 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Update: rebased patch (13.94 KB, patch)
2011-01-04 12:35 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review
Updated patch after review on comment 12 (12.54 KB, patch)
2011-01-11 15:24 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
accepted-commit_now Details | Review
Updated patch after review on comment 15 (12.15 KB, patch)
2011-01-20 12:22 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review

Description Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-07 18:31:02 UTC
StWidget has some specific information that it would be required to be exposed to the accessibility framework.

Some examples:

 * A proper focusable state
 * selected state
 * the tooltip_text as a description, etc

This bug is about create the basic StWidget accessibility (ATK) object, that could be used as the base for the rest of the St widgets.
Comment 1 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-07 18:34:36 UTC
Created attachment 176021 [details] [review]
New object StWidgetAccessible

This adds a new object called StWidgetAccessible. It also redefines StWidget->get_accessible in order to return a accessible object of this type.

The tricky part is how computes that a StWidget is selected. As explained on bug 634016 comment 5, I tried to use the StWidget pseudo_class. But in some cases it is used style-class. In the same way, not always the same name ("selected") is used. So the heuristic is:
  * Object is selected if the word "selected" is included on the "style-class" or the "pseudo-class" property.

Something that is somewhat hackish.
Comment 2 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-07 18:35:56 UTC
Created attachment 176022 [details] [review]
Expose the tooltip as the accessible description

AtkObject allow to add a description. If no one is available, on a StWidget the tooltip_text is used.
Comment 3 Dan Winship 2010-12-21 22:27:25 UTC
Comment on attachment 176021 [details] [review]
New object StWidgetAccessible

>+static AtkObject *st_widget_get_accessible (ClutterActor *actor)

(style nit: newline between "*" and "st_..."

>+  accessible = g_object_get_qdata (G_OBJECT (actor), quark_accessible_object);
>+
>+  if (accessible == NULL)
>+    {
>+      accessible = st_widget_accessible_new (actor);
>+      g_object_set_qdata (G_OBJECT (actor), quark_accessible_object,
>+                          accessible);

I don't remember why you had to do it this way, but IIRC, we came to a conclusion involving adding a virtual method to StWidget that would make the qdata stuff unnecessary.

>+  /* Check the cached selected state and notify the first
>+     selection. Ie: it is required to ensure a first notification
>+     when Alt+Tab popup appears */

another style nit: make the comments match the comment style in the rest of St. Eg:

  /* Check the cached selected state and notify the first
   * selection. Ie: it is required to ensure a first notification
   * when Alt+Tab popup appears
   */

>+  if (widget->priv->can_focus)
>+    atk_state_set_add_state (result, ATK_STATE_FOCUSABLE);
>+  else
>+    atk_state_set_remove_state (result, ATK_STATE_FOCUSABLE);

Do we need to do something from st_widget_set_can_focus() too? (Or connect to notify:can-focus?)

>+ * In a ideal world we would have a more standard way to check if the
>+ * item is selected or not, like the widget-context (as in the case of
>+ * gtktreeview-cells), or something like the property "can-focus".
>+ **/
>+static void
>+_check_string_for_selected_notification (StWidgetAccessible *self,
>+                                         const gchar *string)

Right. The fix here is, we should be consistent about using a "selected" pseudo class, and anywhere that isn't doing it that way should just be fixed. Then StWidgetAccessible can just check st_widget_has_style_pseudo_class("selected").
Comment 4 Dan Winship 2010-12-21 22:29:52 UTC
Comment on attachment 176022 [details] [review]
Expose the tooltip as the accessible description

>+static G_CONST_RETURN gchar* st_widget_accessible_get_description (AtkObject *accessible);

G_CONST_RETURN is only for headers, and only in modules that already use it. Just say "const".

However, StTooltip is broken in several different ways right now, and we currently aren't using it *anywhere*. So this isn't a good way to get the description...

We can add a11y-related properties (description, role, etc) directly to StWidget though.
Comment 5 Dan Winship 2010-12-22 20:03:02 UTC
filed bug 637830 with a patch to use 'selected' pseudo-class for alt-tab dialog
Comment 6 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-23 09:44:30 UTC
(In reply to comment #3)
> (From update of attachment 176021 [details] [review])
> >+static AtkObject *st_widget_get_accessible (ClutterActor *actor)
> 
> (style nit: newline between "*" and "st_..."

Ok, sorry, C&P from the declaration.

> >+  accessible = g_object_get_qdata (G_OBJECT (actor), quark_accessible_object);
> >+
> >+  if (accessible == NULL)
> >+    {
> >+      accessible = st_widget_accessible_new (actor);
> >+      g_object_set_qdata (G_OBJECT (actor), quark_accessible_object,
> >+                          accessible);
> 
> I don't remember why you had to do it this way, but IIRC, we came to a
> conclusion involving adding a virtual method to StWidget that would make the
> qdata stuff unnecessary.

Well, the purpose of this qdata was to avoid a private variable on both StLabel and StWidget.

But as you are saying, adding that virtual method (that was proposed to avoid define a mostly equal ->get_accessible on each object) would allow us to just have a private variable on StWidget.

Anyway, it would still makes sense to do that, as for the moment CallyActor is still an AtkObjectAccessible [1], and it uses set_qdata to set the accessible object on the object.

> >+  /* Check the cached selected state and notify the first
> >+     selection. Ie: it is required to ensure a first notification
> >+     when Alt+Tab popup appears */
> 
> another style nit: make the comments match the comment style in the rest of St.
> Eg:
> 
>   /* Check the cached selected state and notify the first
>    * selection. Ie: it is required to ensure a first notification
>    * when Alt+Tab popup appears
>    */

Ok

> >+  if (widget->priv->can_focus)
> >+    atk_state_set_add_state (result, ATK_STATE_FOCUSABLE);
> >+  else
> >+    atk_state_set_remove_state (result, ATK_STATE_FOCUSABLE);
> 
> Do we need to do something from st_widget_set_can_focus() too? (Or connect to
> notify:can-focus?)

Good catch. Anyway I think that we shouldn't do anything extra on st_widget_set_can_focus, and just connect to notify:can-focus to notify the state change, as in other cases.

In fact I have just checked GAIL, and it also don't notify this state change. I guess that this means that it is not a really important issue, but anyway I think that it is worth to create a bug with that.

> >+ * In a ideal world we would have a more standard way to check if the
> >+ * item is selected or not, like the widget-context (as in the case of
> >+ * gtktreeview-cells), or something like the property "can-focus".
> >+ **/
> >+static void
> >+_check_string_for_selected_notification (StWidgetAccessible *self,
> >+                                         const gchar *string)
> 
> Right. The fix here is, we should be consistent about using a "selected" pseudo
> class, and anywhere that isn't doing it that way should just be fixed. Then
> StWidgetAccessible can just check st_widget_has_style_pseudo_class("selected").

So I should update the patch adding a dependency to bug 637830 or should I maintain that in that way until gnome shell became homogeneous?

Thanks for the review.

It will be complex to update the patch this week, I will do that on the next week.


[1] http://bugzilla.clutter-project.org/show_bug.cgi?id=2371
Comment 7 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-23 09:45:14 UTC
(In reply to comment #4)
> (From update of attachment 176022 [details] [review])
> >+static G_CONST_RETURN gchar* st_widget_accessible_get_description (AtkObject *accessible);
> 
> G_CONST_RETURN is only for headers, and only in modules that already use it.
> Just say "const".
> 
> However, StTooltip is broken in several different ways right now, and we
> currently aren't using it *anywhere*. So this isn't a good way to get the
> description...

Ok, I just thought that the tooltip was the obvious default description for a StWidget. So we can forget this patch.

Anyway, part of the gtk wisdom to make an app accessible is use tooltips as far as possible, although ...

> We can add a11y-related properties (description, role, etc) directly to
> StWidget though.

... it is also stated that in the case that you can't you can use atk [1] to do that.

Anyway it is clear that having that information on the StWidget would make the ui code cleaner (not so many atk calls, although some apps do that).

[1] http://library.gnome.org/devel/accessibility-devel-guide/nightly/gad-coding-guidelines.html.en
Comment 8 Dan Winship 2010-12-23 14:52:27 UTC
(In reply to comment #6)
> So I should update the patch adding a dependency to bug 637830 or should I
> maintain that in that way until gnome shell became homogeneous?

637830 is committed
Comment 9 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-23 16:09:40 UTC
(In reply to comment #8)
> (In reply to comment #6)
> > So I should update the patch adding a dependency to bug 637830 or should I
> > maintain that in that way until gnome shell became homogeneous?
> 
> 637830 is committed

Ok, in the following update of this patch (sorry, procrastinated to next week) I would also simplify the ad-hoc selection check, to just search for "selected" on the pseudo-class.

Thanks
Comment 10 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-12-29 16:09:26 UTC
Created attachment 177196 [details] [review]
Updated patch after Dan Winship review

Patch updated with all Dan Winship comments. Some extra stuff:

  * The virtual method commented is called ->get_accessible_type
  * Although it would be good to maintain the "accessible-object" qdata to keep the coherence with AtkGobjectAccessible, I just added a new private element.
  * I also removed st_widget_accessible_new as it is not used anywhere
  * I also set as obsolete the tooltip-as-description patch, as we concluded that use right now the tooltip is a bad idea, and if required, a new property can be added
Comment 11 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-04 12:35:54 UTC
Created attachment 177473 [details] [review]
Update: rebased patch
Comment 12 Dan Winship 2011-01-06 15:52:58 UTC
Comment on attachment 177473 [details] [review]
Update: rebased patch

>+  /* < private > */
>+  StWidgetAccessiblePrivate *priv;

Pretty sure it has to be "/*< private >*/" (no spaces between *s and <>s) for gtk-doc/g-i-scanner to recognize it.

>+  /* The real dispose of this accessible is done on AtkGobject weak
>+   * ref callback*/

Nitpickiness: the "*/" should be on a separate line. (Here and in all other multi-line comments.)

>+GType
>+st_widget_get_accessible_type (StWidget *widget)
>+{
>+  g_return_val_if_fail (ST_IS_WIDGET (widget), 0);
>+
>+  return ST_WIDGET_GET_CLASS (widget)->get_accessible_type (widget);
>+}

I don't think you need to define this as a visible method; just have st_widget_get_accessible() call ST_WIDGET_GET_CLASS(widget)->get_accessible_type() directly.

>+  GObjectClass   *gobject_class = G_OBJECT_CLASS (klass);
>+  AtkObjectClass *class         = ATK_OBJECT_CLASS (klass);
>+  CallyActorClass *cally_class  = CALLY_ACTOR_CLASS (klass);

if those are going to be almost-lined-up, they should be completely-lined-up.

>+_check_selected (StWidgetAccessible *self,

lose the initial "_"

>+  if (found != self->priv->selected)
>+    {
>+      atk_object_notify_state_change (ATK_OBJECT (self),
>+                                      ATK_STATE_SELECTED,
>+                                      found);
>+
>+      self->priv->selected = found;

probably best to update self->priv->selected first, in case the notification triggers something that causes another state change. (Hopefully that's not supposed to happen any more with dbus-based a11y, but...)

>+  GType    (*get_accessible_type)   (StWidget *widget);

If you made this

    GType    (*get_accessible_type)   (void);

instead, then it would simplify implementations. Eg, st_widget_class_init could just do

    klass->get_accessible_type = st_widget_accessible_get_type;

without needing to define st_widget_real_get_accessible_type() at all.
Comment 13 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-11 15:24:05 UTC
Created attachment 178042 [details] [review]
Updated patch after review on comment 12

(In reply to comment #12)
> (From update of attachment 177473 [details] [review])
> >+  /* < private > */
> >+  StWidgetAccessiblePrivate *priv;
> 
> Pretty sure it has to be "/*< private >*/" (no spaces between *s and <>s) for
> gtk-doc/g-i-scanner to recognize it.

Ok, done.

> >+  /* The real dispose of this accessible is done on AtkGobject weak
> >+   * ref callback*/
> 
> Nitpickiness: the "*/" should be on a separate line. (Here and in all other
> multi-line comments.)

Done. Nitpickiness over nitpickiness:the comment on source just 5 lines before has the same wrong format ;)

> >+GType
> >+st_widget_get_accessible_type (StWidget *widget)
> >+{
> >+  g_return_val_if_fail (ST_IS_WIDGET (widget), 0);
> >+
> >+  return ST_WIDGET_GET_CLASS (widget)->get_accessible_type (widget);
> >+}
> 
> I don't think you need to define this as a visible method; just have
> st_widget_get_accessible() call
> ST_WIDGET_GET_CLASS(widget)->get_accessible_type() directly.

Ok, done.

> >+  GObjectClass   *gobject_class = G_OBJECT_CLASS (klass);
> >+  AtkObjectClass *class         = ATK_OBJECT_CLASS (klass);
> >+  CallyActorClass *cally_class  = CALLY_ACTOR_CLASS (klass);
> 
> if those are going to be almost-lined-up, they should be completely-lined-up.

So lets not lined-up them, so if in the future we need to add a longer class name, we will not required to change them all.

> >+_check_selected (StWidgetAccessible *self,
> 
> lose the initial "_"

Ok, done.

> >+  if (found != self->priv->selected)
> >+    {
> >+      atk_object_notify_state_change (ATK_OBJECT (self),
> >+                                      ATK_STATE_SELECTED,
> >+                                      found);
> >+
> >+      self->priv->selected = found;
> 
> probably best to update self->priv->selected first, in case the notification
> triggers something that causes another state change. (Hopefully that's not
> supposed to happen any more with dbus-based a11y, but...)

Ok, done.

> >+  GType    (*get_accessible_type)   (StWidget *widget);
> 
> If you made this
> 
>     GType    (*get_accessible_type)   (void);
> 
> instead, then it would simplify implementations. Eg, st_widget_class_init could
> just do
> 
>     klass->get_accessible_type = st_widget_accessible_get_type;
> 
> without needing to define st_widget_real_get_accessible_type() at all.

Ok, done.
Comment 14 André Klapper 2011-01-16 22:23:56 UTC
Setting GNOME Target field to 3.0 as per Piñeiro's post at https://mail.gnome.org/archives/gnome-accessibility-list/2011-January/msg00046.html
Comment 15 Dan Winship 2011-01-18 15:33:55 UTC
Comment on attachment 178042 [details] [review]
Updated patch after review on comment 12

also basically good

> * Management of the selected stated, using the current pseudo_class.
>   It is somewhat hacky, but the only option right now.

IMHO we have decreed that this is *not* a hack, and that CSS pseudo_class "selected" means exactly ATK_STATE_SELECTED.

>+  /* The real dispose of this accessible is done on AtkGobject weak

(oops, just noticed miscapitalization of "AtkGObject" there)

>@@ -1300,6 +1315,7 @@ st_widget_init (StWidget *actor)
>   actor->priv = priv = ST_WIDGET_GET_PRIVATE (actor);
>   priv->is_stylable = TRUE;
>   priv->transition_animation = NULL;
>+  priv->accessible = NULL;

eh, remove that; we don't generally bother explicitly initializing NULL priv members to NULL. (The transition_animation initialization there is "wrong".)

>+st_widget_accessible_init (StWidgetAccessible *self)
>+{
>+  StWidgetAccessiblePrivate *priv = ST_WIDGET_ACCESSIBLE_GET_PRIVATE (self);
>+
>+  self->priv = priv;
>+  priv->selected = FALSE;

likewise here
Comment 16 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-18 16:46:15 UTC
(In reply to comment #15)
> (From update of attachment 178042 [details] [review])
> also basically good
> 
> > * Management of the selected stated, using the current pseudo_class.
> >   It is somewhat hacky, but the only option right now.
> 
> IMHO we have decreed that this is *not* a hack, and that CSS pseudo_class
> "selected" means exactly ATK_STATE_SELECTED.

Yeah sorry. Legacy comment. I just reused the one I added when we have that method that checked both pseudo_class and style_class looking for the string "*selected*". I will update it.

> >+  /* The real dispose of this accessible is done on AtkGobject weak
> 
> (oops, just noticed miscapitalization of "AtkGObject" there)

Ok.

> >@@ -1300,6 +1315,7 @@ st_widget_init (StWidget *actor)
> >   actor->priv = priv = ST_WIDGET_GET_PRIVATE (actor);
> >   priv->is_stylable = TRUE;
> >   priv->transition_animation = NULL;
> >+  priv->accessible = NULL;
> 
> eh, remove that; we don't generally bother explicitly initializing NULL priv
> members to NULL. (The transition_animation initialization there is "wrong".)
> 
> >+st_widget_accessible_init (StWidgetAccessible *self)
> >+{
> >+  StWidgetAccessiblePrivate *priv = ST_WIDGET_ACCESSIBLE_GET_PRIVATE (self);
> >+
> >+  self->priv = priv;
> >+  priv->selected = FALSE;
> 
> likewise here

Ok
Comment 17 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-20 12:22:33 UTC
Created attachment 178832 [details] [review]
Updated patch after review on comment 15
Comment 18 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-01-20 14:55:52 UTC
Taking into account the flag "accepted-commit_now" on the previous patch, and after a brief chat with Dan Winship on IRC, patch applied.

Close bug as FIXED

Thanks!