Closed Bug 331290 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XUL, defect, P1)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bryner, Assigned: bryner)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files)

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.
So this is the case when isXBLCommand and isReceiverCommandElement are both true?  Or the case when isReceiverCommandElement is false?
(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.
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?
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.
> 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?
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?
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?
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.
Sounds reasonable to me too.
Attached patch patchSplinter Review
Assignee: nobody → bryner
Status: NEW → ASSIGNED
Attachment #220863 - Flags: superreview?(bzbarsky)
Attachment #220863 - Flags: review?(bzbarsky)
Attachment #220863 - Flags: superreview?(bzbarsky)
Attachment #220863 - Flags: superreview+
Attachment #220863 - Flags: review?(bzbarsky)
Attachment #220863 - Flags: review+
checked in on trunk
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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?
Attachment #220863 - Flags: approval-branch-1.8.1?(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.
(In reply to comment #4)
>Note that the event should be fired in a way that preserves the modifiers.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
... 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.
Depends on: 336740
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....
Attachment #220863 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1-
Priority: -- → P1
ok, regression fixes are in, marking this FIXED again.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
This has the regression fixes from 336740, and I backported it to use HandleDOMEvent() for dispatching the event.
Attachment #221736 - Flags: approval-branch-1.8.1?(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.
Attachment #221736 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
checked in on branch
Keywords: fixed1.8.1
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.