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)

http://git.gnome.org/browse/gnome-shell/commit/?h=gnome-3-6&id=06e5c253838e9e8b174eb47f30cd3bdfb6d80297

This may have broken it by accident.

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. :)

Looks like it was http://git.gnome.org/browse/gnome-shell/commit/?h=gnome-3-6&id=6487cd8c6f4 ...

Confirmed.

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.

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.

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.

(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.

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.

Ping? It would be awesome if we can get this in for 3.6.2.

Why doesn't updating the accessible_name in setLabelText work?

(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.

Is this in 3.6.2.?

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.

(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.

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?

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)

(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 :-)

Review of attachment 228170 [details] [review]:

ACN

(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.