alexander :surkov (:asurkov) Reporter
	Description • 17 years ago

I guess it's related with bug 331290. It looks the reason is if @oncommand is missed then key is not processed properly.

See branch 1.8

http://lxr.mozilla.org/mozilla1.8/source/content/xbl/src/nsXBLWindowHandler.cpp#335

or trunk
http://lxr.mozilla.org/mozilla/source/content/xbl/src/nsXBLWindowKeyHandler.cpp#533

	neil@parkwaycc.co.uk
	Comment 1 • 17 years ago

As I recall, this was done deliberately. Before bug 331290 the key handling code did the equivalent of this:
for (var i = 0; i < keyset.childNodes.length; ++i) {
  var target = keyset.childNodes[i];
  if (this.keyEventMatches(target)) {
    if (target.hasAttribute("command"))
      target = document.getElementById(target.getAttribute("command"));
    if (target && target.hasAttribute("oncommand"))
      new Function(target.getAttribute("oncommand")).call(event.target, event);
  }
}

Note that a <keyset> didn't check that its child nodes were keys, so that if it found a random node (or more typically, a nested keyset) then the keyEventMatches code would accept this as a "catch-all" key and only didn't process it because the target had no oncommand attribute.

While this bug has been fixed, there is another issue in that some of the keys are actually processed by XBL (such as cut and paste). The key elements still exist so that the menu accelerator texts can be computed. However these dummy keys also have no oncommand attribute.

	alexander :surkov (:asurkov) Reporter
	Comment 2 • 17 years ago

