Closed Bug 371900 Opened 15 years ago Closed 9 days ago

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

Categories

(Core :: XUL, defect)

defect

Tracking

()

RESOLVED FIXED
98 Branch
Tracking Status
firefox98 --- fixed

People

(Reporter: surkov, Assigned: enndeakin)

References

(Blocks 5 open bugs)

Details

Attachments

(4 files, 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

The edit-related commands are special because they are handled by ShortcutKeyDefinitions.cpp yet we have duplicate keys because we want the menu disabled state to update properly, so we don't fire command events on those.

Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
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
Status: ASSIGNED → RESOLVED
Closed: 12 days ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch
Regressions: 1749182
Target Milestone: 97 Branch → ---
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
No longer blocks: 977512
Duplicate of this bug: 977512
No longer blocks: 961524
See Also: → 961524
No longer blocks: 1195060
See Also: → 1195060
Severity: normal → --
Status: REOPENED → RESOLVED
Closed: 12 days ago9 days ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
Regressions: 1749667
You need to log in before you can comment on or make changes to this bug.