1455379 - Make <key> equivalent to <key key="">

Status: RESOLVED FIXED

(Core :: XBL, enhancement)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

In bug 1454323 we hit a scenario when due to l10n fallback XUL registered the <key> element before its localization was populated in DOM.

That results in the registration of <key> which seems to capture all command keys taking over cmd+t, cmd+q etc.

But when we set it to `<key key="" data-l10n-id="...">` then the key doesn't takeover all the other command keys.

I'd like to switch the default behavior to the same as for `key=""` to avoid having to populate empty `key=""` in each <key> controlled by Fluent.

Now, here come famous last words:
I hope there's no way that any code depends on that behavior, right?

Comment 1

[:Gijs (he/him)]

Funny thing, this code seems to actually live in XBL. :-)

I think I have a patch, just doing a sanity check...

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1455379 - ignore key elements without a key attribute,

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

Comment 3

[:Gijs (he/him)]

I tested the patch by removing the data-l10n-id attribute from the key in preferences.xul. Pre-patch, this broke using ctrl-t to open a tab on my windows machine when the prefs were open. Post-patch, this worked correctly. Note that some shortcuts did continue working even when the prefs were open. Not really clear on why that would be.

On a more technical note in the patch, I wonder if I need to initialize the nsAutoString before using? The attached patch compiles and works fine for me on Windows, but I'm paranoid and got lost in string headers trying to find a definition to resolve the question...

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8969627 [details]
Bug 1455379 - ignore key elements without a key attribute,

https://reviewboard.mozilla.org/r/238420/#review244248

that looks great! Thank you Gijs!

Comment 5

[Brian Grinstead [:bgrins]]

Curious - why is this implemented in xbl/ instead of xul/? And would removing platformHTMLBindings.xml (as in Bug 1419091) make it easier to decouple this from XBL?

Comment 6

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Curious - why is this implemented in xbl/ instead of xul/?
I don't know, I just noticed it was...


> And would
> removing platformHTMLBindings.xml (as in Bug 1419091) make it easier to
> decouple this from XBL?

I don't know that, either. Sorry!

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8969627 [details]
Bug 1455379 - ignore key elements without a key attribute,

https://reviewboard.mozilla.org/r/238420/#review244274

::: dom/xbl/nsXBLWindowKeyHandler.cpp:214
(Diff revision 1)
> -    bool attrExists =
> +    // Hopefully at least one of the attributes is set:
> -      keyElement->GetAttr(kNameSpaceID_None, nsGkAtoms::key, valKey) ||
> +    keyElement->GetAttr(kNameSpaceID_None, nsGkAtoms::key, valKey) ||
>        keyElement->GetAttr(kNameSpaceID_None, nsGkAtoms::charcode, valCharCode) ||
>        keyElement->GetAttr(kNameSpaceID_None, nsGkAtoms::keycode, valKeyCode);
> -    if (attrExists &&
> -        valKey.IsEmpty() && valCharCode.IsEmpty() && valKeyCode.IsEmpty())
> +    // If not, ignore this key element.
> +    if (valKey.IsEmpty() && valCharCode.IsEmpty() && valKeyCode.IsEmpty())

Please brace the body (I know this was preexisting).

Comment 8

[:Gijs (he/him)]

Comment on attachment 8969627 [details]
Bug 1455379 - ignore key elements without a key attribute,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238420/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edde2964d8bc
ignore key elements without a key attribute, r=bz,gandalf

Comment 10

[Andreea Pavel [:apavel]]

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