(In reply to comment #1)

> Note that a <keyset> didn't check that its child nodes were keys, so that if it
> found a random node (or more typically, a nested keyset) then the
> keyEventMatches code would accept this as a "catch-all" key and only didn't
> process it because the target had no oncommand attribute.

If keyset contains keyset then I think nested keyset shouldn't have 'command' attribute to obtain xul:command element. Therefore checking for @oncommand on xul:command shouldn't be happen.

> While this bug has been fixed, there is another issue in that some of the keys
> are actually processed by XBL (such as cut and paste). The key elements still
> exist so that the menu accelerator texts can be computed. However these dummy
> keys also have no oncommand attribute.
> 

Sorry, xul:key or xul:command?

	neil@parkwaycc.co.uk
	Comment 3 • 17 years ago

(In reply to comment #2)
>(In reply to comment #1)
>>Note that a <keyset> didn't check that its child nodes were keys, so that if it
>>found a random node (or more typically, a nested keyset) then the
>>keyEventMatches code would accept this as a "catch-all" key and only didn't
>>process it because the target had no oncommand attribute.
>If keyset contains keyset then I think nested keyset shouldn't have 'command'
>attribute to obtain xul:command element. Therefore checking for @oncommand on
>xul:command shouldn't be happen.
What I was saying that this behaviour was deliberately retained. I guess you want to move the attribute check to inside the if (!commandElt) block? The alternative is to remove the attribute check and add disabled="true" to all the relevant <key>s (see below for an example) instead.

>>While this bug has been fixed, there is another issue in that some of the keys
>>are actually processed by XBL (such as cut and paste). The key elements still
>>exist so that the menu accelerator texts can be computed. However these dummy
>>keys also have no oncommand attribute.
>Sorry, xul:key or xul:command?
Here's an example of a key that needs to be ignored:
http://lxr.mozilla.org/seamonkey/source/mail/base/content/utilityOverlay.xul#77

	alexander :surkov (:asurkov) Reporter
	Comment 4 • 17 years ago

(In reply to comment #3)
> (In reply to comment #2)
> >(In reply to comment #1)
> >>Note that a <keyset> didn't check that its child nodes were keys, so that if it
> >>found a random node (or more typically, a nested keyset) then the
> >>keyEventMatches code would accept this as a "catch-all" key and only didn't
> >>process it because the target had no oncommand attribute.
> >If keyset contains keyset then I think nested keyset shouldn't have 'command'
> >attribute to obtain xul:command element. Therefore checking for @oncommand on
> >xul:command shouldn't be happen.
> What I was saying that this behaviour was deliberately retained. I guess you
> want to move the attribute check to inside the if (!commandElt) block?

Yes, it looks correctly. Can I go ahead by this way?

> >>While this bug has been fixed, there is another issue in that some of the keys
> >>are actually processed by XBL (such as cut and paste). The key elements still
> >>exist so that the menu accelerator texts can be computed. However these dummy
> >>keys also have no oncommand attribute.

> Here's an example of a key that needs to be ignored:
> http://lxr.mozilla.org/seamonkey/source/mail/base/content/utilityOverlay.xul#77
> 

That is probably is out of box but why these keys are dummy, do they have related real xul:command element? And why are they needed? Can you show mechanism of their work?

	neil@parkwaycc.co.uk
	Comment 5 • 17 years ago

(In reply to comment #4)
>(In reply to comment #3)
>>I guess you want to move the attribute check to inside the if (!commandElt) block?
>Yes, it looks correctly. Can I go ahead by this way?
I've got no particular objection, but I'm not the module owner ;-)

>>Here's an example of a key that needs to be ignored:
>>http://lxr.mozilla.org/seamonkey/source/mail/base/content/utilityOverlay.xul#77
>That is probably is out of box but why these keys are dummy, do they have
>related real xul:command element? And why are they needed? Can you show
>mechanism of their work?
They are needed so that the key labels show up correctly in the menuitems,
but they are actually handled in the content/xbl/builtin files.

	neil@parkwaycc.co.uk
	Comment 6 • 10 years ago

Actually the nested <keyset> issue is bogus, that was actually already fixed in bug 336740. There might still be an issue regarding the dummy edit menu keys though.

	neil@parkwaycc.co.uk
	Comment 8 • 10 years ago

(In reply to comment #6)
> Actually the nested <keyset> issue is bogus, that was actually already fixed
> in bug 336740. There might still be an issue regarding the dummy edit menu
> keys though.

In fact I already said that in comment #1.

(In reply to alexander surkov from comment #4)
> (In reply to comment #3)
> > I guess you want to move the attribute check to inside the if (!commandElt) block?
> 
> Yes, it looks correctly. Can I go ahead by this way?

Didn't seem to cause any test failures when I tried.

	Anthony Shipman
	Comment 9 • 10 years ago

I think I've just been bitten by this bug in FF 28. I'm creating XUL key and command elements from JS.
I want the command event for shortcuts to bubble up to the window.

If I create a <command> element and write
   cmd.addEventListener("command", ...)
no command event is generated.

If I write
   cmd.oncommand = "alert('hi');";
no command event is generated.

If I write
   cmd.setAttribute("oncommand", "var x = true;");
then a command event is generated. The dummy JS is needed
for this to work. (The 'var x' is needed to silence "use strict" complaints).

It looks like the test for the presence of command listeners 
is faulty.

	James Long (:jlongster)
	Comment 10 • 9 years ago

Just ran into this...

	Pulsebot
	Comment 14 • 2 years ago

Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6434bce9c7d4
always fire a command event on key elements except for those that are marked not to, r=masayuki
https://hg.mozilla.org/integration/autoland/rev/e59035c65af3
remove more now-unneeded empty oncommand attributes, r=bgrins,preferences-reviewers,Gijs

	Pulsebot
	Comment 19 • 2 years ago

Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d45cfb68e9b
always fire a command event on key elements except for those that are marked not to, r=masayuki
https://hg.mozilla.org/integration/autoland/rev/aeff4d2d703f
remove more now-unneeded empty oncommand attributes, r=bgrins,preferences-reviewers,Gijs
https://hg.mozilla.org/integration/autoland/rev/0f4d001ba7fb
delete command is not handled in ShortcutKeyDefinitions.cpp so does not need to be defined as internal, r=mak