When a command event is dispatched to a XUL element which has a command= attribute, it's retargeted to the node pointed to by that attribute (see http://lxr.mozilla.org/seamonkey/source/content/xul/content/src/nsXULElement.cpp#1676). The way that this retargeting works means that there's no way, given the command event, to figure out what the "source" of it was. I think we should add a way to do this, it's very useful if you're trying to track event flow. This type of retargeting is different from any kind of anonymous-content retargeting we do (mainly in that the new target is not in the parent chain of the old target), so I'd be very hesitant to use a field like originalTarget for this. I think it would make more sense to add a specialized interface for command events, which has a "sourceNode" property that we'd set to the element that redispatched the event.

This implements a new interface for command events, which exposes a "sourceNode" property. When a command event is redispatched because of a command= attribute, the sourceNode is set to the element which had the command attribute.

I'm not sure I'll be able to get to this before my net connection goes away next week... Maybe try one of jst/sicking/peterv?

If there's no explicit sourceNode set, return the originalTarget. This makes things a little more consistent.

Comment on attachment 221781 [details] [diff] [review] patch 2 Trying jst while sicking is away at xtech...

This makes a bit scarier of a change to the way command events are dispatched, but I think it makes things more consistent. Right now, we retarget a command event to the element pointed to by command=, only if the element with command= is the event's originalTarget. If the element with command= is _not_ the originalTarget, then the command's oncommand handler still gets executed, but via broadcaster hookup (command= is treated like observes=, for everything except menuitem and key elements). It's not executed in a very nice way though -- since the oncommand attribute was simply inherited onto the element, |this| refers to the element with command=, not the <command> element. This is pretty broken compared to how event handlers work everywhere else. I think the right answer here is to always do the retargeting, whether the element is the originalTarget of the event or not. Now, to solve this bug, which is about getting the source event info, what we actually want is to expose the target and originalTarget of the event before it was retargeted. We could add some more properties on the event for these, but since we're basically starting dispatch over, we may as well just create a new command event, copy the modifier state from the old event, and set a property on the new event that points to the old event. That way, you can get anything you want (target, originalTarget, ...) from before the event was caught by command=, and we're no longer doing something that's sketchy as far as the DOM event spec goes. As a result of this change, we'll ensure that oncommand handlers on <command> elements always execute with |this| set to the command element, which I'm fairly sure we want. One thing I'm kind of unsure about here is how sourceEvent should work if there was no retargeting. I've implemented it so that sourceEvent will give you |this| if there was no explicit sourceEvent, the same way originalTarget will give you target if there was no separate originalTarget. I could be convinced to have it return null instead if someone has a good argument for that.

Comment on attachment 222264 [details] [diff] [review] alternate approach - In nsXULElement::PreHandleEvent(): + // Create a new command event to dispatch to the element + // pointed to by the command attribute. The new event's + // sourceEvent will be the original command event that we're + // handling. + + PRBool trusted = + (aVisitor.mEvent->flags & NS_EVENT_FLAG_TRUSTED) != 0; Please use NS_IS_TRUSTED_EVENT(aVisitor.mEvent) here. sr=jst with that changed and assuming Neil is ok with this change.

I think sourceEvent should return null for a .doCommand()-generated event. I would like to think that it was possible for sourceEvent to be the original key or mouse event for other command events. In that case a command event would be responsible for forwarding its modifier getters to the source event (or returning false if there is no source event). I am more concerned about the retargetting change, as I believe it will impact http://lxr.mozilla.org/mozilla/source/mail/components/compose/content/messengercompose.xul#610 because the command on the toolbarbutton will override the oncommand on the menuitems that get generated by the popupshowing handler. Note that the current situation isn't much better; because of the broadcaster hookup the cmd_spelling's oncommand handler gets copied to the toolbarbutton.

