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 729603 - Regression: GDM user list buttons no longer have accessible labels
Regression: GDM user list buttons no longer have accessible labels
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.12.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2014-05-05 21:50 UTC by Joanmarie Diggs (IRC: joanie)
Modified: 2015-03-06 17:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
accessible-event listener (660 bytes, application/octet-stream)
2014-05-05 21:50 UTC, Joanmarie Diggs (IRC: joanie)
  Details
Added accessible_name to the user list button on the GDM login screen (937 bytes, patch)
2014-10-19 08:38 UTC, Peter Vágner
none Details | Review
Patch that do as described on comment 4 (3.79 KB, patch)
2014-11-13 12:44 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
patch v2 (3.96 KB, patch)
2014-11-14 07:00 UTC, Peter Vágner
none Details | Review
sets userwidgetlabel as login dialog label (3.79 KB, patch)
2015-01-09 15:29 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
sets userwidgetlabel as login dialog label (3.55 KB, patch)
2015-03-06 15:37 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
none Details | Review
loginDialog: Pass-through UserWidget's label-actor (2.55 KB, patch)
2015-03-06 16:23 UTC, Florian Müllner
committed Details | Review

Description Joanmarie Diggs (IRC: joanie) 2014-05-05 21:50:20 UTC
Created attachment 275923 [details]
accessible-event listener

As reported on the gnome-accessibility-list [1], Orca no longer presents the user name when arrowing in the list of users in GDM. This was working in 3.10 and is broken in 3.12.

From some quick triaging, the accessible objects claim to come from GNOME Shell rather than GDM, so filing here for further triaging.

As a quick-and-dirty way to reproduce this without using Orca:
  0. Create a second user for the system if one doesn't already exist
  1. Replace /usr/bin/orca with the contents of the attached accessible event listener
  2. Get into the login screen
  3. Start "orca" (Alt+Super+S)
  4. Arrow up and down between the first and second users several times
  5. Stop "orca" (Alt+Super+S)
  6. Look in /tmp for pyatspi-listener.out

Here are my results from Fedora 20 (gnome-shell 3.10.4):
  05/05/14 17:21:45 - [push button | ] label: [label | GNOME Developer]
  05/05/14 17:21:46 - [push button | ] label: [label | Joanmarie Diggs]
  05/05/14 17:21:46 - [push button | ] label: [label | GNOME Developer]
  05/05/14 17:21:47 - [push button | ] label: [label | Joanmarie Diggs]
  05/05/14 17:21:47 - [push button | ] label: [label | GNOME Developer]
  05/05/14 17:21:48 - [push button | ] label: [label | Joanmarie Diggs]

In contrast, here are my results from Fedora Rawhide (gnome-shell 3.13.1):
  05/05/14 17:23:28 - [push button | ] label: None
  05/05/14 17:23:28 - [push button | ] label: None
  05/05/14 17:23:29 - [push button | ] label: None
  05/05/14 17:23:29 - [push button | ] label: None
  05/05/14 17:23:29 - [push button | ] label: None
  05/05/14 17:23:29 - [push button | ] label: None

[1] https://mail.gnome.org/archives/gnome-accessibility-list/2014-April/msg00003.html
Comment 1 Peter Vágner 2014-10-18 12:43:58 UTC
I can see this is also present in gnome 3.14. However I have tried to look for the code responsible for this and it does not appear to come from GDM.
Has someone got an idea on how and where to fix this please?
Comment 2 Peter Vágner 2014-10-19 08:38:16 UTC
Created attachment 288831 [details] [review]
Added accessible_name to the user list button on the GDM login screen

With this applied I can hear orca presenting 'username realname pushbutton' when browsing these buttons on the login screen. This resembles the real labels layout on the screen I guess.
Comment 3 Alejandro Piñeiro Iglesias (IRC: infapi00) 2014-11-10 12:55:19 UTC
Review of attachment 288831 [details] [review]:

