GNOME Bugzilla – Bug 667432
Orca doesn't present the proper role for items on the top panel
Last modified: 2012-03-15 18:00:50 UTC
STEPS TO REPRODUCE 1. Open Orca 2. Navigate on the top panel (Ctrl+Alt+Tab) EXPECTED OUTCOME Proper name/role combination ACTUAL OUTCOME Most of the items are just "PANEL" NOTES: In this case this is not just about defining an accessible object with the proper role by default. Most of those items (PopupMenuItem, PopupMenu, etc) are in fact Shell.GenericContainer. So as a "generic" container, PANEL is a proper default role. So in order to solve that, it would be required to set on the javascript code the proper role.
Created attachment 204771 [details] [review] Adds a new property "accessible-role" on StWidget A long time ago, when I discussed this role thing with Dan Winship, we briefly commented that it would be good adding a property like this. It is true that you can get the same asking for the accessible object and calling atk_object_set_role, but with the property you can avoid that intermediate step, and you could also pass the property to the GenericContainer constructor (not always the case, as sometimes it is created on the parent class, and you only now the specific role in the subclass).
Created attachment 204772 [details] [review] Setting the specific role on some panel items All those panel items are GenericContainer. Using accessible-role to set the specific role
Created attachment 205711 [details] [review] Setting the specific role on some panel items
Review of attachment 204771 [details] [review]: Looks generally good - most of my comments here are on the docs ::: src/st/st-widget.c @@ +995,3 @@ + * StWidget:accessible-role: + * + * Accessible role for this object This should either be exactly what you have in the property itself, or you should have something more informative (I don't see much point in having it at all if it's exactly the same, but all the properties here do seem to have a doc comment) What would be useful here is linking to st_widget_set_accessible_role(), where you have more docs. @@ +2233,3 @@ + * + * Sets the #AtkRole for this widget, overriding (so refining) the one + * provided for the specific accessible object. Note that this is the This docs is written here to the wrong audience - it seems to be targeting someone who is deeply familiar with accessibility. Start off by saying what this function is, as if you were speaking to someone who runs into a call to this in code they were reading or were told "you should set the accessible role", and if you need caveats or comparisons, do that afterwards. @@ +2240,3 @@ + * The reasons is that generic objects (like #ShellGenericContainer, + * #StContainer subclass) will behave in a specific way (ie: menu, + * menu items, buttons, etc) and the js code. I don't understand this paragraph at all, unfortunately. @@ +2256,3 @@ + accessible = st_widget_get_accessible (CLUTTER_ACTOR (widget)); + + if (accessible != NULL) this check is a maybe unnecessary because I think it's not legitimate to override the default get_accessible and return NULL, but OK. @@ +2280,3 @@ + g_return_val_if_fail (ST_IS_WIDGET (widget), ATK_ROLE_INVALID); + + accessible = st_widget_get_accessible (CLUTTER_ACTOR (widget)); ugh, I don't like the getter here creating the accessible obejct - I'd probably just use widget->priv->accessible directly
Review of attachment 205711 [details] [review]: Just a few things ::: js/ui/dateMenu.js @@ +59,3 @@ + // At this moment calendar is not keyboard navigable at all + // (so no accessible), so it doesn't make sense to set as role s/no/not/. @@ +61,3 @@ + // (so no accessible), so it doesn't make sense to set as role + // ATK_ROLE_CALENDAR. From the accessibility POV it is just a + // label with the current date I'm a little confused here - "An object that displays a calendar and allows the user to select a date " - but this is the menu label, right? for a panel that shows appointments, etc. Isn't it ATK_ROLE_MENU_ITEM (ATK_ROLE_MENU?) if anything?
Created attachment 206540 [details] [review] Adding a wrap method to set the accessible role This patch tries to solve the issues with the documentation. It also modify a little the implementation. Previous implementation just got the accessible object to do the set/get. Owen mentioned that this would mean to create the accessible object on the get_role. But that would be also true on the set_object. If the accessible object doesn't exist, that method will create it, when the accessible object should be only created if the accessibility technologies required it. So now this is implemented by having an accessible-role var on StWidget. I redefined _get_role implementation of StWidgetAccessible to return that role if available.
Created attachment 206541 [details] [review] Setting the specific role on some panel items (From comment 5) > I'm a little confused here - "An object that displays a calendar and allows the > user to select a date " - but this is the menu label, right? for a panel that > shows appointments, etc. Isn't it ATK_ROLE_MENU_ITEM (ATK_ROLE_MENU?) if > anything? You are right. ATK_ROLE_CALENDAR is for the calendar itself. That item should have ATK_ROLE_MENU. I have updated that comment.
Created attachment 208501 [details] [review] Adding a wrap method to set the accessible role Due last changes on gnome-shell previous patch didn't apply with just an "git am"
Created attachment 208502 [details] [review] Setting the specific role on some panel items Due last changes on gnome-shell previous patch didn't apply with just an "git am"
Created attachment 208674 [details] [review] Setting the specific role on some panel items Taking into account PopupSwitchMenuItem.setStatus(). See bug 668366 comment 9 and bug 668366 comment 11 for more information.
Created attachment 208697 [details] [review] Adding a wrap method to set the accessible role Fixing some style issues
Comment on attachment 208674 [details] [review] Setting the specific role on some panel items >@@ -18,6 +19,7 @@ const ButtonBox = new Lang.Class({ > params = Params.parse(params, { style_class: 'panel-button' }, true); > this.actor = new Shell.GenericContainer(params); > this.actor._delegate = this; >+ this.actor.accessible_role = Atk.Role.PUSH_BUTTON; OK, so the class hierarchy is: PanelMenu.ButtonBox PanelMenu.Button DateMenu.DateMenuButton Panel.AppMenuButton Panel.ActivitiesButton PanelMenu.SystemStatusButton UserMenu.UserMenuButton status.keyboard.XKBIndicator legacy trayicon boxes The legacy trayicon boxes are really neither PUSH_BUTTON nor MENU. Everything else is MENU (except DateMenuButton because it's broken). So, I think you should not set a role on PanelMenu.ButtonBox (or set it to whatever GtkBin is), and set a role of MENU on PanelMenu.Button. (And downgrade that to LABEL on DateMenuButton, and remove the places where you're setting the other subclasses to MENU.)
Comment on attachment 208697 [details] [review] Adding a wrap method to set the accessible role >Subject: [PATCH 1/2] a11y: Adding a wrap method to set the accessible role "Add a wrapper method"... >+ PROP_ACCESSIBLE_ROLE, >+ g_param_spec_int ("accessible-role", Should be g_param_spec_enum() (and likewise use g_value_get/set_enum() in the get/set_property() functions).
Created attachment 208705 [details] [review] Add a wrap method to set the accessible role (In reply to comment #13) > (From update of attachment 208697 [details] [review]) > >Subject: [PATCH 1 [details]/2] a11y: Adding a wrap method to set the accessible role > > "Add a wrapper method"... > > >+ PROP_ACCESSIBLE_ROLE, > >+ g_param_spec_int ("accessible-role", > > Should be g_param_spec_enum() (and likewise use g_value_get/set_enum() in the > get/set_property() functions). Updated patch
Created attachment 208706 [details] [review] Set the specific role on some panel items (In reply to comment #12) > So, I think you should not set a role on PanelMenu.ButtonBox (or set it to > whatever GtkBin is), and set a role of MENU on PanelMenu.Button. (And downgrade > that to LABEL on DateMenuButton, and remove the places where you're setting the > other subclasses to MENU.) This patch updates sets the MENU role on PanelMenu.Button. Keeps LABEL on DateMenuButton. Using PUSH_BUTTON on PanelMenu.ButtonBox was required due the Activities button. But it is true that it is not proper as default role for the hierarchy. So this patch sets PUSH_BUTTON on Panel.ActivitiesButton
Comment on attachment 208705 [details] [review] Add a wrap method to set the accessible role >Subject: [PATCH 1/2] a11y: Add a wrap method to set the accessible role "wrapper" not "wrap" >+ case PROP_ACCESSIBLE_ROLE: >+ g_value_set_int (value, st_widget_get_accessible_role (actor)); needs to be g_value_set_enum(). (Did you test the updated patch? It should fail and g_warn like this...) Otherwise fine, and OK to commit with that fix (though, really, you should test that it's actually working first...)
Comment on attachment 208706 [details] [review] Set the specific role on some panel items > So this patch sets PUSH_BUTTON on Panel.ActivitiesButton Hm. Would ATK_ROLE_TOGGLE_BUTTON be more correct? Visually, it is highlighted the entire time the overview is open, not just when you click on it. (OK to commit with or without that change.)
Comment on attachment 208706 [details] [review] Set the specific role on some panel items >@@ -744,6 +747,9 @@ const PopupSwitchMenuItem = new Lang.Class({ > this.label = new St.Label({ text: text }); > this._switch = new Switch(active); > >+ this.actor.accessible_role = Atk.Role.CHECK_MENU_ITEM; >+ this.actor.label_actor = this.label; actually, per bug 668366, weren't you supposed to make this alternate between CHECK_MENU_ITEM and MENU_ITEM depending on whether the switch was visible or not?
(In reply to comment #17) > (From update of attachment 208706 [details] [review]) > > So this patch sets PUSH_BUTTON on Panel.ActivitiesButton > > Hm. Would ATK_ROLE_TOGGLE_BUTTON be more correct? Visually, it is highlighted > the entire time the overview is open, not just when you click on it. > > (OK to commit with or without that change.) This makes sense. Anyway I will not commit the patch because ... (In reply to comment #18) > (From update of attachment 208706 [details] [review]) > >@@ -744,6 +747,9 @@ const PopupSwitchMenuItem = new Lang.Class({ > > this.label = new St.Label({ text: text }); > > this._switch = new Switch(active); > > > >+ this.actor.accessible_role = Atk.Role.CHECK_MENU_ITEM; > >+ this.actor.label_actor = this.label; > > actually, per bug 668366, weren't you supposed to make this alternate between > CHECK_MENU_ITEM and MENU_ITEM depending on whether the switch was visible or > not? ... sorry, when I updated the patch on but 668366 I also updated this patch, but I didn't upload it here.
Created attachment 209368 [details] [review] Set the specific role on some panel items This patch includes new stuff, like deciding if a panel item is CHECK_MENU_ITEM or just MENU_ITEM. So I guess that requires further review. See comment 19 for more information. Sorry for the noise.
(In reply to comment #16) > (From update of attachment 208705 [details] [review]) > >Subject: [PATCH 1 [details]/2] a11y: Add a wrap method to set the accessible role > > "wrapper" not "wrap" > > >+ case PROP_ACCESSIBLE_ROLE: > >+ g_value_set_int (value, st_widget_get_accessible_role (actor)); > > needs to be g_value_set_enum(). (Did you test the updated patch? It should fail > and g_warn like this...) > > > Otherwise fine, and OK to commit with that fix (though, really, you should test > that it's actually working first...) Yes my bad, I tested the commit but just with the real environment and with Orca. I didn't realize that in this case set_property was not used at all. Anyway, patch committed.
Review of attachment 209368 [details] [review]: ::: js/ui/panelMenu.js @@ +20,3 @@ this.actor = new Shell.GenericContainer(params); this.actor._delegate = this; + this.actor.accessible_role = Atk.Role.TOGGLE_BUTTON; I don't think this makes sense - panel-buttons are basically the same as system-status-buttons, just wider. In my opinion it makes more sense to set a reasonable default here and only overwrite the Activities button (which really is the special case, not app/user menu)
Created attachment 209739 [details] [review] Set the specific role on some panel items (In reply to comment #22) > Review of attachment 209368 [details] [review]: > > ::: js/ui/panelMenu.js > @@ +20,3 @@ > this.actor = new Shell.GenericContainer(params); > this.actor._delegate = this; > + this.actor.accessible_role = Atk.Role.TOGGLE_BUTTON; > > I don't think this makes sense - panel-buttons are basically the same as > system-status-buttons, just wider. In my opinion it makes more sense to set a > reasonable default here and only overwrite the Activities button (which really > is the special case, not app/user menu) Yes sorry. I already made that change after Dan review on comment 12. It seems that when I updated again the patch on comment 19 and comment 20 I screwed the patch.
Comment on attachment 209739 [details] [review] Set the specific role on some panel items looks right