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

Status: VERIFIED FIXED

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

Description

[James Teh [:Jamie]]

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.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1479642: Make Windows XULMenuitemAccessibleWrap::Name const so it overrides AccessibleWrap::Name.

This again exposes the accelerator (e.g. Ctrl+T for New Tab) for menu items via IAccessible::accName on windows.

Bug 1438193 made Accessible::Name const.
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 called the const version, since that's the one inherited into AccessibleWrap.
Making this const causes this override to be called.

Review commit: https://reviewboard.mozilla.org/r/260406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260406/

Comment 2

[alexander :surkov (:asurkov)]

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.

Comment 3

[James Teh [:Jamie]]

(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. :)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de9367e7a511
Make Windows XULMenuitemAccessibleWrap::Name const so it overrides AccessibleWrap::Name. r=surkov

Comment 5

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/de9367e7a511

Comment 6

[James Teh [:Jamie]]

Verified fixed in Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0 ID:20180731220208.

Comment 7

[James Teh [:Jamie]]

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.

Comment 8

[Liz Henry (:lizzard)]

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.

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d3320ef71ef2

Comment 10

[Gyula Palko[:gpalko]]

Verified, that the issue is no longer reproducible on Beta 62.0b15(20180806191531), the Menu keyboard shortcuts are read by the NVDA screen reader.