Not gnome-shell maintainer, but adding some comments.

::: js/gdm/loginDialog.js
@@ +65,3 @@
                                      x_align: St.Align.START,
                                      x_fill: true });
+        this.actor.accessible_name = this.user.get_user_name() + ' ' + this.user.get_real_name();

TL;DR version

Patch looks to me, in spite that we usually set relations with the already existing labels.

Full explanation:

As mentioned, the label contained the name is already available. What it is usually done is set the relation between the actor getting the focus and that label using the object label_actor:

"this.actor.label_actor = this.<label>"

*But*, after a quick skim, the problem here is that the label is an internal object of an internal object (there are more that one level of containment). Exposing it would be somewhat ugly. So not sure if the hypothetical issues of getting out-of-sync with the label that is visible worths the effort to make public a private-private label. After all, I assume that that label will use too .get_user_name() and .get_real_name().
Comment 4 Alejandro Piñeiro Iglesias (IRC: infapi00) 2014-11-10 17:54:02 UTC
IRC conversation about the patch:

<API> Jasper, could you take a look to bug 729603
<API> is a small patch that some people are asking to add to 14.2

[some time later]

<Jasper> API, I'm not sure if the accessible name should just be "username realname"
<Jasper> What about other languages? Is that gramatically correct?

[some time later]

