331290 - xul <key> handling doesn't fire a command event

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Brian Ryner (not reading)]

nsXBLPrototypeHandler::ExecuteHandler just finds the command element and executes its script directly.  It really should fire a command event so that this follows a similar code path to <button command="cmdFoo"/>.  This would make it possible to use an event listener to watch for <key>-triggered commands.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So this is the case when isXBLCommand and isReceiverCommandElement are both true?  Or the case when isReceiverCommandElement is false?

Comment 2

[[e-mail]]

(In reply to comment #1)
>So this is the case when isXBLCommand and isReceiverCommandElement are both
>true?  Or the case when isReceiverCommandElement is false?
This has nothing to do with isXBLCommand (that's for platformHTMLBindings.xml), this is for the isXULKey case; isReceiverCommandElement may be true or false.
Also I think this case is specific to the call from WalkHandlersInternal.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  So for isXULKey, this would be the case when the "oncommand" attr is empty and isReceiverCommandElement is true?

Put another way, which node should we be firing the event on?

Comment 4

[[e-mail]]

The oncommand attribute cannot currently be empty, otherwise nothing would ever get executed. That is the point of this bug...

The event should be fired on the receiver. The caller has already computed that. What I don't know is why the caller didn't fire the command event itself. Note that the event should be fired in a way that preserves the modifiers.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> The oncommand attribute cannot currently be empty

So what's the code at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp&rev=1.101&mark=378,380-382#375 doing?  Is the stuff inside that IsEmpty() if check just dead code?

Comment 6

[[e-mail]]

Sorry, I meant that from a XUL developer's point of view. Perhaps I should have said that the oncommand attribute currently cannot usefully be empty?

Comment 7

[Brian Ryner (not reading)]

This should apply both to:

<key oncommand="..."/>

and

<key command="..."/>

What would make the most sense to me, in either case, is just to fire a command event at the key element once we've computed that it matches in WalkHandlersInternal (rather than calling ExecuteHandler).  nsXULElement::PreHandleEvent will take care of retargeting it in the command= case.  For the oncommand= case, I'm assuming that whatever generic code makes onfoo="blah();" install an event handler will have set one up for "command" on the <key> element.

Does that sound workable?

Comment 8

[[e-mail]]

Sounds good to me. I had a peek at ExecuteHandler myself but wasn't quite sure which bits were used for <keys>, which bits weren't, and which were shared.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Sounds reasonable to me too.

Comment 10

[Brian Ryner (not reading)]

Attachment: patch

Comment 11

[Brian Ryner (not reading)]

checked in on trunk

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 220863 [details] [diff] [review]
patch

What do you think about taking this one on the branch?  I'd really like to have it there for bug 328069, and it should be pretty safe... maybe we should just give it a few days to shake out any regressions?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'd give it a week or week and a half on trunk, then see how things look...  This code scares me, mostly because I don't understand it as well as I probably should.

Comment 14

[[e-mail]]

(In reply to comment #4)
>Note that the event should be fired in a way that preserves the modifiers.

Comment 15

[[e-mail]]

... e.g. the SpaceHit function is called for both space and shift+space events and needs to know which modifiers were pressed; also for consistency with everywhere else that we generate command events from UI events.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 220863 [details] [diff] [review]
patch

I've verified (by local backout) that this causes blocker bug 336740.  Given that and Neil's comments....

Comment 17

[Brian Ryner (not reading)]

ok, regression fixes are in, marking this FIXED again.

Comment 18

[Brian Ryner (not reading)]

Attachment: revised patch for branch

This has the regression fixes from 336740, and I backported it to use HandleDOMEvent() for dispatching the event.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 221736 [details] [diff] [review]
revised patch for branch

>Index: content/xbl/src/nsXBLPrototypeHandler.cpp
>+      // if the focused element is a link then we do want space to 
>+     // scroll down. focused element may be an element in a 

Fix the indent?

a=bzbarsky for the branch.

Comment 20

[Brian Ryner (not reading)]

checked in on branch