Found while working on bug 1572641, particularly part 3, where when sending a "Site not working" report, associating the xul:label and html:input or html:textarea via the xul:label's control attribute does not generate the accessible name for the input and textareas respectively. The association does work, in that when clicking with the mouse on the label text, the associated input receives focus, but the accessibility engine does not accept the xul:label as a name source for html:textarea or html:input in a XUL document.

Marco, what's the priority of this for Skyline? Can we use aria-labelledby as a fallback? Fixing this shouldn't be too hard, but I just want to know what our options are.

Looking briefly, we'd need to reuse some of the code from XULElmName:
https://searchfox.org/mozilla-central/rev/9775cca0a10a9b5c5f4e15c8f7b3eff5bf91bbd0/accessible/generic/Accessible.cpp#787
We'd do this in NativeName when considering HTML elements:
https://searchfox.org/mozilla-central/rev/9775cca0a10a9b5c5f4e15c8f7b3eff5bf91bbd0/accessible/generic/Accessible.cpp#1981
I don't think we can unify the code path for this into a single code path, as XULElmName only does this if it can't get the name elsewhere first.

One question is what should happen if there's both HTML label @for and XUL label @controls. Should they both combine?

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

Marco, what's the priority of this for Skyline? Can we use aria-labelledby as a fallback? Fixing this shouldn't be too hard, but I just want to know what our options are.

This is not a P1, since the workaround is to just slam aria-label on the input and textarea respectively, and reuse the localized string from the label above for that. The label doesn't have an ID, so without introducing one, aria-labelledby can't be used. But the same string entity can, so aria-label works until this bug is fixed.

One question is what should happen if there's both HTML label @for and XUL label @controls. Should they both combine?

I actually don't know. Would be interesting to find out this edge case for mouse users, e. g. do both label clicks redirect focus, or does noe get ignored?

I neglected to note above that the relation code also needs to be tweaked to expose this label.

xul:label serving as a label for HTML controls feels hacky, I suspect this UI pattern may have not only accessible name issue, but it also can have other problems, like clicking a label doesn't focus HTML:input or accesskeys don't work. I'm curious why they don't switch to HTML:labels as well, they should be working in XUL documents.

(In reply to alexander :surkov (:asurkov) from comment #4)

xul:label serving as a label for HTML controls feels hacky, I suspect this UI pattern may have not only accessible name issue, but it also can have other problems, like clicking a label doesn't focus HTML:input or accesskeys don't work.

Actually, the association does cause mouse clicks on the label to focus the input or textarea. So that pattern at least works. They don't have access keys associated with these particular labels, so I don't know if those will work, too. But you're right, using html:label elements would have been more consistent. The whole system is a mix of XUL and HTML anyway, since it pulls in external information through the messaging system, too.

Worked around this via aria-labelledby, no longer blocking Skyline.

This is breaking the Library window. For example, when looking at a bookmark, the Name, Location, Tags and Keyword fields are all unlabelled. The access keys for those fields do work, so only a11y APIs are broken here.

New Bookmark/Edit Bookmark and Bookmark Properties also have broken labelling for a11y.

Marking this as a regression because even though this issue always existed, it was never a problem from a user perspective until bug 1562664.

The Tags field was regressed separately from the others in bug 1534455.

The only reason why I didn't switch to HTML labels was because the XUL labels have special access key formatting which HTML labels don't have.

A XUL <label control="id"> can refer to an HTML element.
Keyboard and mouse support for this already works, but previously, this wasn't being exposed via accessibility APIs.

1. Split the code to get the name from an associated XUL label out of Accessible::XULElmName into a new function Accessible::NameFromAssociatedXULLabel.
2. Use NameFromAssociatedXULLabel for HTMl elements.
3. Update AccessKey and RelationByType to support HTML elements with associated XUL labels.
4. Rename accessible/tests/mochitest/actions/test_keys_menu.xhtml to test_keys.xhtml so it can cover accessKey outside of menus.
5. Add tests.

Pascal, I think we'll have to revisit that WONTFIX. Maybe not for the initial 71 release, but the breakage is bad enough that I'd be willing to push for inclusion in a dot release of 71. It basically makes the bookmarks dialogs unusable for people with assistive technologies, see Jamie's comments above.

Pascal, I think we'll have to revisit that WONTFIX. Maybe not for the initial 71 release, but the breakage is bad enough that I'd be willing to push for inclusion in a dot release of 71. It basically makes the bookmarks dialogs unusable for people with assistive technologies, see Jamie's comments above.

Well, it's unfortunate that this became a P1 only a few hours ago, that tracking for 71 was not requested while the bug was open months ago and that the fix was just committed to autoland and is not in nightly yet…

Please file an uplift request now, I have a blocker that means that we will spin a RC2 later today, let's see if we can still take it. Thanks.

(In reply to Marco Zehe (:MarcoZ) from comment #14)

Comment on attachment 9112178 [details]
Bug 1572677: Correct a11y exposure for HTML elements with associated XUL labels.

Beta/Release Uplift Approval Request

• User impact if declined: Screen reader user won't be able to use the bookmarks manager, Add/Edit bookmarks dialogs any more.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None.
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only run when accessibility is active, has extensive Mochitests.
• String changes made/needed: None.

Hope this was the right flag, or should this have been approval release?

Comment on attachment 9112178 [details]
Bug 1572677: Correct a11y exposure for HTML elements with associated XUL labels.

Approval-mozilla-release, we are past betas now.

https://hg.mozilla.org/mozilla-central/rev/775b5f7f7499

(In reply to Marco Zehe (:MarcoZ) from comment #16)

Comment on attachment 9112178 [details]
Bug 1572677: Correct a11y exposure for HTML elements with associated XUL labels.

Beta/Release Uplift Approval Request

• User impact if declined: Screen reader user won't be able to use the bookmarks manager, Add/Edit bookmarks dialogs any more.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None.
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only run when accessibility is active, has extensive Mochitests.
• String changes made/needed: None.

This has landed on Central now, so it passes all tests on our infra.

Comment on attachment 9112178 [details]
Bug 1572677: Correct a11y exposure for HTML elements with associated XUL labels.

Fix for top a11y problem, patch has tests and was not backed out and not taking it seems like a major problem with assistive technologies that could lead to a dot release, approved for RC2, thanks.

https://hg.mozilla.org/releases/mozilla-release/rev/b4717df2d39f

(In reply to Pascal Chevrel:pascalc from comment #15)

Well, it's unfortunate that this became a P1 only a few hours ago, that tracking for 71 was not requested while the bug was open months ago and that the fix was just committed to autoland and is not in nightly yet…

My sincere apologies for the late p1. When this was originally opened, it was an issue encountered in a new Skyline feature which we were able to easily work around. From that point, it was (so we thought) only a theoretical issue. I wasn't aware it had user impact on an existing feature until yesterday (due to the de-XBL work), at which point I started working on a patch immediately.

Thanks for taking this despite the lateness.

So with this change, we no longer need to put aria-labelledby on the html:inputs referencing xul:label?

(In reply to Magnus Melin [:mkmelin] from comment #24)

So with this change, we no longer need to put aria-labelledby on the html:inputs referencing xul:label?

Correct.