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 686583 - Regression: Dash item labels fail to report accessible names
Regression: Dash item labels fail to report accessible names
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.6.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-10-21 17:44 UTC by Joanmarie Diggs (IRC: joanie)
Modified: 2012-11-06 09:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
event listener (940 bytes, text/plain)
2012-10-21 17:44 UTC, Joanmarie Diggs (IRC: joanie)
  Details
dash: Update the label immediately for accessibility purposes (1.09 KB, patch)
2012-10-21 22:54 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review
dash: Use a separate label for accessibility for dash items (1.71 KB, patch)
2012-10-21 22:55 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review
Fix the regression using accessible-name (1.41 KB, patch)
2012-10-24 17:37 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
Uses accessible_name at setLabelText (1.66 KB, patch)
2012-11-05 17:39 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
accepted-commit_now Details | Review
Uses accessible_name at setLabelText (1.70 KB, patch)
2012-11-05 17:59 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
committed Details | Review

Description Joanmarie Diggs (IRC: joanie) 2012-10-21 17:44:21 UTC
Created attachment 226938 [details]
event listener

Steps to reproduce:
1. Launch the attached event listener in a terminal
2. Press Alt+F1
3. Press Ctrl+Alt+Tab to cycle through the elements once, then select Dash
4. Down Arrow through the dash items

Expected results: Dash items would have accessible names or labels with accessible names

Actual results: No Dash items have accessible names; all Dash items have labels; some labels report having names, others do not -- and in some cases, no labels report having names. Timing issue??

Related note: The final item which has the applications doesn't seem to emit an accessible signal like the others.

Sample output:

$ ./dash.py 
Name: <> Label count: 1 Label(s) name: <Top Bar>
Name: <> Label count: 1 Label(s) name: <Dash>
Name: <> Label count: 1 Label(s) name: <Windows>
Name: <> Label count: 1 Label(s) name: <Applications>
Name: <> Label count: 1 Label(s) name: <Search>
Name: <> Label count: 1 Label(s) name: <Message Tray>
Name: <> Label count: 1 Label(s) name: <Top Bar>
Name: <> Label count: 1 Label(s) name: <Dash>
Name: <> Label count: 1 Label(s) name: <> 
Name: <> Label count: 1 Label(s) name: <>
Name: <> Label count: 1 Label(s) name: <>
Name: <> Label count: 1 Label(s) name: <>
Name: <> Label count: 1 Label(s) name: <Documents>
Name: <> Label count: 1 Label(s) name: <Thunderbird>
Name: <> Label count: 1 Label(s) name: <>
Name: <> Label count: 1 Label(s) name: <>
Name: <> Label count: 1 Label(s) name: <>
Comment 1 Joanmarie Diggs (IRC: joanie) 2012-10-21 21:52:48 UTC
BTW, I have an F18 secondary laptop I've not yet updated. It currently has gnome-shell 3.6.0 and the bug is not present:

Name: <> Label count: 1 Label(s) name: <Top Bar>
Name: <> Label count: 1 Label(s) name: <Dash>
Name: <> Label count: 1 Label(s) name: <Windows>
Name: <> Label count: 1 Label(s) name: <Applications>
Name: <> Label count: 1 Label(s) name: <Search>
Name: <> Label count: 1 Label(s) name: <Message Tray>
Name: <> Label count: 1 Label(s) name: <Top Bar>
Name: <> Label count: 1 Label(s) name: <Dash>
Name: <> Label count: 1 Label(s) name: <Google Chrome>
Name: <> Label count: 1 Label(s) name: <Thunderbird>
Name: <> Label count: 1 Label(s) name: <Pidgin Internet Messenger>
Name: <> Label count: 1 Label(s) name: <Files>
Name: <> Label count: 1 Label(s) name: <Terminal>
Name: <> Label count: 1 Label(s) name: <Emacs>

(The opening report output is from F18 with gnome-shell 3.6.1)
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-10-21 22:14:52 UTC
http://git.gnome.org/browse/gnome-shell/commit/?h=gnome-3-6&id=06e5c253838e9e8b174eb47f30cd3bdfb6d80297

