1628415 - Stop using nsIDOMXULSelectControlItemElement::label for a11y to fix broken richlistitem names

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[:Gijs (he/him)]

Richlistitems have a custom label getter that collates the contents of all the value attributes of all the label elements in the item. This is seldom helpful - in fact, in bug 1628255 and bug 1568453 it produced the wrong results.

At this point, it probably makes sense to just remove this, and audit consumers that may be relying on it to give them explicit aria-labelledby or similar attributes to provide an appropriate accessible name.

Comment 1

[James Teh [:Jamie]]

One other consumer I was thinking of was the applications list in about:preferences. It turns out this label property is also doing the wrong thing there, resulting in useless labels like "Always ask Always ask Save File Use other…"; i.e. the content type is missing altogether. I'm not sure if just removing the label property would do the right thing, but certainly, this is another case where the label property is absolutely doing the wrong thing.

I want to say this is access-p1 because it's breaking the applications list, but the broader fix here will require auditing all usage of richlistitem, which probably isn't realistic in a short time frame. Thus, access-p2.

Comment 2

[Asa Dotzler [:asa]]

Updating the Accessibility Team's impact assessment to conform with the new triage guidelines. See https://wiki.mozilla.org/Accessibility/Triage for descriptions of these whiteboard flags.

Comment 3

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #1)

One other consumer I was thinking of was the applications list in about:preferences. It turns out this label property is also doing the wrong thing there, resulting in useless labels like "Always ask Always ask Save File Use other…"; i.e. the content type is missing altogether. I'm not sure if just removing the label property would do the right thing,

it does. Surprise surprise.

Comment 4

[James Teh [:Jamie]]

I think we should prioritise getting the applications list in about:preferences working, since we're looking at using the Firefox default PDF viewer when opening a PDF from the Downloads list instead of using the OS default PDF viewer; see bug 1191591. Given the a11y issues with pdf.js, a11y users may be relying on the current behaviour and may need to change it. Furthermore, the support article addressing this will instruct users as to how to change the default; see bug 1631359. Right now, screen reader users can't change the default due to this list being effectively inaccessible.

I looked into where nsIDOMXULSelectControlItemElement::label is implemented and it looks like richlistitem is the only implementer now.

I also searched for existing uses of richlistitem by searching for <richlistitem and "richlistitem on Searchfox. I'm testing these with the label property removed. I'll follow up with my test results soon, but it's looking promising so far.

Comment 5

[James Teh [:Jamie]]

Test results with the richlistitem label property removed. TL;DR: all looks good; better or same.

• The applications list in about:preferences: now works where it didn't before. This also has two associated dialogs which both have richlistitems:
  • Choose application (AKA Use other…, appPicker.xhtml): Works as expected; no change.
  • Application manager (AKA Application Details…, applicationManager.xhtml): Works as expected; no change.
• The categories list in about:preferences: works as expected; no change.
• toolkit/mozapps/handling/content/dialog.xhtml: Tested by going to irc://test. Works as expected; no change.
• Downloads list: Works as expected; no change.
• Tags selector in the Edit Bookmark dialog: works as expected; no change.
• Tab containers list in about:preferences#containers: Now works better than it did before, though still includes button names in the item names where it probably shouldn't (e.g. "Personal Preferences Remove" instead of just "Personal". However, this is far better than it was before; e.g. it just reported "Preferences Remove".
• Page display languages in about:preferences: Works as expected; no change.
• Language Settings dialog (AKA Set Alternatives…, browserLanguages.xhtml, pref intl.multilingual.enabled set to true): Works as expected; no change.
• Manage Cookies and Site Data dialog in about:preferences: Works as expected; no change.
• Exceptions - Cookies and Site Data dialog, Exceptions - Saved Logins dialog in about:preferences: Works as expected; no change.
• Settings for various permissions in about:preferences (e.g. location): Works as expected and better than it did before. Now, you get, for example, "https://browserleaks.com Block". Previously, you got "https://browserleaks.com Allow Block".
• Pop-up block exceptions in about:preferences: Works as expected; no change.
• Saved Addresses: Works as expected; no change.
• Delete Certificate: Works as expected; no change.
• Form autofill (using datalist): Works as expected; no change.
• Search bar autocomplete: Works as expected; no change.
• toolkit/mozapps/extensions/content/blocklist.js: Not sure how to bring this up. However, looking at the code, I think it should be fine.
• Update history: Works as expected; no change.
• Profile selection screen (-profilemanager command line argument): Works as expected; no change.

