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

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gandalf, Assigned: Gijs)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

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?
(Reporter)

Updated

a year ago
Blocks: 1365426
(Assignee)

Comment 1

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

I think I have a patch, just doing a sanity check...
Component: XUL → XBL
(Assignee)

Updated

a year ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
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...
(Reporter)

Comment 4

a year ago
mozreview-review
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!
Attachment #8969627 - Flags: review?(gandalf) → review+
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?
(Assignee)

Comment 6

a year ago
(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

a year ago
mozreview-review
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).
Attachment #8969627 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)

Comment 9

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/edde2964d8bc
ignore key elements without a key attribute, r=bz,gandalf

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edde2964d8bc
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.