This may have broken it by accident.
Comment 3 Joanmarie Diggs (IRC: joanie) 2012-10-21 22:29:54 UTC
Doesn't seem to be it -- at least not in and of itself. If I reverse that patch in gnome-shell 3.6.1 the bug is still present.

But I've now got an environment where I should be able to git-bisect my way to the answer. Stay tuned. :)
Comment 4 Florian Müllner 2012-10-21 22:33:02 UTC
Looks like it was http://git.gnome.org/browse/gnome-shell/commit/?h=gnome-3-6&id=6487cd8c6f4 ...
Comment 5 Joanmarie Diggs (IRC: joanie) 2012-10-21 22:41:51 UTC
Confirmed.
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-10-21 22:54:50 UTC
Created attachment 226948 [details] [review]
dash: Update the label immediately for accessibility purposes

We changed the dash to only update the label when it was shown,
but this broke accessibility as the item would originally have
a blank name until the label was shown for the first time.

Update the label immediately, if it's not visible.
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-10-21 22:55:17 UTC
Created attachment 226949 [details] [review]
dash: Use a separate label for accessibility for dash items

We changed the dash to only update the label when it was shown,
but this broke accessibility as the item would originally have
a blank name until the label was shown for the first time.

Create a different label for accessibility purposes, rather
than reusing the existing one.



and approach 2. Tell me which you like better.
Comment 8 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-10-22 19:19:35 UTC
Thanks for the patches, I have just tested them. The problem with approach 1 and approach 2 is that using them instead of getting the correct:
"Blah push button"

Orca exposes something like:
"Blah panel Blah push button"

So not sure why, both push button and his (not relevant) container are getting the name. I have the feeling that the container hierarchy changed a little since the last time that I reviewed it (probably due the addition of new stuff like ShowAppsIcon). Anyway not sure as when I did it the change I made was trivial (bug 644255 comment 6).

