GNOME Bugzilla – Bug 644253
Required a way to relate a StWidget with the ClutterActor that labels it
Last modified: 2011-04-27 00:18:01 UTC
On several cases, there are a several ui elements that includes a StLabel with his name. For example: * On the alt+tab or ctrl+alt+tab menu, each icon has a label with his name * On the overview, the application list, also include a label with the application name Right now, this label is just a different StWidget (StLabel) that it is part of the visual ui. It would be good to have a way to relate one with the other, it is, specify that this object is the label for the whole. There are a similar management on gtk, with the mnemonics, but probably it would be enough something simpler. Just a specific use for that: accessibility support. Visually it is easy to identify that the label is identifying the ui element, but it would be required more information to the ATs (ie screen readers). So having this relation explicit, it would be possible to get the name of the ui element from the label.
Created attachment 182891 [details] [review] Adds a new StWidget:label-object property This patch adds a new property to StWidget: label-object So using st_widget_set_label_object, you can specify the StWidget that represents the label for this widget. You can see it aa simplified version of gtk_widget_add_mnemonic_label, as in this case you only label one widget with just one label. Although there was the possibility of explicitly ask for a StLabel, in the end I prefer to be more general, as any widget (current of future) with a text representation and name would be valid as label. Note: I use as name "label-object" instead of "label" because I have some overlapping problems with StButton:label and StTooltip:label. In both cases label is just a string.
Created attachment 182892 [details] [review] StWidgetAccessible using StWidget:label-object StWidgetAccessible using StWidget:label-object, specifically it sets the relationships LABELLED_BY and LABEL_FOR, updating it on any notification on a change on the "label-object" of the base object.
Created attachment 182893 [details] [review] Using StWidget:label-object on some ui elements Specifically: * Alt+Tab ui elements * Control+Alt+Tab ui elements * Application element on the application search This is more a testing patch, as in the case of the application element I'm still directly accessing to private data. IMHO, the easiest way to solve it is consider it public.
BTW, for testing purposes, I tried the patch I uploaded on 640057, and with that more things became meaningful, although of course, it required more testing (ie: too many events exposed).
Comment on attachment 182891 [details] [review] Adds a new StWidget:label-object property >widget. The name is label-object to avoid problems with the current >StButton:label and StTooltip:label how about label-actor? >+ StWidget *label; >+ PROP_LABEL for consistency, please rename these to match the property >@@ -964,6 +991,7 @@ st_widget_class_init (StWidgetClass *klass) > NULL, NULL, > _st_marshal_VOID__VOID, > G_TYPE_NONE, 0); >+ > } kill that extra newline >+ if (widget->priv->label) >+ g_object_unref (widget->priv->label); >+ >+ widget->priv->label = g_object_ref (label); You need a NULL check on the g_object_ref() too
Comment on attachment 182892 [details] [review] StWidgetAccessible using StWidget:label-object >+ /* The current_label. Right now there are the proper atk >+ relationships between this object and the label */ >+ AtkObject *current_label; /* multi-line comments * like this please */ > } > >+ > static void > on_can_focus_notify (GObject *gobject, another random whitespace addition >+ * so it is fine to remove the previous relatioships if we have the missing "n" in "relationships" >+ label = st_widget_get_label_object (widget); >+ if (label == NULL) >+ return; need to set priv->current_label to NULL
Comment on attachment 182893 [details] [review] Using StWidget:label-object on some ui elements >@@ -1053,6 +1057,7 @@ ThumbnailList.prototype = { > this._thumbnailBins.push(bin); > > let title = windows[i].get_title(); >+ > if (title) { stop adding blank lines! :) >@@ -399,6 +399,8 @@ AppWellIcon.prototype = { > this.icon = new AppIcon(app, iconParams); > this.actor.set_child(this.icon.actor); > >+ this.actor.label_object = this.icon._name; //FIXME: _name is private You can just rename BaseIcon._name to "label". Also, what about Dash items? They use { showLabel: false }, so currently they have no label. Maybe you should change BaseItem to just create the StLabel anyway, but hide it? (Actually, that would make it show up in st_describe_actor()'s output too, so I think that's probably the right thing.)
Also... what about the status icons? In that case, each one has a name, but it's not indicated in the UI anywhere... probably we should add an accessible_name property to StWidget (in addition to accessible_role, as suggested in bug 626660 comment 9).
Thanks for the review, I will update all those patches (probably on the weekend). (In reply to comment #7) > Also, what about Dash items? They use { showLabel: false }, so currently they > have no label. Maybe you should change BaseItem to just create the StLabel > anyway, but hide it? (Actually, that would make it show up in > st_describe_actor()'s output too, so I think that's probably the right thing.) Well, about the dash items, I already detected this problem, and I created bug 644255 to debate this. So I will copy this comment there.
(In reply to comment #8) > Also... what about the status icons? In that case, each one has a name, but > it's not indicated in the UI anywhere... probably we should add an > accessible_name property to StWidget (in addition to accessible_role, as > suggested in bug 626660 comment 9). Right now, by default clutter::name is used as the default accessible name, so what about setting the name of the status icons clutter object? That would allow to avoid defining a new property. About the accessible_role, before defining new roles, it would be good to check which objects require it. As you suggest on that comment, this could be useful for pushable objects, but it could be better to just create a accessible class for StClickable with the proper role. And now a (off-topic) rant: In fact, there are more reasons to define a accessible class for StClickable. Right now, any clutter actor includes as actions "click","release" and "push". The main reason is that on those times, we lack any contextual information from the actor, and apps (ie hildon-desktop) were just using textures as buttons. Now, as toolkits start to appear, giving that context, I thing that the "right thing" is: * Remove any default action from cally-actor. It is the most abstracted class, so we can't suppose that we can do specific actions with it. * Define a accessible class for StClickable, adding those actions ("click", "press", "release") on it.
(In reply to comment #10) > Right now, by default clutter::name is used as the default accessible name, so > what about setting the name of the status icons clutter object? That would > allow to avoid defining a new property. The problem is that ClutterActor:name is also used for CSS lookup, and those two uses conflict; in the CSS case we need a syntactically-simple and constant name, and in the a11y case we need a possibly-multi-word and localized name. > Now, as toolkits start to appear, giving that context, I thing that the "right > thing" is: > * Remove any default action from cally-actor. It is the most abstracted > class, so we can't suppose that we can do specific actions with it. > * Define a accessible class for StClickable, adding those actions ("click", > "press", "release") on it. Agree.
(In reply to comment #11) > (In reply to comment #10) > > Right now, by default clutter::name is used as the default accessible name, so > > what about setting the name of the status icons clutter object? That would > > allow to avoid defining a new property. > > The problem is that ClutterActor:name is also used for CSS lookup, and those > two uses conflict; in the CSS case we need a syntactically-simple and constant > name, and in the a11y case we need a possibly-multi-word and localized name. Ok, I will take that into account when I start to review the status icons, probably as part of bug 634016
Note: while updating the patches with Dan Winship suggestion I realized that it was not working for the settings and places icons on the overview. Using the inspector, those are also containers with a label inside, but I didn't find yet where to set it, or if the current code doesn't update properly the relation.
Created attachment 185453 [details] [review] Add a new property StWidget:label-actor
Created attachment 185454 [details] [review] Using the new property 'label-actor' on StWidgetAccessible
Created attachment 185455 [details] [review] Setting the 'label-actor' on the ui code Updated after Dan review, and also setting the label-actor on some icons that I missed (as I said on a previous comment)
Created attachment 185456 [details] [review] Adds a null check It seems that there is something wrong on my system, as it is not able to find some icons, so this null check avoids a warning. Although this not really belong here, not sure if it is worth to report a new bug (in case it is a bug). BTW, funny, because on those icons without image, I just see a hole and some "...", no label. But orca properly speechs out the name (ie: 'gedit').
Comment on attachment 185453 [details] [review] Add a new property StWidget:label-actor >+ StWidget *label_actor; This could be ClutterActor instead (and likewise everywhere else). I don't have any specific examples of when we'd want to use a non-widget actor, but it's good to leave the possibility open. >+StWidget* >+st_widget_get_label_actor (StWidget *widget) space before the "*" on the first line >+ * @widget. @label can be NULL to indicate that @widget is not anymore >+ * labelled "%NULL". (And "not labelled any more".)
Comment on attachment 185454 [details] [review] Using the new property 'label-actor' on StWidgetAccessible >+check_labels (StWidgetAccessible *widget_accessible, >+ StWidget *widget) line up "widget" with "widget_accessible" Otherwise this all looks good, though it would need a few changes if the previous patch is going to be changed to use ClutterActor instead of StWidget.
Comment on attachment 185455 [details] [review] Setting the 'label-actor' on the ui code >@@ -832,6 +834,8 @@ AppIcon.prototype = { > this.actor.add(this._iconBin, { x_fill: false, y_fill: false } ); > this.label = new St.Label({ text: this.app.get_name() }); > this.actor.add(this.label, { x_fill: false }); >+ >+ this.actor.label_actor = this.label; So for the application icons, you set label_actor on both the AppIcon.actor (here) and the 'item-box' (in addItem). But for window thumbnails you only set it on the 'item-box'. Any reason for that?
Comment on attachment 185456 [details] [review] Adds a null check this is caused by not having gnome-settings-daemon running, and there's another bug about that somewhere
Created attachment 186678 [details] [review] Add a new property:label-actor
Created attachment 186679 [details] [review] Use StWidget:label actor on StWidgetAccessible
Created attachment 186680 [details] [review] Setting 'label-actor' on the ui code (In reply to comment #20) > (From update of attachment 185455 [details] [review]) > >@@ -832,6 +834,8 @@ AppIcon.prototype = { > > this.actor.add(this._iconBin, { x_fill: false, y_fill: false } ); > > this.label = new St.Label({ text: this.app.get_name() }); > > this.actor.add(this.label, { x_fill: false }); > >+ > >+ this.actor.label_actor = this.label; > > So for the application icons, you set label_actor on both the AppIcon.actor > (here) and the 'item-box' (in addItem). But for window thumbnails you only set > it on the 'item-box'. Any reason for that? Basically I started on the constructor at seemed the logical way to set the label, but after test it, that didn't solve all the cases, so I just added the same on addItem. But you are right, it is enough to set the label just on addItem. Patch updated.
Patchs applied. Closing bug. Thanks