Comment 6

[James Teh [:Jamie]]

Raising to access-s2 given that several list boxes are inaccessible to screen reader users at present.

Comment 7

[James Teh [:Jamie]]

(Duplicate comment.)

Comment 8

[James Teh [:Jamie]]

Ug. It looks like a bunch of tests depend on richlistitem's label getter:
https://treeherder.mozilla.org/pushhealth.html?revision=dbfdd5ab30ec1204a0b6000b5dbb182e5c7ea383&repo=try
Also, bug 460942 suggests this property wasn't just for a11y... although I don't really know what it's useful for in reality.

I don't really want to deal with the potential non-a11y fallout from changing this, so I'm just going to stop calling this from a11y.

Comment 9

[James Teh [:Jamie]]

Attachment: Bug 1628415: Remove richlistitem label getter.

This was only used for a11y, but it did the wrong thing in several cases.
The a11y engine's label computation code generally does a better job.
Where the label does need to be overridden in specific cases, we should use ARIA instead.

Comment 10

[James Teh [:Jamie]]

Attachment: Bug 1628415: Remove nsIDOMXULSelectControlItemElement::label attribute.

This was only used by a11y, which no longer uses it.

Comment 11

[James Teh [:Jamie]]

Since I already wrote them, I uploaded the patches to remove nsIDOMXULSelectControlItemElement::label for reference in case we ever want to deal with this in future; see comment 9, comment 10. I've marked them as abandoned, though.

Comment 12

[James Teh [:Jamie]]

Attachment: Bug 1628415 part 1: Remove use of nsIDOMXULSelectControlItemElement::label in Accessible.

This was only used for richlistitems, but the richlistitem implementation of this property did the wrong thing in several cases.
The a11y engine's label computation code generally does a better job.
Where the label does need to be overridden in specific cases, we should use ARIA instead.

Comment 13

[James Teh [:Jamie]]

Attachment: Bug 1628415 part 2: Correct comments regarding XUL control Accessible name computation.

This is not strictly related to this bug, but I'm touching the surrounding code and I've been confused by this on several occasions.
The previous comments suggested that NameFromAssociatedXULLabel (which uses XULLabelIterator) looks at child labels.
This is incorrect: XULLabelIterator only looks at <label control="id">.
The inclusion of child labels in the name comes from GetNameFromSubtree, which is called elsewhere.

Comment 14

[James Teh [:Jamie]]

Attachment: Bug 1628415 part 3: Remove XULListboxAccessible::Value.

This depends on richlistitem's nsIDOMXULSelectControlItemElement::label implementation, which does the wrong thing in several cases.
We could make this use a11y name computation, but I can't find any other list box implementation that exposes accessible value on the list box itself, so I don't see any reason to keep this.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/daaca69843b1
part 1: Remove use of nsIDOMXULSelectControlItemElement::label in Accessible. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/4ccaabe1369e
part 2: Correct comments regarding XUL control Accessible name computation. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/533d0fa6719d
part 3: Remove XULListboxAccessible::Value. r=MarcoZ

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/daaca69843b1
https://hg.mozilla.org/mozilla-central/rev/4ccaabe1369e
https://hg.mozilla.org/mozilla-central/rev/533d0fa6719d

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Jamie, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 18

[James Teh [:Jamie]]

Comment on attachment 9146691 [details]
Bug 1628415 part 1: Remove use of nsIDOMXULSelectControlItemElement::label in Accessible.

Beta/Release Uplift Approval Request

• User impact if declined: Some list boxes such as the applications list and the tab containers list in Firefox Options will be inaccessible to screen reader users.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: While I've requested uplift for parts 1 and 3 and both patches are low risk, part 1 is sufficient to fix the major user-visible issue.
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only affects accessibility labels for richlistitems and doesn't alter most use cases. I've manually verified all uses of richlistitem I could find in the code.
• String changes made/needed:

Comment 19

[Pascal Chevrel:pascalc]

Comment on attachment 9146691 [details]
Bug 1628415 part 1: Remove use of nsIDOMXULSelectControlItemElement::label in Accessible.

Fix a significant accessibility bug for screen readers, uplift approved for beta, thanks.

Comment 20

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9235f8bbbcc1
https://hg.mozilla.org/releases/mozilla-beta/rev/a77a916d69d6