Closed Bug 1578140 Opened 3 months ago Closed 3 months ago

Accessible::DoAction fails on XULLabelAccessible inside a button

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox70 --- verified
firefox71 --- verified

People

(Reporter: Jamie, Assigned: Jamie)

References

Details

(Whiteboard: [skyline] [access-p1])

Attachments

(1 file)

STR (with the NVDA screen reader):

  1. Open https://mozilla.org/
  2. Press alt+d, shift+tab to focus the tracking protection icon.
  3. Press space to open the Protections panel.
  4. Press b several times to focus "Protection Settings".
  5. Press enter.
    • Expected: Protection settings should open.
    • Actual: Nothing happens.

This button (along with several others in that panel) is a XUL toolbarbutton containing a XUL label:
https://searchfox.org/mozilla-central/rev/9415ecf29ba1acbef9381335e0ecde5916ca4073/browser/components/controlcenter/content/protectionsPanel.inc.xul#128
The a11y tree reflects this; i.e. there's a XULLabelAccessible inside a XULToolbarButtonAccessible.

When you press enter, NVDA, as it always does, starts from the deepest accessible (the label). Because there's a click action, NVDA executes it (Accessible::DoAction).
This does call the click handler on the XUL label, which does nothing because this isn't for another control. This then bubbles up to the button. However, the button has a command handler, not a click handler, and that never gets run.

I guess the way that a11y simulates a click (Accessible::DispatchClickEvent) doesn't trigger this command handler, probably because of the click handler on the label. Interestingly, calling .click() on the label from javascript does work. Also, some brief playing suggests that really clicking the button with the mouse doesn't trigger the label's click handler. I'm not sure how this is happening.

I think the simplest fix here is probably just to override XULLabelAccessible to use the equivalent of JS .click(). Ideally, I'd like to know why this is misbehaving and/or fix the root cause, but given that this is XUL specific and the age of some of this XUL stuff, I'm not sure it's worth going down the rabbit hole.

For labels inside buttons, The base implementation of DispatchClickEvent doesn't fire a command event on the button.
To fix this, override DispatchClickEvent to use nsXULElement::Click, which does fire a command event on the button.

Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68678ee894cb
Use nsXULElement::Click for DoAction on XULLabelAccessible so it works for labels inside buttons. r=eeejay
Regressions: 1578604
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Jamie, please request uplift to 70 for this.

Flags: needinfo?(jteh)
Whiteboard: [skyline] → [skyline] [access-p1]

Comment on attachment 9089938 [details]
Bug 1578140: Use nsXULElement::Click for DoAction on XULLabelAccessible so it works for labels inside buttons.

Beta/Release Uplift Approval Request

  • User impact if declined: Buttons in the Skyline Protections panel will be unusable by 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: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Only affects accessibility actions on XUL labels. Covered by automated tests.
  • String changes made/needed: None.
Flags: needinfo?(jteh)
Attachment #9089938 - Flags: approval-mozilla-beta?

Verified fixed in Firefox 71 nightly 20190916155843 on Windows.

Comment on attachment 9089938 [details]
Bug 1578140: Use nsXULElement::Click for DoAction on XULLabelAccessible so it works for labels inside buttons.

a11y improvement for the Protections Panel. Approved for 70.0b8.

Attachment #9089938 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

James, can you please verify that this is fixed on 70.0b8 as well? https://archive.mozilla.org/pub/firefox/candidates/70.0b8-candidates/build1/

Flags: needinfo?(jteh)

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #9)

James, can you please verify that this is fixed on 70.0b8 as well? https://archive.mozilla.org/pub/firefox/candidates/70.0b8-candidates/build1/

Marco, would you mind verifying this? See the STR in comment 0.

Flags: needinfo?(jteh) → needinfo?(mzehe)

Verified fixed in Firefox 70.0b8 as well.

Flags: needinfo?(mzehe)

Thanks so much! Closing this bug as verified.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.