GNOME Bugzilla – Bug 636717
[PATCH] Required accessibility support for StWidget
Last modified: 2011-01-20 14:55:52 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.
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.
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 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 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.
filed bug 637830 with a patch to use 'selected' pseudo-class for alt-tab dialog
(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
(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
(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
(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
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
Created attachment 177473 [details] [review] Update: rebased patch
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.
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.
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 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
(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
Created attachment 178832 [details] [review] Updated patch after review on comment 15
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!