(In reply to comment #7) > I think sourceEvent should return null for a .doCommand()-generated event. I > would like to think that it was possible for sourceEvent to be the original key > or mouse event for other command events. In that case a command event would be > responsible for forwarding its modifier getters to the source event (or > returning false if there is no source event). Ok, so in this case: <key command="cmd_foo"/> are you thinking that the event that <command id="cmd_foo"/> sees would have its sourceEvent set to the original command event that was targeted at the <key>, and _that_ command event would have its sourceEvent set to the keypress event? > > I am more concerned about the retargetting change, as I believe it will impact > http://lxr.mozilla.org/mozilla/source/mail/components/compose/content/messengercompose.xul#610 > because the command on the toolbarbutton will override the oncommand on the > menuitems that get generated by the popupshowing handler. Hm, you're right. This is actually something I was a little worried about. I don't think PreHandleEvent is really the right place to do this work, because it's called before dispatch begins. If we suppress forwarding of the oncommand attribute, we could do this in PostHandleEvent and probably be ok...

(In reply to comment #8) >Ok, so in this case: ><key command="cmd_foo"/> >are you thinking that the event that <command id="cmd_foo"/> sees would have its >sourceEvent set to the original command event that was targeted at the <key>, >and _that_ command event would have its sourceEvent set to the keypress event? Yes; triggered by your idea from switching from a source node to a source event, I wondered whether we, I mean you, could usefully, that is, relative to the potential additional implementation cost, generalise it in exactly this way.

Ok, so I think the event dispatching change (making command= work right when it's on something other than the originalTarget) is a bit larger of a change than I want to deal with at the moment. It's a separate bug, really, so I filed it as bug 338386. I'll post a safer patch for sourceEvent that doesn't mess with that part. As far as Neil's comments, I'm not opposed to exposing the mouse or key events, but I wonder if putting it in sourceEvent makes that field a little too general-purpose. Just thinking about how I'd document it -- /** * sourceEvent points to the event which triggered this command event. In the * case of command= retargeting, this will be another command event. If the * command was triggered by a mouse or key event, that event will be returned. * If no event triggered this event, the sourceEvent is null. */ It seems like maybe we want to disambiguate the two, that is, maybe make sourceEvent just deal with command= retargeting, and have an inputEvent field that can give you the mouse or key event. The mouse and key event stuff might be slightly tricky to implement given the way those events are currently dispatched (the frame code doesn't have access to the original DOM event)... so my thinking at the moment is that I'd just implement sourceEvent as I described above and leave the rest for another time.

This patch doesn't mess with the originalTarget checking - if the command event bubbles, it'll work the same way it does currently. I also changed the event to return null for the sourceEvent if there wasn't one set, rather than returning itself.

Comment on attachment 222547 [details] [diff] [review] safer patch sr=jst

checked in on trunk.

Comment on attachment 222547 [details] [diff] [review] safer patch I'd like to get this in on the branch so that we can use it in the metrics extension (see bug 338591). I think the changes are fairly low-risk -- the only danger would be some code depending on command events being mouse events (in some way other than accessing the modifiers).

Comment on attachment 222547 [details] [diff] [review] safer patch a=jst, but there's some branch porting related to event dispatching that'll need to be done, and that part probably re-reviewed once merged.

I missed some call sites in the mac widget code, I'll post a fix once I've had a chance to build and test on Mac.

The mac menu code was doing its _own_ command= retargeting... this should no longer be necessary since XULElements do this generically (this code likely predates that).

Comment on attachment 223262 [details] [diff] [review] fix mac menu code Looks reasonable but I don't think I'm eligible to give r+sr especially given that I don't actually have access to a Mac.

Comment on attachment 223262 [details] [diff] [review] fix mac menu code cool. r=pink

jst, does this look ok for the branch?

Comment on attachment 223336 [details] [diff] [review] branch patch with mac fixes r+a=jst

checked in on the branch per jst and mconnor's approval

This seems like the most likely cause of bug 341301.