Closed Bug 1455379 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XBL, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: zbraniecki, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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?
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: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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 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?
(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 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+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/edde2964d8bc
ignore key elements without a key attribute, r=bz,gandalf
https://hg.mozilla.org/mozilla-central/rev/edde2964d8bc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.