Open Bug 371900 Opened 13 years ago Updated 3 days ago

xul <key> handling doesn't fire a command event if @oncommand is missed

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

People

(Reporter: surkov, Unassigned)

References

(Blocks 4 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

Attached file testcase
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
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.
(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?
(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
(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?
(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.
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Blocks: abp
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.
Duplicate of this bug: 547189
Blocks: 961524
Blocks: 978115
(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.
Blocks: 993850
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.
Just ran into this...
Blocks: 1195060
Attachment #9009569 - Attachment is obsolete: true
Attachment #256593 - Attachment mime type: application/vnd.mozilla.xul+xml → text/plain
See Also: → 1550061
You need to log in before you can comment on or make changes to this bug.