Closed Bug 1479642 Opened Last year Closed Last year

Keyboard shortcut no longer exposed in IAccessible::accName for XUL menuitems

Categories

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

All
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- verified
firefox63 --- verified

People

(Reporter: Jamie, Assigned: Jamie)

References

Details

(Keywords: regression)

Attachments

(1 file)

STR (with the NVDA screen reader):
1. Press alt+f to open the File menu.
Expected: NVDA should report "New Tab  Ctrl+T  1 of 7"
Actual: NVDA reports "New Tab  1 of 7" (Ctrl+T is missing)

21:55.46 INFO: Last good revision: 8d8c17bc31455084017d5fa5a3ecb1c98f448cfa
21:55.46 INFO: First bad revision: 1c7fc1a1c68314f568ed25f35f885e901d17083a
21:55.47 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8d8c17bc31455084017d5fa5a3ecb1c98f448cfa&tochange=1c7fc1a1c68314f568ed25f35f885e901d17083a

So, this regression was introduced by bug 1438193, specifically part 11-4 related to Name.

On Windows, XULMenuitemAccessibleWrap::Name is overridden to append the accelerator to the name. Unfortunately, this override was missed when converting Name to const (and wasn't a compile error because it didn't use the override keyword). As such, this override stopped being called; we always call the const version, since that's the one inherited into AccessibleWrap.
Blocks: 1438193
Keywords: regression
Comment on attachment 8996169 [details]
Bug 1479642: Make Windows XULMenuitemAccessibleWrap::Name const so it overrides AccessibleWrap::Name.

https://reviewboard.mozilla.org/r/260406/#review267474

so bad, static analysis doesn't handle things like this, thank you for the fix.
Attachment #8996169 - Flags: review?(surkov.alexander) → review+
(In reply to alexander :surkov (:asurkov) from comment #2)
> so bad, static analysis doesn't handle things like this, thank you for the
> fix.

I think clang probably does warn about stuff like this, but it warns about so much right now in our tree that it's hard to spot. :)
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de9367e7a511
Make Windows XULMenuitemAccessibleWrap::Name const so it overrides AccessibleWrap::Name. r=surkov
https://hg.mozilla.org/mozilla-central/rev/de9367e7a511
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Verified fixed in Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0 ID:20180731220208.
Comment on attachment 8996169 [details]
Bug 1479642: Make Windows XULMenuitemAccessibleWrap::Name const so it overrides AccessibleWrap::Name.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1438193.
[User impact if declined]: Screen reader users will be unable to discover shortcut keys in menus; e.g. Ctrl+T for New Tab. This is one of the primary ways screen reader users learn about keyboard shortcuts and keyboard shortcuts are super important for efficiency.
[Is this code covered by automated tests?]: No, because no mechanism for Windows a11y testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only affects Windows accessibility and only in one specific area. Just changes things to call exactly the same code that was called before the regressing patch landed.
[String changes made/needed]: None.
Attachment #8996169 - Flags: approval-mozilla-beta?
Comment on attachment 8996169 [details]
Bug 1479642: Make Windows XULMenuitemAccessibleWrap::Name const so it overrides AccessibleWrap::Name.

Fix for new a11y regression in 62, let's uplift for beta 15.
Attachment #8996169 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified, that the issue is no longer reproducible on Beta 62.0b15(20180806191531), the Menu keyboard shortcuts are read by the NVDA screen reader.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.