Closed
Bug 1455379
Opened 6 years ago
Closed 6 years ago
Make <key> equivalent to <key key="">
Categories
(Core :: XBL, enhancement)
Core
XBL
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?
Assignee | ||
Comment 1•6 years 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•6 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years 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•6 years 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+
Comment 5•6 years ago
|
||
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•6 years 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•6 years 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) |
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/edde2964d8bc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•