1864606 - "Allow Nightly to send backlogged crash reports on your behalf" check box has no accessibility label

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P2)

Description

[Catalin Sasca, Desktop QA [:csasca]]

Attachment: voiceover issue.png

Found in

• Firefox 121.0a1

Affected versions

• Firefox 120.0
• Firefox 121.0a1

Tested platforms

• Affected platforms: macOS 13.6.1
• Unaffected platforms: Windows, Ubuntu

Preconditions

• Have VoiceOver active

Steps to reproduce

1. Launch Firefox
2. Access about:preferences#privacy
3. Tab through the page to "Firefox/Nightly Data Collection and Use" category
4. Tab to "Allow Firefox to send backlogged crash reports on your behalf"

Expected result

• The option is read by VoiceOver like the other ones before

Actual result

• VoiceOver only says "unticked, checkbox"

Regression range

• Will see if this is a regression

Additional notes

• The issue can be seen in the attachment.

Comment 1

[James Teh [:Jamie]]

Maybe another AXTitle/AXDescription quirk?

Comment 2

[Alexandru Trif, Desktop QA [:atrif]]

Hello! I have made a regression range:
Last good revision: c4bd8fe7ee285cbd80419f23f175053921fe5cd9 (2021-05-02)
First bad revision: c97286566c4529deeb7c1da7ce593f83b0699992 (2021-05-03)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c4bd8fe7ee285cbd80419f23f175053921fe5cd9&tochange=c97286566c4529deeb7c1da7ce593f83b0699992

Unfortunately, I don't know which bug is the regressor from the above list. Also, it seems this is reproducible on Windows 10x64 with NVDA and Ubuntu 22 with Orca.

Comment 3

[James Teh [:Jamie]]

This is a problem in the Preferences code. Here is the problematic control. The check box is an HTML input. There is a label and the label does have a for attribute, but it's a XUL label, not an HTML label, and the for attribute is not valid on an XUL label. Initially, I was confused as to why clicking the label works fine if the for attribute isn't valid, but it turns out that there is JS code for this specific label.

I don't know why it has been implemented this way. If it's not possible to use an HTML label, a XUL checkbox, etc., the label should be associated with aria-labelledby. Even then, this isn't entirely desirable because the Learn more link is inside the label, which shouldn't be part of the accessibility label. If aria-labelledby is used here, it should only refer to the text of the label itself (e.g. in an HTML span), not the Learn more link.

Comment 4

[Molly Howell (she/her) (no longer active)]

Setting severity to match accessibility severity. I'm also going to have a go at fixing this myself, it's bugging me.

Comment 5

[Molly Howell (she/her) (no longer active)]

Attachment: Bug 1864606 - Standardize the backlogged crash reports checkbox implementation.

This checkbox has some, I'll call it "bespoke" features of its implementation,
which, as far as I can tell, were done the way they were for reasons which no
longer apply [1]. So I've undone that and made this work more like the rest of our
checkboxes do, which also happens to fix the bug.

[1] I cannot be entirely certain of this because I don't feel like I fully
understand what those reasons were, but I did check for bug 1703792, which was
the most recent thing that seems to have triggered a lot of changes here, and
this patch does not appear to have brought it back.

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Marking as P2, since Molly is no longer active

Comment 7

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

:jhirsch could we find a new assignee for this given that it is a P2/S2?

Comment 8

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Sorry, busy week! I'll take this one

Comment 9

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

:jhirsh just as a reminder, string freeze is friday! so this has potential of missing that train as well ..

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Thanks for the ping. I'm too busy with other work, but I'll see if someone else can pick this up.

Comment 11

[Tim Giles [:tgiles]]

I have some time, I'll see if I can get a quick fix.

Comment 12

[Tim Giles [:tgiles]]

Attachment: Bug 1864606 - Fix missing label association for crash reports checkbox r=#settings-reviewers,#fluent-reviewers

Thanks to mhowell for the STRIP_ANCHOR function in the Fluent migration.

We were using the "for" attribute on a XUL label which does not behave
like an HTML label. This prevented the programmatic association
between the checkbox and the label. By utilizing moz-label, we maintain
the accesskey behavior of the previous XUL label.

Additionally we use moz-support-link to remove the
"collection-backlogged-crash-reports-link" Fluent string.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5114114b454d
Fix missing label association for crash reports checkbox r=fluent-reviewers,Gijs,bolsson

Comment 14

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/5114114b454d

Comment 15

[bhidecuti]

I've managed to reproduce the issue on an affected Nightly build from 2023-11-14 on macOS 12.6.6.
Verified this as fixed on Firefox 125.0b5 (20240327092250) on the same OS. "Allow Firefox to send backlogged crash reports on your behalf" check box is correctly read by the VoiceOver.

Comment 16

[bhidecuti]

Verified this as fixed on Firefox 125.0b6 (20240329091918) on Windows 11 and Ubuntu 22.04 as well. "Allow Firefox to send backlogged crash reports on your behalf" check box is correctly read by NVDA (Windows)/Orca (Ubuntu).