1297993 - Warnings for "key events not available" should include the key ID

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Steps to reproduce:
On Windows 10 Nightly 2016-08-23 51.0a1
Open Nightly
Open the Browser Console
See the following warnings:
Key event not available on some keyboard layouts: key=“c” modifiers=“accel,alt”
Key event not available on some keyboard layouts: key=“r” modifiers=“accel,alt”
Key event not available on some keyboard layouts: key=“i” modifiers=“accel,alt,shift”

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

The first warning was just removed by https://hg.mozilla.org/mozilla-central/rev/0860fa85235a so that doesn't apply anymore.

The second warning was introduced by https://hg.mozilla.org/mozilla-central/rev/c2849a432eee because on Windows this translates to Ctrl+Shift+R which is also used for Browser:ReloadSkipCache.

Comment 2

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> The second warning was introduced by
> https://hg.mozilla.org/mozilla-central/rev/c2849a432eee because on Windows
> this translates to Ctrl+Shift+R which is also used for
> Browser:ReloadSkipCache.

I'm confused. Accel-alt-r is ctrl-alt-r - why would that translate to ctrl-shift-r ?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

The third warning comes from https://hg.mozilla.org/integration/fx-team/rev/9b9b8514c6b0479d470b0c30f84b6e9552591da5

I've updated nsXBlPrototypeHandler.cpp to include the key ID to help with tracking down these regressions.

(In reply to :Gijs Kruitbosch (away 26-29 incl.) from comment #2)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> > The second warning was introduced by
> > https://hg.mozilla.org/mozilla-central/rev/c2849a432eee because on Windows
> > this translates to Ctrl+Shift+R which is also used for
> > Browser:ReloadSkipCache.
> 
> I'm confused. Accel-alt-r is ctrl-alt-r - why would that translate to
> ctrl-shift-r ?

Yes, you are right. I don't see an actual key conflict here in my usage. The key conflict detection code is very primitive, it will warn about anything with only ctrl and/or alt as the keyboard modifiers on Windows.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1297993 - Include the ID of the offending <key> element in the key conflict message.

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

On Windows, there is no AltGr modifier state. Instead, Windows represents its state with Ctrl+Alt. Therefore, nsXBLPrototypeHandler warns such shortcut key handlers because if the combination causes text input, both Ctrl and Alt modifier state are not set to true by widget. Therefore, with some keyboard layouts, Ctrl+Alt+[A-Z] may not be available.

# Sorry for the delay to review due to my business trip and having been sick yesterday.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 8784799 [details]
Bug 1297993 - Include the ID of the offending <key> element in the key conflict message.

https://reviewboard.mozilla.org/r/74110/#review73412

::: dom/locales/en-US/chrome/layout/xbl.properties:6
(Diff revision 1)
>  # LOCALIZATION NOTE: do not localize key=“%S” modifiers=“%S”
> -GTK2Conflict=Key event not available on GTK2: key=“%S” modifiers=“%S”
> -WinConflict=Key event not available on some keyboard layouts: key=“%S” modifiers=“%S”
> +GTK2Conflict2=Key event not available on GTK2: key=“%S” modifiers=“%S” id=“%S”
> +WinConflict2=Key event not available on some keyboard layouts: key=“%S” modifiers=“%S” id=“%S”

Do you really need to change the key names of these resource? (If you did it for easier to review, I'm really happy, though.) IIRC, l10n tools detect which keys' values are changed even without changing the key names...

And please add id=%S to the "LOCALIZATION NOTE:" too.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for the review and for catching the localization note change. I changed the keys because https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Updating_Entity_Names says that l10n tools do not detect key value changes and that they only look at key names.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/286a80b93223
Include the ID of the offending <key> element in the key conflict message. r=masayuki

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/286a80b93223

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

This varies by platform. I'm not sure what more can be done but at least the patch that landed helps make debugging this easier. I'm going to close this bug now and file a new bug since this patch has landed quite some time ago.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Filed bug 1303876 for the follow-up.