1484914 - Access key in context menu appears duplicated

Status: VERIFIED FIXED

(WebExtensions :: General, defect)

Description

[Francesco Lodolo [:flod]]

Attachment: screenshot.png

Example add-on
https://bugzilla.mozilla.org/attachment.cgi?id=9002655

Comment 1

[Francesco Lodolo [:flod]]

My assumption is that the code doesn't account for intl.menuitems.alwaysappendaccesskeys, which is true in Japanese and other languages
https://hg.mozilla.org/l10n/gecko-strings/file/default/toolkit/chrome/global/intl.properties#l47
https://transvision.mozfr.org/string/?entity=toolkit/chrome/global/intl.properties:intl.menuitems.alwaysappendaccesskeys

Comment 2

[Rob Wu [:robwu]]

This happens on Windows and Linux only (and not on macOS, where access keys aren't being displayed in the menu item).

I'll fix this by stripping " (&x)" from the end of the title, under the following conditions:
- intl.menuitems.alwaysappendaccesskeys is true
- Platform is Windows or Linux.
- "x" is the access key.

I have tried hard to write automated tests, but will not an automated regression test since the relevant logic resides at the layout/ layer, and the internal mTitle is set but not available through JS.
  https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/layout/xul/nsTextBoxFrame.cpp#833-885
- In debug builds, there exist a GetFrameName method to retrieve the internal mTitle:
  https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/layout/xul/nsTextBoxFrame.cpp#1192-1200
- That method is used by various C++ test functions, but a long shot from being exposed to JS. (And even if it were, it is a layout frame, not the XUL element itself).

Comment 3

[Rob Wu [:robwu]]

Actually, rather than trying to detect the cases where the key would be duplicated followed by stripping, I am going to check whether the suffix already exists and not duplicate it again if present.

bz seems to have reviewed some patches to nsTextBoxFrame::UpdateAccessTitle, so I'm going to push another review in his queue for when he returns from PTO.

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1484914 - Never append duplicate access key hints

On macOS, access keys are not visually observable.

On Linux and Windows, access keys are underlined.
Moreover, if intl.menuitems.alwaysappendaccesskeys is set, the access
key is always appended as "(X)", optionally preceded by a space, where X
is the uppercased version of the access key.

This commit updates the logic to not append a new "(X)" if this expected
suffix is already present at the end of the label.

Comment 5

[Kazumasa Hasegawa (Kazz)]

Hi,
Is the patch requested for review?
If possible, I hope it will be fixed in Firefox 63.
Thanks.

Comment 6

[Rob Wu [:robwu]]

I have asked bz for review, but I'm not sure if he sees the notification since I added the review request before the Phabricator account was re-enabled.

Comment 7

[kernp25]

(In reply to Rob Wu [:robwu] from comment #6)
> I have asked bz for review, but I'm not sure if he sees the notification
> since I added the review request before the Phabricator account was
> re-enabled.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9003192 [details]
Bug 1484914 - Never append duplicate access key hints

Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/964610ef7e3f
Never append duplicate access key hints r=bzbarsky

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/964610ef7e3f

Comment 11

[CosminB]

Attachment: Bug1484914.gif

I was able to reproduce this issue on Firefox 63.0a1.ja and .en-US (20180829100131) under Win 7 64-bit and Ubuntu 14.04 LTS.

This issue is verified as fixed on Firefox 63.0a1.ja and .en-US (20180902220539) under Win 7 64-bit, Mac OS X 10.13.3 and Ubuntu 14.04 LTS.

Please see the attached video.