So I will take a look to this in order to remove the extra name exposal, initially based on approach 2, as it is easier to understand.
Comment 9 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-10-22 19:53:07 UTC
(In reply to comment #7)
> Created an attachment (id=226949) [details] [review]
> dash: Use a separate label for accessibility for dash items
> 
> We changed the dash to only update the label when it was shown,
> but this broke accessibility as the item would originally have
> a blank name until the label was shown for the first time.
> 
> Create a different label for accessibility purposes, rather
> than reusing the existing one.
> 
> 
> 
> and approach 2. Tell me which you like better.

I see the problem now. With this patch a "virtual" label is created. But this is assigned as label_actor to both the AppWellIcon (the button) created at dash.Dash._createAppItem, and to the DashItemContainer itself at DashItemContainer._init. Yes, I know that with that explanation, that should have been happening before (see my previous reference about changes on the container hierarchy)

As I said the container having a name is irrelevant, as far as the final object receiving the focus has a proper name/role combination.

So one option would be make _a11yLabel as public (so a11yLabel). Remove the label_actor assignment at DashItemContainer. It works (already tested).

Anyway, I see creating a virtual label something like an overkill. One of the reasons to use label_actor, that creates a relation between an actor and his label, is make the most of the fact that we already have a label that can provide a name. In that way, we can just assign the relation, and any label change is properly managed.

But as this is not the case here (as the label is just updated in some cases), it would be more natural to use the StWidget.accessible_name property. After all we have that property _labelText. The problem here is that property is at the parent not the child, so we would need to add the "update accessible name when the labelText changes" code that is automatically managed by label_actor.

Tomorrow I will play a little more with this and came with a final solution.
Comment 10 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-10-24 17:37:15 UTC
Created attachment 227184 [details] [review]
Fix the regression using accessible-name

As I said on my previous comment, the main reason to use label_actor is reuse an existing label with a proper accessible name. As this is not true anymore, this patch just sets the property accessible_name

At the beginning I was trying to update the accessible_name each time setLabelText is called, adding a signal emission on that method. In theory that would be required in order to get the name updated on ShowAppsIcon, that is the only one that call that method after construction. But after some tests, I never got that code called. Taking a look to the code seems some code related to Drag&Drop.
Comment 11 Joanmarie Diggs (IRC: joanie) 2012-11-01 15:31:43 UTC
Ping? It would be awesome if we can get this in for 3.6.2.
Comment 12 Jasper St. Pierre (not reading bugmail) 2012-11-01 15:40:18 UTC
Why doesn't updating the accessible_name in setLabelText work?
Comment 13 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-11-01 16:13:50 UTC
(In reply to comment #12)
> Why doesn't updating the accessible_name in setLabelText work?

setLabelText is a DashItemContainer method. But the widget that needs a name (as is the one that it seems that is receiving the focus) is the AppDisplay.AppWellIcon created at Dash._createAppItem. In that method a AppWellIcon and a DashItemContainer are created, and the Icon is set at the child.

Anyway, now that you mention that, the child is a property at the ItemContainer, so one option would be set the accessible_name of the child at setLabelText. Anyway, I think that this will not solve the issue at the show applications button.

Will try this and create a new patch if required.
Comment 14 chris 2012-11-05 11:30:20 UTC
Is this in 3.6.2.?
Comment 15 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-11-05 17:39:56 UTC
Created attachment 228164 [details] [review]
Uses accessible_name at setLabelText

This patch is more simple. As I said on comment 13 I didn't realize that the DashItemContainer maintained a reference to the child. So this patch just sets the accessible_name of the child at setLabelText. Also added more information at one of the explanatory comments.
Comment 16 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-11-05 17:41:19 UTC
(In reply to comment #14)
> Is this in 3.6.2.?

3.6.2 was not released yet. As Joanmarie said at comment 11, it would be awesome if we can get this in for 3.6.2. 3.6.2 release is planned for next week.
Comment 17 Jasper St. Pierre (not reading bugmail) 2012-11-05 17:50:48 UTC
Review of attachment 228164 [details] [review]:

One minor nit, looks fine otherwise.

::: js/ui/dash.js
@@ +511,3 @@
+        // Override default AppWellIcon label_actor, now the
+        // accessible_name is set at DashItemContainer.setLabelText
+        display.actor.label_actor = null;

Would you mind moving this before the setLabelText call above?
Comment 18 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-11-05 17:59:34 UTC
Created attachment 228170 [details] [review]
Uses accessible_name at setLabelText

(In reply to comment #17)
> Review of attachment 228164 [details] [review]:
> 
> One minor nit, looks fine otherwise.
> 
> ::: js/ui/dash.js
> @@ +511,3 @@
> +        // Override default AppWellIcon label_actor, now the
> +        // accessible_name is set at DashItemContainer.setLabelText
> +        display.actor.label_actor = null;
> 
> Would you mind moving this before the setLabelText call above?

Patch uploaded with this change just in case.

Next question: what I need to do to indicate that I would like to have this patch included for 3.6.2? Just say that here? (As I assume that I should just commit this patch on master but not on master and gnome-3-6 branch)
Comment 19 Florian Müllner 2012-11-05 18:17:22 UTC
(In reply to comment #18)
> Next question: what I need to do to indicate that I would like to have this
> patch included for 3.6.2? Just say that here?

Yeah, that's easiest - I can then tell you to go ahead and push to gnome-3-6 as well :-)
Comment 20 Jasper St. Pierre (not reading bugmail) 2012-11-05 21:17:48 UTC
Review of attachment 228170 [details] [review]:

ACN
Comment 21 Alejandro Piñeiro Iglesias (IRC: infapi00) 2012-11-06 09:07:35 UTC
(In reply to comment #19)
> (In reply to comment #18)
> > Next question: what I need to do to indicate that I would like to have this
> > patch included for 3.6.2? Just say that here?
> 
> Yeah, that's easiest - I can then tell you to go ahead and push to gnome-3-6 as
> well :-)

Ok, committed on both master and gnome-3-6 branches.

Thanks everybody. Closing bug.