<API> Jasper, in fact , joanie pointed me out that on screen only one appears on screen
<API> the screen reader should expose what is on screen, and not something else
<API> so I guess that is as easy as suggesting him to just use get_user_name
<API> anyway
<API> oh well
<API> is not so easy
<API> by default is real_name, afais
<Jasper> API, it depends on what fits in the space
<Jasper> API, label_actor is going to be the easiest thing to do
<API> Jasper, but as far as I see
<API> there are two labels
<Jasper> API, one for the username, one for the realname?
<API> Jasper, yes, as far as I see gdm module is using userWidget
<API> and it internally has two labels
<Jasper> API, yeah, I forgot that I wired it up that way
<API> oh well
<API> there is a currentLabel
<API> hmmm
<API> so each time currentLabel changes
<API> label_actor would need to change
<API> sooo
<API> userWidget would need to expose currentLabel
<API> and also add a "current-label-changed" signal
<API> so loginDialog, that is where the button that contains userWidget is created, could update this button label_actor
<API> Jasper, is that right or Im wrong?
<API> wrong as "there is something more simple to do"
<Jasper> API, that should be fine
Comment 5 Peter Vágner 2014-11-11 16:45:32 UTC
Oh well so let's change that to only include the user name without realname in the accessiblename property. I know we are faking it because the proper solution would be setting proper relations however as API has already pointed out this would be more difficult and also ugly like what I have currently done. Grammar is not as important here as knowing which button corresponds to which user. And hmm. do you think this should ever change while sitting at the login screen? It's verry unlikelly so we don't need to care about emmitting signals at least not in this dev cycle. If you can just push it or remove the realname part and make some of us happy if there is no better solution.
Comment 6 Peter Vágner 2014-11-13 07:46:12 UTC
Okay, I'll try again being a bit more helpfull than I was previously.
On my 15,4 inch screen both user widget internal labels are shown. How do I then expose the current label on the user widget so the proper relation can be created? I am looking to make this work and also be acceptable.
Comment 7 Alejandro Piñeiro Iglesias (IRC: infapi00) 2014-11-13 12:20:21 UTC
(In reply to comment #5)
> Oh well so let's change that to only include the user name without realname in
> the accessiblename property.

The issue is that the name exposed should be the one showed at screen. And as mentioned on the IRC chat, sometimes is one, and sometimes is the other.

> I know we are faking it because the proper
> solution would be setting proper relations however as API has already pointed
> out this would be more difficult and also ugly like what I have currently done.
> Grammar is not as important here as knowing which button corresponds to which
> user. And hmm. do you think this should ever change while sitting at the login
> screen? 

Not while sitting at the login screen, but looking at the code, the decision about which one to show is done after the creation of the elements, just when the element is shown at screen. So although it will not change while sitting at the login screen, it will change just before the login screen appears.

(In reply to comment #6)
> Okay, I'll try again being a bit more helpfull than I was previously.
> On my 15,4 inch screen both user widget internal labels are shown. How do I
> then expose the current label on the user widget so the proper relation can be
> created? I am looking to make this work and also be acceptable.

I don't have a system to work and test any patch to solve it, but I will create a non-tested patch so you could work on if you want.
Comment 8 Alejandro Piñeiro Iglesias (IRC: infapi00) 2014-11-13 12:44:23 UTC
Created attachment 290632 [details] [review]
Patch that do as described on comment 4

This patch is not tested. Uploaded as a starting point.
Comment 9 Peter Vágner 2014-11-13 16:41:02 UTC
I have played with the patch a bit and I have tracked it down to problem emitting 'label-updated'. If I comment out all emit calls it runs fine however the desired functionality is missing.
I am looking through other classes to figure out what might be need in order to make it work but I'm failing.
Comment 10 Peter Vágner 2014-11-14 07:00:58 UTC
Created attachment 290684 [details] [review]
patch v2

OK, leson on emitting signals using javascript taken and I think they are even working here. But still I am failing to understand what to do with the label in relation to the button (actor object in this case). Do I have to create the label as its child object? Won't that label be visible on the screen then? Or should I just use method labelledby on the actor to create the relationship with the label in that case I will be missing labelfor on the label it-self. I am adding revised patch which does break nothing but still does not work like we would like it to.
Comment 11 Alejandro Piñeiro Iglesias (IRC: infapi00) 2014-11-14 11:36:23 UTC
(In reply to comment #10)

> Or
> should I just use method labelledby on the actor to create the relationship
> with the label in that case I will be missing labelfor on the label it-self. I
> am adding revised patch which does break nothing but still does not work like
> we would like it to.

That is automatically done if you use label_actor as I had on my original patch:

    _onLabelUpdated: function(newLabel) {
        this.actor.label_actor = newLabel;

label_actor is used to stablish what is the label of a given actor. So when you set label_actor, internally it creates the proper accessible relations [1]

So my question is, why did you need to change that line, and used directly accessible_name and the label text?


[1] https://git.gnome.org/browse/gnome-shell/tree/src/st/st-widget.c#n2821
Comment 12 Peter Vágner 2014-11-14 16:57:48 UTC
I have tried both what you have suggested in your first version of the patch and I also tried to change it to assign to the accessible_name property directly however none of them appear to work.
Is it possible signals are not emitting then? How can I verify it? Can I print something to the log using javascript? That is mostlikelly a thing I need to learn now.

Thanks and greetings

Peter
Comment 13 Alejandro Piñeiro Iglesias (IRC: infapi00) 2015-01-09 15:29:50 UTC
Created attachment 294165 [details] [review]
sets userwidgetlabel as login dialog label

Implemented as agreed (see comment 4).

Some comments:

For UserWidgetLabel I emit the signal notify:accessible-name to notify that the label changed. This is because as UserWidgetLabel is an GObject, so emitting a new signal is not as easy as doing a this.emit("my-new-signal") as with a vanilla Javascript object. It seems that you need to declare the new GObject signal. Apart of not knowing right now how to do that (I would need to ask), I think that it would be easier to use an already available signal. After all, if the label gets updated, the accessible-name changes.

Instead of passing the label as argument of the callbacks, I ask for the currentLabel at loginDialog (I added a getter for that). One reason is that I had some problems because we are jumping from Object to GObjects. Second reason is that is more clear in this way, as it is more clear what we are setting as the label.
Comment 14 Alejandro Piñeiro Iglesias (IRC: infapi00) 2015-01-30 18:53:48 UTC
ping?
Comment 15 Florian Müllner 2015-03-06 12:53:55 UTC
Review of attachment 294165 [details] [review]:

(In reply to Alejandro Piñeiro Iglesias (IRC: infapi00) from comment #13)
> Some comments:
> 
> For UserWidgetLabel I emit the signal notify:accessible-name to notify that
> the label changed. [...]
> 
> Instead of passing the label as argument of the callbacks, I ask for the
> currentLabel at loginDialog (I added a getter for that) ...

From the comment I'm pretty sure you attached the wrong patch?

::: js/ui/userWidget.js
@@ +76,3 @@
         this.add_child(this._userNameLabel);
 
+        this.currentLabel = null;

This is still only used from the class itself, so why make it public?

@@ +116,2 @@
         if (natRealNameWidth <= availWidth)
+            _updateLabel (this._realNameLabel);

this._updateLabel()

@@ +131,3 @@
+        this.currentLabel = newLabel;
+
+        this.emit('label-updated', newLabel);

As you said yourself in your comment, this doesn't work in a GObject subclass

@@ +162,3 @@
         this.actor.add_child(this._avatar.actor);
 
+        this.label = new UserWidgetLabel(user);

Either use "this._userWidget.label.connect('label-updated', ...)" in loginDialog, or keep the property private

@@ +164,3 @@
+        this.label = new UserWidgetLabel(user);
+        this.actor.add_child(this.label);
+        this.label.connect('label-updated', Lang.bind(this, this.labelUpdated));

The actual function name is _labelUpdated() with underscore ...
Comment 16 Alejandro Piñeiro Iglesias (IRC: infapi00) 2015-03-06 15:37:31 UTC
Created attachment 298725 [details] [review]
sets userwidgetlabel as login dialog label

Thanks for the review.

(In reply to Florian Müllner from comment #15)
> Review of attachment 294165 [details] [review] [review]:
> 
> (In reply to Alejandro Piñeiro Iglesias (IRC: infapi00) from comment #13)
> > Some comments:
> > 
> > For UserWidgetLabel I emit the signal notify:accessible-name to notify that
> > the label changed. [...]
> > 
> > Instead of passing the label as argument of the callbacks, I ask for the
> > currentLabel at loginDialog (I added a getter for that) ...
> 
> From the comment I'm pretty sure you attached the wrong patch?

Yes. I'm an idiot.

And worse, I didn't have a local copy of the correct patch anymore. I'm an idiot^2.

In any case, as I was gentle enough to explain what I exactly did on my lost patch, so I re-did the patch again. It also helped that now the gdm mode works smoothly (don't know who solved that, but good job and thanks).

So new patch attached. Sorry for the mistake.
Comment 17 Florian Müllner 2015-03-06 16:23:48 UTC
Created attachment 298734 [details] [review]
loginDialog: Pass-through UserWidget's label-actor

While looking at the patch, I came up with a slightly cleaner approach, which we agreed on on IRC.
Comment 18 Florian Müllner 2015-03-06 16:25:17 UTC
Attachment 298734 [details] pushed as 8eb0782 - loginDialog: Pass-through UserWidget's label-actor
Comment 19 Alejandro Piñeiro Iglesias (IRC: infapi00) 2015-03-06 17:15:41 UTC
(In reply to Florian Müllner from comment #17)
> Created attachment 298734 [details] [review] [review]
> loginDialog: Pass-through UserWidget's label-actor
> 
> While looking at the patch, I came up with a slightly cleaner approach,
> which we agreed on on IRC.

Note that as part of that IRC chat, Florian offered to include this patch on the gnome 3-14 branch. So ...

(In reply to Peter Vagner from comment #1)
> I can see this is also present in gnome 3.14. 

... this will get solved on the next gnome 3.14.X release, without needing to update to 3.16.

Thanks everybody.