1297342 - "reserved" attribute should be on <key> elements instead of <command> ones

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[:Gijs (he/him)]

It's not clear to me why the "reserved" attribute was added on command elements instead of <key> ones. AIUI, because the keys are the things we're actually "blocking" content from seeing / getting, it would make more sense to have the attribute directly on those elements than via 1 level of indirection on the <command> they refer to. The current implementation also means that you need that level of indirection, whereas without it you can just use an oncommand attribute on a <key> element without having to create a <command> element. This hurt us in bug 1296863 when we tried to detach the accel-t binding from the <command> element to make some of the code a bit saner.

Masayuki-san, Felipe, do either of you know why this lives on <command> elements instead?

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't know, I was involved when we got some bug reports about the implementation.

I think that available either <command> or <key> is better.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

I don't recall if there was a solid reasoning or if it was just implementation simplicity. Trying to back-fill a reason, I believe that it's the action that should be reserved (e.g. "it shouldn't be possible to prevent a user from creating a new tab") instead of the key ("it shouldn't be possible to prevent a user from using ctrl + t").

But if changing it will make things better I'm not opposed.

Comment 3

[Dão Gottwald [:dao]]

A <key> is always associated with an action, so I fail to see how putting the attribute on the <command> provides any benefit.

Comment 4

[Neil Deakin]

Attachment: Move reserved attribute to keys

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8825796 [details] [diff] [review]
Move reserved attribute to keys

Review of attachment 8825796 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks sane to me. But I know that a lot of this reserved handling has changed since the first version to deal with the non-e10s case, so I'm not 100% if this is enough. Did you test both on e10s and non-e10s?

Comment 6

[Neil Deakin]

> Did you test both on e10s and non-e10s?

Yes, it works in both.

Comment 7

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f14ba5415dd0

Comment 8

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=174d6ff40d6a

Comment 9

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=440381c54a8e

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8db5ac137c5
reserved attribute should be on key element not on the command, r=felipe

Comment 11

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

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