Brian Ryner (not reading) Reporter
	Description • 21 years ago

I've been doing some thing about this and I think we could make things a lot
simpler, and also more correct, by pulling the event dispatch logic out of the
content nodes into a centralized dispatcher method.  Here are the benefits I
think we'd see:

- Less duplication of code in ::HandleDOMEvent() methods -- content nodes would
(hopefully) no longer have to worry about how and when to send events to their
parent node, anonymous content retargeting, etc.  They would only need to worry
about the local handling stage.

- We could build up an event path prior to beginning the event dispatch, which
would fix one case where we violate the DOM spec -- the event flow is not
supposed to be affected by adding or removing nodes during event dispatch.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 21 years ago

So the way this would work is that we would build that path and then do the
following:

1) Walk path top-down for the capturing phase in the normal event group, firing
   listeners
2) Fire target's listeners in normal event group
3) Walk path bottom up for the bubbling phase in the normal event group, firing
   listeners
4) Repeat above three steps for system event group
5) Walk bottom up starting at target executing default actions (local handling).

Seem reasonable?

Or is that last part supposed to be covered by the system event group?

	neil@parkwaycc.co.uk
	Comment 2 • 21 years ago

(In reply to comment #1)
>So the way this would work is that we would build that path and then do the
>following:
>
>1) Walk path top-down for the capturing phase in the normal event group,
>   firing listeners
>2) Fire target's listeners in normal event group
>3) Walk path bottom up for the bubbling phase in the normal event group,
>   firing listeners
>4) Repeat above three steps for system event group
>5) Walk bottom up starting at target executing default actions (local
>   handling).
>
>Seem reasonable?
>
>Or is that last part supposed to be covered by the system event group?
I understand that it is :-) But currently, we have a mixture of frame-based
default actions and system event group default actions, and executing them in
the order shown above causes a minor bug in that certain keys are always
processed by xbl in the system event group as a default action and are therefore
technically unavailable to frame handlers as a default action. Because of this
the menu frame just ignores the prevent default flag and always opens the
menulist even when someone else (e.g. the menu bar) wants to process the down
arrow key. This would of course not be a problem if the menu frame handler was
in (or before) the system event group, because then it would fire before the xbl
key handler.

Also, there are two further issues that this may or may not resolve... one was a
menulist in the mailnews advanced search widget that rebuilt itself after one of
its menuitems was clicked, which caused some strangeness, and one was an event
listener that removed itself during event processing which caused the next event
listener to get skipped.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 20 years ago

Should look into fixing bug 53118 with this too.

	Hixie (not reading bugmail)
	Comment 4 • 19 years ago

When you do this, you should make <form> elements only do their default action for "submit" and "reset" events if they were the target of the event. The events should in all other respects act like normal events, bubbling and so forth, and should not be killed upon bubbling into a form as they are today.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 5 • 19 years ago

This is a scary patch ;)
So the basic idea is to have the following phases in the event dispatching:
 - PreHandle phase, this is used to create the event target chain. It also
   handles possible re-targeting. (This is a bubbling phase.)
 - capture/target/bubble, default group.
(- Possible nsDispatchingCallback, needed by PresShell.)
 - capture/target/bubble, system group.
 - PostHandle phase, this is the place where default handling should happen.
   Event bubbles from the original target.

Visitor object is used to create the event target chain. Similar object is 
used also in PostHandle phase to pass the nsEvent and nsDOMEvent and flags and 
so on to the items in the event target chain.

The *target members of the nsDOMEvent are now in nsEvent. With this change
there is usually no need to create nsDOMEvent, even if the event crosses 
binding boundary (DOMEvent is created only if there is a listener for it).
What is perhaps more important is that the |target| is 
always set before starting to dispatch the event.

Some other changes are also that DOMEvent dispatching doesn't need prescontext
and creating a DOMEvent doesn't need nsEventListenerManager anymore.

This is still in progress. Especially form controls need testing/hacking and 
I haven't tested this with TB nor SM at all.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 19 years ago

I was still under the impression that default handling goes in the bubbling phase in the system group... isn't that the whole point of the system group?

Other than that, this sounds fantastic!

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 7 • 19 years ago

(In reply to comment #6)
> I was still under the impression that default handling goes in the bubbling
> phase in the system group... isn't that the whole point of the system group?

Well, we could actually remove the whole system group, but IIRC
there are some XBL bindings which want to make sure that their
handlers are called after default group.
The posthandling is needed because of the used visitor pattern. Default handling 
needs in some cases few special flags. In the HandleDOMEvent implementation
those are handled by the recursive method call, but in this new code
some flags need to be set in PreHandleEvent so that those can be used in PostHandleEvent. The flags are "event-dispatching-dependent", meaning they are
stored in the event target chain.

> 
> Other than that, this sounds fantastic!
> 

:)
Btw, I did also few performance tests. Those showed that the new code should
be faster, in some cases significantly faster than the old implementation.
Apparently doing 1 recursion (PreHandle) and 5 while loops 
(capture/bubble/capture/bubble/default) is faster than the old 4 recursions
(capture/bubble/capture/bubble).

	neil@parkwaycc.co.uk
	Comment 8 • 19 years ago

(In reply to comment #7)
>(In reply to comment #6)
>>I was still under the impression that default handling goes in the bubbling
>>phase in the system group... isn't that the whole point of the system group?
>Well, we could actually remove the whole system group, but IIRC
>there are some XBL bindings which want to make sure that their
>handlers are called after default group.
I agree with bz - it should be possible to make all our handling (except possibly frames) happen in the system event group's bubbling phase, in which case you only need to worry about DOM event listeners for other phases.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 9 • 19 years ago

(In reply to comment #8)
> I agree with bz - it should be possible to make all our handling (except
> possibly frames) happen in the system event group's bubbling phase, in which
> case you only need to worry about DOM event listeners for other phases.
> 
Could you explain why?
The posthandling is anyway a bubbling phase and default
handling needs to be done in a bit different way than normal
DOM's handleEvent().
IMO bindings which are handling events in system group, should
be able to cancel the default handling using preventDefault(), even
if they are listening only bubble phase.

But I can ofc easily replace system group's bubbling phase with
post handling. Or merge them. I just wouldn't like to do that without a reason.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 19 years ago

> there are some XBL bindings which want to make sure that their
> handlers are called after default group.

Yes, these are XBL bindings defining default handling.

> Default handling  needs in some cases few special flags.

Like what?  That is, what's needed other than the preventDefault flag on the event?

As far as that goes, bryner, should preventDefault prevent system event group dispatch?

> Could you explain why?
...
> IMO bindings which are handling events in system group, should
> be able to cancel the default handling using preventDefault(), even
> if they are listening only bubble phase.

If I have a binding and the binding's content has an anchor and I click on the anchor, and the binding has a default click action defined for the bound element (aka an event handler in the bubbling phase of the system event group), then it seems to me that we should traverse the anchor.  Right now we don't, but that's a bug...

So I would say that XBL system event group handlers on a target should fire before the default handling, but that both should happen before the event bubbles up one level.  That way if I attach a binding to an anchor I can override the click action, but if I attach one to the parent of an anchor I should have to install a capturing handler in the system event group to override the anchor's default handling.  Or install a handler in the regular event group, of course.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 19 years ago

Put another way, we need a way to install new default actions on elements via XBL, for editor, etc.  That's what the system event group is for, as I understand.

I could be wrong, of course.  I'd really like to hear Brian's thoughts on this.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 12 • 19 years ago

(In reply to comment #10)
> 
> > Default handling  needs in some cases few special flags.
> 
> Like what?  That is, what's needed other than the preventDefault flag on the
> event?

With radiobuttons for example the checked state is set before event dispatching, 
but if the event is cancelled the state must be restored to the original value.
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLInputElement.cpp#1265
But I guess I could make that to work also in system group - somehow.
Any comments bryner or jst?

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 19 years ago

Ah, ok.  So we have two categories of "default processing" -- actual default actions (like an anchor following a link) which should just happen in the system event group, and weird DOM0 compat stuff like the radio button thing.  I have no problem with a separate post-dispatch callback for the latter.  Thanks for explaining!

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 14 • 19 years ago

So, should I merge the system group and post handling?
I could do it so that when events are handled in system group's bubble phase
I first call HandleEvent (if there are any event listeners) and immediately after 
that PostHandleEvent. And then moving to parent target and doing the same and so on.
This way even DOM0 compat things should be handled properly.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 19 years ago

I don't have a strong preference on that...

	Alex Vincent [:WeirdAl]
	Comment 16 • 19 years ago

Comment on attachment 208601 [details] [diff] [review]
work in progress

Patch doesn't compile:
m:/smaugtest\mozilla\content\events\src\nsEventStateManager.cpp(738) : error C2664: 'nsEventDispatcher::Dispatch' : cannot convert parameter 5 from 'int' to 'nsEventStatus *'
        Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or function-style cast

content/events/src/nsEventStateManager.cpp
+                nsEventDispatcher::Dispatch(gLastFocusedContent, oldPresContext,
+                                            &event, nsnull, NS_EVENT_FLAG_INIT,
+                                            &status);

content/events/public/nsEventDispatcher.h
+  static nsresult Dispatch(nsISupports* aTarget,
+                           nsPresContext* aPresContext,
+                           nsEvent* aEvent,
+                           nsIDOMEvent* aDOMEvent = nsnull,
+                           nsEventStatus* aEventStatus = nsnull,
+                           nsDispatchingCallback* aCallback = nsnull,
+                           PRBool aTargetIsChromeHandler = PR_FALSE);

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 17 • 19 years ago

I'll move event handling methods from nsIContent/nsIDocument to nsINode.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 18 • 19 years ago

FF, TH and SM should all be useable with this
(although I've tested this only in Linux...so I haven't 
actually even compiled the Mac related changes).

I was wrong. The default handling *must* be done while handling
system event group (otherwise keybindings don't always work).
So I merged system group event handling and default handling.

I'll have to test this some more, but all comments are welcome.
Asking reviews after testing.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 19 • 19 years ago

(In reply to comment #18)
> Created an attachment (id=209304) [edit]
>
> FF, TH and SM should all be useable with this
> (although I've tested this only in Linux...so I haven't 
> actually even compiled the Mac related changes).

Hmm, GMail doesn't seem to work :(

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 19 years ago

Does it work without this patch?  Note bug 322565

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 21 • 19 years ago

> Does it work without this patch?  Note bug 322565
These kinds of problems make the testing difficult, have to
check with 1.8 and trunk and so on. But this was my bug - 'load' was
bubbling when dispatched to frame elements.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 19 years ago

Note that you can effectively fix bug 322565 on trunk by just changing the PR_TRUE to PR_FALSE at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/caps/include/nsScriptSecurityManager.h&rev=1.96&mark=391#391

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 24 • 19 years ago

Btw, I'm not going to fix all the bugs this bug blocks in the same patch.
Those can be fixed separately in much smaller patches.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 19 years ago

Oh, definitely.  A lot of the deps are just things that _should_ be fixable once this is fixed...

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 27 • 19 years ago

This should be quite ready for reviews, I think (unless I just broke something).
Now I just have to find some masochist who is willing to read and comment this ;)

	Martijn Wargers (dead)
	Comment 28 • 19 years ago

I've tried the "A fix to make it compile also on mac" patch, and when I drag-drop some text in the find bar of Firefox, the text gets doubled. (Not sure, could be a bug in the drag-drop code of the find bar).

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 29 • 19 years ago

(In reply to comment #28)
> I've tried the "A fix to make it compile also on mac" patch, and when I
> drag-drop some text in the find bar of Firefox, the text gets doubled. (Not
> sure, could be a bug in the drag-drop code of the find bar).
> 
I don't see this problem, at least not with the latest patch.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 31 • 19 years ago

The very latest patch is always also in
http://www.cs.helsinki.fi/u/pettay/moztests/events/

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 32 • 19 years ago

To fix middle mouse paste in editor nsDocument::PreHandleEvent should look like this:
+  aVisitor.mCanHandle = PR_TRUE;
+   //XXX Remove me! This is a hack to make middle mouse paste working also
+   //    in Editor.
+  aVisitor.mForceContentDispatch = PR_TRUE;
+  nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(GetWindow());
+  aVisitor.mParentTarget = sgo;
+  return NS_OK;

The old event dispatching code works in this case only because in nsDocument it 
doesn't check NS_EVENT_FLAG_NO_CONTENT_DISPATCH flag.

	neil@parkwaycc.co.uk
	Comment 33 • 19 years ago

Oh, so you mean that text inputs, textareas and editors deliberately override the default middle mouse dispatch behaviour so that the editor event listener will get dispatched? Not that I can think of a better way of achieving this...

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 34 • 19 years ago

Ugh, have to fix prescontext handling later.
Even chrome creates events using one document but dispatches them
to some other document. 
This means that DOMEvent's prescontext != dispatching prescontext :(

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 35 • 19 years ago

Argh, sorry. I forgot few null checks.

	Alex Vincent [:WeirdAl]
	Comment 36 • 19 years ago

I'm going to hold my mutation events work until this bug is fixed.  I can't keep up with all the patches. ;)

	Johnny Stenback (:jst)
	Comment 37 • 19 years ago

Comment on attachment 210051 [details] [diff] [review]
previous + null checks

- In content/base/public/nsINode.h:

+  virtual nsresult DispatchDOMEvent(nsEvent* aEvent,
+                                    nsIDOMEvent* aDOMEvent = nsnull,
+                                    nsPresContext* aPresContext = nsnull,
+                                    nsEventStatus* aEventStatus = nsnull) = 0;

I would much prefer to see this w/o default arguments, defaults arguments tend to hurt more than they help long term, at least that's my experience...

+  virtual nsresult GetEventListenerManager(nsIEventListenerManager** aResult,
+                                           PRBool aOnlyExisting = PR_FALSE) = 0;

Same here, and even more so. If this was to have a default argument it would IMO have to default to not creating an event listener manager, if we leave this like this it'd be too easy (IMO) to write code that would unnecessarily allocate event listener managers for no reason. And I'd flip the logic around with the aOnlyExisting argument too, name it aCreateIfNotFound (or something).

The above method signatures are spread out all over this patch, and any changes obviously need to be made in all places.

- In content/base/src/nsGenericDOMDataNode.h:

+  virtual nsresult GetEventListenerManager(nsIEventListenerManager** aResult,
+                                           PRBool aOnlyExisting = PR_FALSE) {
+    return GetListenerManager(aResult, aOnlyExisting);
+  }

Could we not simply consolidate these two getters to avoid double virtual call overhead here?

- In content/events/src/nsDOMEvent.h

-  void* mScriptObject;

Nice! We've carried this unused member since the XPCDOM landing in 2001! :)

- In nsHTMLButtonElement::PostHandleEvent():

       case NS_MOUSE_LEFT_CLICK:
         {
-          nsIPresShell *presShell = aPresContext->GetPresShell();
+          nsIPresShell *presShell = aVisitor.mPresContext->GetPresShell();
           if (presShell) {
             // single-click
-            nsUIEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_UI_ACTIVATE, 1);
+            nsUIEvent event(NS_IS_TRUSTED_EVENT(aVisitor.mEvent),
+                            NS_UI_ACTIVATE, 1);
             nsEventStatus status = nsEventStatus_eIgnore;
 
             presShell->HandleDOMEventWithTarget(this, &event, &status);
-            *aEventStatus = status;
+            aVisitor.mEventStatus = status;

In patterns like this, is there any reason not to use aVisitor.mEventStatus directly here and eliminate the local status variable?

That's all I got through so far, more to follow (starting in nsHTMLFormElement.cpp)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 38 • 19 years ago

> I would much prefer to see this w/o default arguments, defaults arguments
> tend to hurt more than they help long term, at least that's my experience...

Will remove, or perhaps leave only to nsEventDispatcher::Dispatch(), because I
think default arguments are quite useful there.

> - In content/base/src/nsGenericDOMDataNode.h:
> 
> +  virtual nsresult GetEventListenerManager(nsIEventListenerManager** 
>                                             aResult,
> +                                           PRBool aOnlyExisting = PR_FALSE){
> +    return GetListenerManager(aResult, aOnlyExisting);
> +  }
> 
> Could we not simply consolidate these two getters to avoid double virtual 
> call overhead here?

I added GetEventListenerManager because both nsIContent and nsIDOMEventReceiver
have GetListenerManager method and nsDocument is nsIDOMEventReceiver and 
nsINode - and I don't like to implement things from two interfaces using just
one method. (It should compile, I think, but is ugly IMHO.) 

>              nsEventStatus status = nsEventStatus_eIgnore;
> 
>              presShell->HandleDOMEventWithTarget(this, &event, &status);
> -            *aEventStatus = status;
> +            aVisitor.mEventStatus = status;
> 
> In patterns like this, is there any reason not to use aVisitor.mEventStatus
> directly here and eliminate the local status variable?

I don't want to change the current behaviour, at least not now.

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 19 years ago

> both nsIContent and nsIDOMEventReceiver have GetListenerManager method

Perhaps we can eliminate one of those methods in a followup bug or something?

	Johnny Stenback (:jst)
	Comment 41 • 19 years ago

Comment on attachment 210051 [details] [diff] [review]
previous + null checks

- In nsEventStateManager::PreHandleEvent():

+            //XXXsmaug bubbling for now, because in the old event dispathing

Typo, add a 'c' in dispatching...

- In content/html/content/src/nsHTMLInputElement.cpp:

   /**
+   * This is needed for event dispatching.
+   */
+  nsCOMPtr<nsIDOMHTMLInputElement> mSelectedRadioButton;

This changed from a local variable in the old HandleDOMEvent() to a member of nsHTMLInputElement since it's needed in both the new pre and post hooks. Wouldn't it make more sense to give the event visitors the ability to carry opaque info from the pre hook to the post hook (as a void *, or nsISupports *, named "data" or whatever), and sticking the selected radio button in there, rather than carrying an additional member on all input elements?

- In nsHTMLInputElement::PostHandleEvent():

-        case NS_MOUSE_MIDDLE_BUTTON_DOWN:
+        //XXXsmaug Why?
+        /*case NS_MOUSE_MIDDLE_BUTTON_DOWN:
         case NS_MOUSE_MIDDLE_BUTTON_UP:
...

Don't know, but seems it would be safer to leave this in and file a followup bug to figure this out rather than just commenting it out here in this patch.

- In nsGlobalWindow::PostHandleEvent():

+  FORWARD_TO_INNER(PostHandleEvent, (aVisitor), NS_OK);
[...]
-  if (outer && outer->mFullScreen && (NS_EVENT_FLAG_BUBBLE & aFlags)) {
-    if (aEvent->message == NS_DEACTIVATE || aEvent->message == NS_ACTIVATE) {
+  if (mFullScreen) {

This needs to get mFullScreen from the outer like the old code did. mFullScreen is only ever set on outer window objects, as stated in nsGlobalWindow.h.

- In toolkit/content/widgets/colorpicker.xml:

       <method name="_fireEvent">
         <parameter name="aTarget"/>
         <parameter name="aEventName"/>
         <body>
         <![CDATA[      
           try {
             var event = document.createEvent("Events");
-            event.initEvent(aEventName, false, true);
+            event.initEvent(aEventName, true, true);

Why does this now need to be a bubbling event? (same thing later on in this file)

- In xpfe/global/resources/content/bindings/tabbrowser.xml:

-        <xul:tabpanels flex="1" class="plain" selectedIndex="0" anonid="panelcontainer">
+        <xul:tabpanels flex="1" class="plain" selectedIndex="0" anonid="panelcontainer"
+                       onselect="if (event.originalTarget == this) this.parentNode.parentNode.updateCurrentBrowser();">


[...]

     <handlers>
-      <handler event="select" action="if (event.originalTarget == this.mPanelContainer) this.updateCurrentBrowser();"/>
-

Why did this handler move to an onselect attr on the tabpanels? Just to simplify the handler, or some other reason?

(more still to follow, starting in nsEventDispatcher.h)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 42 • 19 years ago

> Wouldn't it make more sense to give the event visitors the ability to carry
> opaque info from the pre hook to the post hook (as a void *, or nsISupports *,
> named "data" or whatever), and sticking the selected radio button in there,
> rather than carrying an additional member on all input elements?

Just jumping in sideways here, but this sounds like a great idea. Wouldn't you otherwise get re-entrency issues, like if an event is fired from an event-handler.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 43 • 19 years ago

>    /**
> +   * This is needed for event dispatching.
> +   */
> +  nsCOMPtr<nsIDOMHTMLInputElement> mSelectedRadioButton;
> 
> This changed from a local variable in the old HandleDOMEvent() to a member of
> nsHTMLInputElement since it's needed in both the new pre and post hooks.
> Wouldn't it make more sense to give the event visitors the ability to carry
> opaque info from the pre hook to the post hook (as a void *, or nsISupports *,
> named "data" or whatever), and sticking the selected radio button in there,
> rather than carrying an additional member on all input elements?
> 

That is a good idea. :)

> This needs to get mFullScreen from the outer like the old code did. mFullScreen
> is only ever set on outer window objects, as stated in nsGlobalWindow.h.
> 
> - In toolkit/content/widgets/colorpicker.xml:
> 
>        <method name="_fireEvent">
>          <parameter name="aTarget"/>
>          <parameter name="aEventName"/>
>          <body>
>          <![CDATA[      
>            try {
>              var event = document.createEvent("Events");
> -            event.initEvent(aEventName, false, true);
> +            event.initEvent(aEventName, true, true);
> 
> Why does this now need to be a bubbling event? (same thing later on in this
> file)
> 

Because currently events always bubble in XUL :( , and I don't want to change
the way how _fireEvent works.
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#2186
http://lxr.mozilla.org/seamonkey/source/content/xul/content/src/nsXULElement.cpp#1880
Only nsGenericElement has (NS_EVENT_FLAG_CANT_BUBBLE & aEvent->flags) check.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 44 • 19 years ago

(In reply to comment #41)
> Why did this handler move to an onselect attr on the tabpanels? Just to
> simplify the handler, or some other reason?

That is also because of the bubbling in XUL.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 45 • 19 years ago

Because the mItemData needed some changes in nsEventDispatcher, I think it is better to update the patch now.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 46 • 19 years ago

(In reply to comment #45)
> Created an attachment (id=210621) [edit]
> 
Oops, just noticed there is a typo _CENTENT, should be _CONTENT ofc ;)

	Johnny Stenback (:jst)
	Comment 48 • 18 years ago

Comment on attachment 211162 [details] [diff] [review]
...and up-to-date

- In nsHTMLInputElement::PreHandleEvent():

             if (GetNameIfExists(name)) {
+              nsCOMPtr<nsIDOMHTMLInputElement> selectedRadioButton;
               container->GetCurrentRadioButton(name,
                                                getter_AddRefs(selectedRadioButton));
+              aVisitor.mItemData = do_QueryInterface(selectedRadioButton);

No need to QI here.

- In nsEventDispatcher.h:
+class nsEventDispatcher
+{
+public:
+  /**
+   * Target should be nsINode, nsPIWindow or nsIChromeEventHandler.

That's nsPIDOMWindow

- In nsEventDispatcher.cpp:

+class nsEventTargetChainItem
+{
[...]
+  nsISupports* Retarget()
+  {
+    return mRetarget;
+  }

Just food for thought... Reading this method signature, it seems to mean that this method will retaget the item, not that it's a getter for what the item is retargetted to. The alternatives seem to be to rename this to GetRetarget(), or to come up with some other more clear way of expressing this. mRetarget can be null though, right? If so, this really should be named GetRetarget(), or something different.

+  already_AddRefed<nsIEventListenerManager> ListenerManager(
+    PRBool aCreateIfNotFound);

Since this may end up creating a listener manager somewhere down the call, it's not impossible for this method to return null (if we're OOM). Because of that, this should be named GetListenerManager().

That's all I found so far, and I think that on my part this is about as reviewed as it'll ever get by me. So with those minor things addressed, r=jst

This is a *great* simplification (plus cleanup) and a *huge* step forward for our event code! Nicely done!

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 49 • 18 years ago

(In reply to comment #48)
> - In nsEventDispatcher.cpp:
> 
> +class nsEventTargetChainItem
> +{
> [...]
> +  nsISupports* Retarget()
> +  {
> +    return mRetarget;
> +  }
> 
> Just food for thought... Reading this method signature, it seems to mean that
> this method will retaget the item, not that it's a getter for what the item is
> retargetted to. The alternatives seem to be to rename this to GetRetarget(), or
> to come up with some other more clear way of expressing this. mRetarget can be
> null though, right? If so, this really should be named GetRetarget(), or
> something different.

mRetarget sounds like an odd name for an object -- it's retarget is a verb, it sounds like a boolean variable.  How about mNewTarget, etc.?  Or if that's not clear enough, mRetargetTarget, etc., although that's really ugly?  (Target can be both a verb or a noun.)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 50 • 18 years ago

Changed Retarget to NewTarget and those methods to Get*().
peterv also noticed that nsContentUtils::LookupListenerManager is not used
anymore (except in debug code), so I removed it.
I think bz mentioned something about sr-ing this ;)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 51 • 18 years ago

I can save the binary size a bit if I don't move mTmpRealOriginalTarget and
mExplicitOriginalTarget to nsEvent. They are not needed there anyway.
So no need to change nsDOMEvent::GetExplicitOriginalTarget or
nsDOMEvent::GetTmpRealOriginalTarget either.

	Boris Zbarsky [:bzbarsky]
	Comment 52 • 18 years ago

I'm hoping to look at this toward the end of next week.  I'm afraid there's just no way it can happen before then.  :(

	Boris Zbarsky [:bzbarsky]
	Comment 55 • 18 years ago

Comment on attachment 213475 [details] [diff] [review]
up-to-date

So first of all, this is great stuff!  I finally got to starting in on this; I'm up to nsGenericElement (yeah,  know.... not very far).  Comments on what I've read so far:

>Index: content/base/public/nsIContent.h
>-class nsAttrValue;
> class nsAttrName;
>+class nsAttrValue;

Why?  May as well put this back the way it was.

>+  virtual nsresult GetListenerManager(PRBool aCreateIfNotFound,
>+                                      nsIEventListenerManager** aResult) = 0;

I assume this is slated to die in favor of the nsINode method and that you just didn't want to make this patch bigger?  If so, please file a bug to do that, and reference it in a FIXME comment here.  If not, why not?  That is, why have two identical methods per content node?

>Index: content/base/public/nsINode.h

>+   * Called before the capture phase of the event flow.
>+   * This is used to create the event target chain and implementations
>+   * shoud fill the 'out' parameters of the visitor object.

s/should/should/

>+   * @see nsEventDispatcher.h

So what are the "'out' parameters"?  What does that even mean for an object?  In my opinion, the documentation here should expicitly say which members of nsEventChainPreVisitor the method must fill in, which it may fill in, etc.  The current setup, where one has to look at several different class decls to get this information, seems inefficient to me...  If there were a nice comment at the very top of nsEventDispatcher.h explaining all this that would be fine too, but there isn't.  As it is, it's not clear to me from looking at the documentation and even nsEventDispatcher.h which of the PreHandleEvent implementations are correct...  I'll have to rereview them once this is documented.

>+   * Called after the bubble phase of the system event group.
>+   * The default handling of the event should happen here.
>+   */
>+  virtual nsresult PostHandleEvent(nsEventChainPostVisitor& aVisitor) = 0;

Document the param?  Again, a comment explaining general setup at the top of nsEventDispatcher.h (with an "@see nsEventDispatcher.h for documementation about aVisitor" here) may be the way to go.

>+   * Dispatch an event.
>+   * @param aEvent the event that is being dispatched.
>+   * @param aDOMEvent the event that is being dispatched, use if you want to
>+   *                  dispatch nsIDOMEvent, not onlyt nsEvent.

s/onlyt/only/

If both aEvent and aDOMEvent are set, are there any restrictions on the relationship between them (eg aEvent should be the nsEvent inside aDOMEvent or something)?  If so, they should be documented here and implementations should assert (or check and bail, but I think assert is fine).

>+   * @param aEventStatus the status returned from the function, can be nsnull.
"can be nsnull if the caller is not interested in the resulting event status" or something?

>+  virtual nsresult DispatchDOMEvent(nsEvent* aEvent,
>+                                    nsIDOMEvent* aDOMEvent,
>+                                    nsPresContext* aPresContext,
>+                                    nsEventStatus* aEventStatus) = 0;

So do callers of DispatchDOMEvent need to call PreHandleEvent and PostHandleEvent?  Should document that clearly.  In fact, should anyone but nsEventTargetChainItem be calling those methods?  If not, perhaps it should be declared a friend class or something and the methods should be protected?  Not a big deal either way on this last, but please do document the other.

>+   * Get the event listener manager, the guy you talk to to register for events
>+   * on this element.

s/element/node/

You probably ought to change the IID on nsIAttribute, since it inherits from
nsINode and hence its vtable has changed...

>Index: content/base/src/nsContentUtils.cpp

> nsContentUtils::GetListenerManager(nsIContent *aContent,

I'm not sure why we're merging two methods into one, but I guess it's ok.  I tried to figure out a way to reduce codesize by just changing the operator based on aCreateIfNotFound, but the checks after the PL_DHashTableOperate that we need in the two cases are pretty different.

>Index: content/base/src/nsDOMAttribute.cpp

>+nsDOMAttribute::GetEventListenerManager(PRBool aCreateIfNotFound,
>+                                        nsIEventListenerManager** aResult)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;

Set *aResult to null, please?  In DOM code it's the nice thing to do (and I should note that there is some code in this patch that doesn't check rv for GetEventListenerManager calls...

>Index: content/base/src/nsDocument.cpp
>         // To dispatch this event we must manually call
>+        // nsEventDispatcher::Dispatch() on the ancestor document since the

Why can't we call DispatchDOMEvent on the ancestor?  That is, why the lower-level call?  If there's a reason, it should be documented.

> nsDocument::GetSystemEventGroup(nsIDOMEventGroup **aGroup)
>+  if (NS_SUCCEEDED(GetListenerManager(PR_TRUE, getter_AddRefs(manager))) &&
>+    manager) {

Weird indent here; line up the two parts of the condition, please.

>+nsDocument::PreHandleEvent(nsEventChainPreVisitor& aVisitor)
>+   //XXX Remove me! This is a hack to make middle mouse paste working also
>+   //    in Editor.

File a bug and reference the bug number here with a FIXME comment instead of this XXX, please.

>+  nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(GetWindow());
>+  aVisitor.mParentTarget = sgo;

So why nsIScriptGlobalObject?  Shouldn't it be nsPIDOMWindow?  That's what the docs about targets in nsEventDispatcher say, no?

>+nsDocument::DispatchDOMEvent(nsEvent* aEvent,
>+  return nsEventDispatcher::DispatchDOMEvent(NS_STATIC_CAST(nsIDocument*, this),

Why not cast to nsINode* instead?  That's what DispatchDOMEvent wants to see.  And if we ever end up with ambiguous inheritance from nsINode, we probably want to catch that right here.

But frankly, it seems to me like we should have type-safe DispatchDOMEvent methods that inline to the protected method we have now...  That would eliminate the need for casting and make it impossible to make mistakes.

> nsDocument::DispatchEvent(nsIDOMEvent* aEvent, PRBool *_retval)
>+    nsEventDispatcher::DispatchDOMEvent(NS_STATIC_CAST(nsIDocument*, this),

Why not DispatchDOMEvent on ourselves?  If there's a reason, document.  If not, I'd prefer that to keep the number of codepaths minimal.

> nsDocument::DispatchEventToWindow(nsEvent *aEvent)
>+  aEvent->target = NS_STATIC_CAST(nsIDocument*, this);

Again, cast to nsINode*?  Or do we just need our canonical nsISupports pointer here, not our nsINode one?  Which nsISupports pointer is wanted by nsEvent.target should probably be documented in in nsGUIEvent.h....

>Index: content/base/src/nsGenericDOMDataNode.cpp
>+nsGenericDOMDataNode::DispatchDOMEvent(nsEvent* aEvent,

>+  return nsEventDispatcher::DispatchDOMEvent(NS_STATIC_CAST(nsIContent*, this),
Again, cast to nsINode or better yet have typesafe DispatchDOMEvent methods on nsEventDispatcher.

>@@ -1134,19 +1071,17 @@ nsGenericDOMDataNode::SetText(const PRUn
>+    nsEventDispatcher::Dispatch(this, nsnull, &mutation);

So I'm still being a little confused here: for any given target we have at least three separate ways of dispatching an event:

1) DispatchDOMEvent on the target itself
2) nsEventDispatcher::DispatchDOMEvent
3) nsEventDispatcher::Dispatch

What are the differences?  Why are we using one over the other in various places?  This should probably be clearly documented somewhere (eg nsEventDispatcher.h)

Hopefully more tomorrow.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 56 • 18 years ago

> 1) DispatchDOMEvent on the target itself
> 2) nsEventDispatcher::DispatchDOMEvent
> 3) nsEventDispatcher::Dispatch

> What are the differences?  Why are we using one over the other in various
> places?  This should probably be clearly documented somewhere (eg
> nsEventDispatcher.h)

I'm not too happy either to have 3 methods. DispatchDOMEvent
on target itself is sort of 'deprecated' - it is just for non-gecko callers, 
which are currently using HandleDOMEvent. All those should be eventually 
converted to use nsIDOMEventTarget::dispatchEvent.
The difference between nsEventDispatcher::DispatchDOMEvent
and nsEventDispatcher::Dispatch is documented, though one could argue
that those could be combined.

btw, if and when nsEvent and nsDOMEvent are merged, the (internal) API will change again.

But more tomorrow :)

	Boris Zbarsky [:bzbarsky]
	Comment 57 • 18 years ago

> DispatchDOMEvent on target itself is sort of 'deprecated' - it is just for
> non-gecko callers, 

Ah, ok.  Let's document that?

> The difference between nsEventDispatcher::DispatchDOMEvent
> and nsEventDispatcher::Dispatch is documented

The difference between what they _do_ is documented.  If it's at all possible to document when one should use which one, I think that would be most helpful.  As in, do the analysis once, instead of relying on everyone to do it and get it right.  If we can't come with such docs, it's not a huge deal, but if we can that would rock.  Preferably at the top of the nsEventDispatcher header in some sort of "here is how the system works" comment?

> btw, if and when nsEvent and nsDOMEvent are merged, the (internal) API will
> change again.

Makes sense.  :)

	Boris Zbarsky [:bzbarsky]
	Comment 58 • 18 years ago

Comment on attachment 213475 [details] [diff] [review]
up-to-date

>Index: content/base/src/nsGenericElement.cpp

>+nsGenericElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)
>   if (IsNativeAnonymous()) {
>+    aVisitor.mEventTargetAtParent = parent;

So I think we should have the same thing in nsGenericDOMDataNode, actually.  Possibly as a followup, to make sure any regressions happen on a different day?  Make sure a bug to look into it is filed, in any case.

>+  } else if (parent) {
>+    nsCOMPtr<nsIContent> content(do_QueryInterface(aVisitor.mEvent->target));
>+    if (content && content->GetBindingParent() == parent.get()) {
>+      aVisitor.mEventTargetAtParent = parent;

So I'm trying to follow this code.  Say we have two XBL bindings.  Binding 1 is attached to node A.  Node B is anonymous content generated by binding 1.  Binding 2 is attached to node B.  Node C is anonymous content generated by binding 2.  We dispatch an event to node C.  As we're going up the tree calling PreHandleEvent, are we changing the target as we go?  That is, when we're at node B, will the mEvent->target be B or C?  Might be nice to document this somewhere too.

In any case, you shouldn't need the .get() on parent there.

>   nsIDocument* ownerDoc = GetOwnerDoc();
>   if (ownerDoc) {
>+    nsCOMPtr<nsIContent> insertionParent;
>     ownerDoc->BindingManager()->GetInsertionParent(this,

Hey, want to add an "// XXX XBL2/sXBL issue" comment here?  It's not clear which document we want the binding manager from (owner vs current).  I'd tend to think current, but not sure.

So by the time we get here we've already set mEventTargetAtParent, right?  So if we now change |parent| things will get weird, no?  In that we'll fire an event at node A which is our insertion parent but the event target will be retargeted to node B?  I _think_ that we can't be retargeting when we have an insertion parent....  but I'm not sure.  If we're pretty sure this is the case, let's add asserts to that effect?

>@@ -2490,19 +2324,17 @@ nsGenericElement::doInsertChildAt(nsICon

>       nsMutationEvent mutation(PR_TRUE, NS_MUTATION_NODEINSERTED, aKid);

Now that we store the target on nsEvent, we may be able to nix the mTarget member from nsMutationEvent.  Followup bug on that, please?

>Index: content/base/src/nsImageLoadingContent.cpp
>@@ -737,28 +738,30 @@ public:
>+  if (eventMsg == NS_IMAGE_LOAD) {
>+    // Load event doesn't bubble.

In the old code, neither did NS_IMAGE_ERROR.  With your change, bug 79133 regresses, no?  So I think you want NS_EVENT_FLAG_CANT_BUBBLE for both event types in this method....  I know that's not what the DOM spec says, but...

>Index: content/events/public/nsIEventListenerManager.h
>   NS_IMETHOD HandleEvent(nsPresContext* aPresContext,
>                          nsEvent* aEvent,
>                          nsIDOMEvent** aDOMEvent,
>-                         nsIDOMEventTarget* aCurrentTarget,
>+                         nsISupports* aCurrentTarget,

Does it need to be any particular nsISupports ptr?  Or just any nsISupports that will QI to something useful?

>+  * Creates a DOM event.
>+  * Preferred way to create an event is to use either
>+  * nsEventDispatcher::CreateEvent or nsIDOMDocumentEvent::createEvent.
>   */
>   NS_IMETHOD CreateEvent(nsPresContext* aPresContext,

Is this method slated to go?  If so, file a followup bug on that, mention it in this comment?

>Index: content/events/src/nsDOMEvent.cpp
> NS_METHOD nsDOMEvent::GetTarget(nsIDOMEventTarget** aTarget)
> {
>+  if (mEvent->target) {
>+    CallQueryInterface(mEvent->target, aTarget);
>   }
>   return NS_OK;

I'm glad to see the improvement here, but you need to set *aTarget to null if mEvent->target is null, no?  And return the reval from CallQI!

> nsDOMEvent::GetCurrentTarget(nsIDOMEventTarget** aCurrentTarget)

Similar issues here.

> nsDOMEvent::GetOriginalTarget(nsIDOMEventTarget** aOriginalTarget)

And here.

> NS_METHOD nsDOMEvent::DuplicatePrivateData()

Please file a followup bug if we don't have one yet to have some sort of header we could include to handle this?  This function is mostly data, not code.

>+      newEvent = new nsEvent(PR_FALSE, msg);

So I assume there's a good reason these are all PR_FALSE for untrusted instead of whatever mEvent is?  If so, document that reason?

In general, this seems like a good place for copy constructors if I ever saw one.  ;)

>+    case NS_POPUP_EVENT:
>+      newEvent = new nsInputEvent(PR_FALSE, msg, nsnull);
>+      NS_ENSURE_TRUE(newEvent, NS_ERROR_OUT_OF_MEMORY);
>+      newEvent->eventStructType = NS_POPUP_EVENT;
>+      break;

Doesn't the nsInputEvent ctor set the eventStructType itself?  Similar for the SVG events...

>+  newEvent->target                 = mEvent->target;
>+  newEvent->currentTarget          = mEvent->currentTarget;
>+  newEvent->originalTarget         = mEvent->originalTarget;

What about copying flags (like NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY say)?

For that matter, isn't NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY write-only with your patch?  That seems wrong to me...  Either fix things so it's still used or remove it all the way?

I'm up to the beginning of nsEventListenerManager.cpp.

Oh, one other thing.  http://beaufour.dk/jst-review/?patch=https%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D213475%26action%3Dview shows some nits.  A lot of them are bogus (eg all of the "O" ones), but the "W", "{", "F" ones are real.  Also possibly the "R" and "L" ones.  Fix them as you see fit?  Just make sure to fix the else-after-return and the indent error that causes the "{" warning.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 59 • 18 years ago

(In reply to comment #58)
> 
> Doesn't the nsInputEvent ctor set the eventStructType itself? 
> Similar for the SVG events...

Only the protected ctors.

> 
> For that matter, isn't NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY write-only 
> with your patch? 

No, you can remove it by calling initEvent().

	Boris Zbarsky [:bzbarsky]
	Comment 60 • 18 years ago

> No, you can remove it by calling initEvent().

That's still writing the flag.  What I meant is that no one reads it.  No one ever checks whether it's set.  We used to not allow dispatch of an event with that flag set, but with this patch we will, no?

Good point on the constructors.  I still say we should have sane copy ctors; probably a followup.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 61 • 18 years ago

Fixing comments, except perhaps some more documentation is still needed
in nsEventDispatcher.h (that is also related to Bug 329124).

Checking NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY was really missing, now
it is back for DOMEvents.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 62 • 18 years ago

jst-review complains still about some cases, but I don't think it makes
sense to fix all of them.

	Boris Zbarsky [:bzbarsky]
	Comment 63 • 18 years ago

Yeah, the output of jst-review is advisory.  ;)  Still reviewing the parts I hadn't gotten to before...

	Boris Zbarsky [:bzbarsky]
	Comment 64 • 18 years ago

Comment on attachment 213816 [details] [diff] [review]
addressing bz's comments

>Index: content/base/public/nsINode.h
>+   * This is used to create the event target chain and implementations
>+   * should set the necessary members of nsEventChainPreVisitor.

So I'm still not very happy, if only because nsEventChainPreVisitor has all sorts of members this method could set (eg mPresContext).  Some of those members should probably be protected...  The rest should be clearly documented as being settable by PreHandleEvent as needed.  A lso, a number of impls of this method set the event status, and how is that really different from mPresContext in terms of the docs?

>+   * Usually setting aVisitor.mCanHandle and aVisitor.mParentTarget.
>+   * First one tells that this object can handle the aVisitor.mEvent event and
>+   * the latter one is the possible parent object for the event target chain.

Possible, or definite?  Is an implementation of this method allowed to not set aVisitor.mCanHandle?  This description sounds like that would be odd, but ok.

nsDOMEvent::DuplicatePrivateData is still pretty broken (eg doesn't copy the
NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY flag).  I guess that'll be fixed in bug 329127?

>Index: content/events/src/nsEventListenerManager.cpp

>+nsEventListenerManager::GetListenerManager(PRBool aCreateIfNotFound,
>+  *aResult = NS_STATIC_CAST(nsIEventListenerManager*, this);

I don't think you need this cast.

>Index: content/events/src/nsEventStateManager.cpp
>@@ -562,51 +563,50 @@ nsEventStateManager::PreHandleEvent(nsPr
>+              blurevent.flags |= NS_EVENT_FLAG_CANT_BUBBLE;

Did we use to not bubble blur events?  If not, could we change behavior in a separate bug?  In particular, note bug 238987 comment 2...  In general, behavior changes to the fragile mess that is focus code should be avoided as part of large patches.  ;)

>@@ -628,26 +628,34 @@ nsEventStateManager::PreHandleEvent(nsPr
>+            //XXXsmaug bubbling for now, because in the old event dispatching
>+            //         code focus event did bubble from document to window.
>+            nsEventDispatcher::Dispatch(mDocument, aPresContext,
>+                                        &focusevent, nsnull, &status);

File a followup bug, mention bug number here?

>+              focusevent.flags |= NS_EVENT_FLAG_CANT_BUBBLE;

Again, if we used to let these bubble, please keep doing it for now.  Same for elsewhere in this file, so I'll stop mentioning these.

>@@ -4225,43 +4251,44 @@ nsEventStateManager::SendFocusBlur(nsPre
>+      nsEventDispatcher::Dispatch(temp, gLastFocusedPresContext, &event, nsnull,
>+                                  &status);
...
>+      nsEventDispatcher::Dispatch(window, gLastFocusedPresContext, &event,
>+                                  nsnull, &status);

Don't we need to null out event.target here?

Also, do things like originalTarget, etc not need to be nulled out when nsEvents are reused?

Frankly, I think we should disallow reuse of nsEvents -- make nsEventDispatcher assert if an nsEvent (with no associated DOMEvent) has NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY set, and fix these callers to just create new nsEvent structs as needed.  The current system makes it too easy to make mistakes.  I guess a followup bug is ok for this, esp if we do it as part of the nsEvent/nsDOMEvent merge or something.

>@@ -4327,20 +4358,24 @@ nsEventStateManager::SendFocusBlur(nsPre
>+    //XXXsmaug bubbling for now, because in the old event dispathing code
>+    //         focus event did bubble from document to window.
>+    //event.flags |= NS_EVENT_FLAG_CANT_BUBBLE;

Again, if this is something that has a bug covering it, cite the bug# in the comment, please.  Same for other XXX comments you add.  If there is no existing bug, please file one.

>Index: content/html/content/src/nsGenericHTMLElement.cpp

>+nsGenericHTMLElement
>+::PostHandleEventForAnchors(nsEventChainPostVisitor& aVisitor)

I think it's ok to ignore jst-review here.  ;) If not, I'd put the args on the next line, indented a bit.  Same for other places where you did this.

>Index: content/html/content/src/nsHTMLButtonElement.cpp
>@@ -308,181 +304,195 @@ nsHTMLButtonElement::HandleDOMEvent(nsPr
>+  //XXXsmaug Should this use NS_UI_ACTIVATE, not NS_MOUSE_LEFT_CLICK?
>+  PRBool bInSubmitClick = mType == NS_FORM_BUTTON_SUBMIT &&

FWIW, that's bug 309348 comment 16.  Want to just put that bug# in the comment while you're here?

>+nsHTMLButtonElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)

>+  if (aVisitor.mItemFlags & NS_IN_SUBMIT_CLICK && mForm) {

Please put parens around the '&' expression?  I know it's OK as is, but I had to look up the operator precedence table in K&R to recall which of those binds tighter....

>       case NS_MOUSE_LEFT_BUTTON_DOWN:
...
>+          if (aVisitor.mPresContext) {

Why the null-check?  That's handled up at the top of the method, no?

>+      //XXXsmaug What to do with these events? Why these should be cancelled?
>+      //         And why only if there is a DOM Event? Disable the code for now.

I'm not sure what "Disable the code for now" means here.  In any case, file a followup bug to look into this; reference bug# here.

>       case NS_MOUSE_ENTER_SYNTH:
...
>+          if (aVisitor.mPresContext) {

Why null-check?

>       case NS_MOUSE_EXIT_SYNTH:
...
>+          if (aVisitor.mPresContext) {

And here?

>Index: content/html/content/src/nsHTMLFormElement.cpp

>+nsHTMLFormElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)

>+      if (mGeneratingSubmit) {
>+        aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
>+        return nsGenericHTMLElement::PreHandleEvent(aVisitor);

So... This isn't what the code used to do.  The event used to not propagate at all; that's not what your code does.  Please restore the old functionality.  Same for reset.  Would just setting aVisitor.mCanHandle to false work here?  Ideally, we wouldn't even enter PostHandleEvent for these events; see below.

>+nsHTMLFormElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)
>+{
>+    if (msg == NS_FORM_SUBMIT) {
>+      // let the form know not to defer subsequent submissions
>+      mDeferSubmission = PR_FALSE;

So... We shouldn't be resetting mDeferSubmission if mGeneratingSubmit was true when we called PreHandleEvent on this event.  Or at least the old code didn't.  Sounds like a use case for flags on the visitor?  Or can we just prevent PostHandleEvent from being called for such events?

>+    if (msg == NS_FORM_SUBMIT) {
>+      mGeneratingSubmit = PR_FALSE;
>+    }
>+    else if (msg == NS_FORM_RESET) {
>+      mGeneratingReset = PR_FALSE;
>+    }

Same here -- this should not happen if the event had the relevant boolean already true in PreHandleEvent.

>Index: content/html/content/src/nsHTMLImageElement.cpp

>+nsHTMLImageElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)

Ideally, this method would just die....  Could we file a bug to remove it, probably dependent on bug 127903?

>Index: content/html/content/src/nsHTMLInputElement.cpp

>+nsHTMLInputElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)
>+  if (!aVisitor.mPresContext) {
>+    return NS_OK;

I don't think that's right.  Note that we changed the state of the event in PreHandleEvent unconditionally; shouldn't we change it back here also unconditionall?  Similar for various other things (eg should a DOM click event on a submit with no prescontext submit?  IMO, yes).  So I think this null-check should happen when actually needed below.

>           // Check to see if focus has bubbled up from a form control's
>           // child textfield or button.  If that's the case, don't focus
>           // this parent file control -- leave focus on the child.
>           nsIFormControlFrame* formControlFrame = GetFormControlFrame(PR_FALSE);
>-          if (formControlFrame && !(aFlags & NS_EVENT_FLAG_BUBBLE) &&
>-              ShouldFocus(this))
>+          if (formControlFrame && ShouldFocus(this))

Doesn't this break what the comment was trying to do?  That is, shouldn't you have an originalTarget check here?

>@@ -1557,86 +1578,83 @@ nsHTMLInputElement::HandleDOMEvent(nsPre
...
>         } break; // NS_KEY_PRESS || NS_KEY_UP
> 
>-        // cancel all of these events for buttons
...
>+      // cancel all of these events for buttons

What happened with the indentation here?  Please fix so the cases in this switch are all indented the same amount.

>+          if (aVisitor.mDOMEvent) {
>+            nsevent = do_QueryInterface(aVisitor.mDOMEvent);

No need for that null-check.  do_QI does one.

>Index: content/html/content/src/nsHTMLLabelElement.cpp

>+nsHTMLLabelElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)
>+{
>+  if (content && !EventTargetIn(aVisitor.mPresContext, aVisitor.mEvent,
>+                                content, this)) {

We should have a followup bug for changing EventTargetIn to actually ask the event for the target instead of messing around with the ESM, prescontexts, etc... with your changes the event's target should always be right, no?

>+          nsEvent event(NS_IS_TRUSTED_EVENT(aVisitor.mEvent), NS_FOCUS_CONTENT);
>+          event.flags |= NS_EVENT_FLAG_CANT_BUBBLE;

Did this use to not bubble?

>Index: content/html/content/src/nsHTMLSelectElement.cpp
>+nsHTMLSelectElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)
...
>+  return NS_OK;

Shouldn't this call the superclass PostHandleEvent?  In fact, shouldn't that happen in general (eg in nsHTMLButtonElement, PostHandleEventForAnchors, nsHTMLFormElement, nsHTMLInputElement, nsHTMLLabelElement, and whatever classes I haven't looked at yet)?  Or is the assumption that the superclass shouldn't be doing default processing?

>Index: content/html/content/src/nsHTMLTextAreaElement.cpp
>+nsHTMLTextAreaElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)
>+{
>+  if (aVisitor.mEvent->message == NS_FORM_SELECTED) {
>     mHandlingSelect = PR_FALSE;

How do we know that's the NS_FORM_SELECTED that triggered mHandlingSelect, not one we bailed early on?

>Index: content/html/document/src/nsHTMLDocument.cpp
> nsHTMLDocument::CaptureEvents(PRInt32 aEventFlags)
>+  if (NS_SUCCEEDED(GetListenerManager(PR_TRUE, getter_AddRefs(manager)))) {

How about propagating the retval from GetListenerManager if it fails?  I know the code didn't use to do it... let's fix it.

> nsHTMLDocument::ReleaseEvents(PRInt32 aEventFlags)

Same here, except there's no reason to pass PR_TRUE to GetListenerManager here -- if there's no listener manager, we're not capturing either.

>Index: content/svg/content/src/nsSVGScriptElement.cpp
>@@ -281,18 +282,18 @@ nsSVGScriptElement::ScriptEvaluated(nsre
>     nsEvent event(PR_TRUE,
>                   NS_SUCCEEDED(aResult) ? NS_SCRIPT_LOAD : NS_SCRIPT_ERROR);
>+    nsEventDispatcher::Dispatch(NS_STATIC_CAST(nsIContent*, this),
>+                                presContext, &event, nsnull, &status);

NS_SCRIPT_LOAD shouldn't bubble, should it?  Certainly the script's load event shouldn't bubble per SVG spec (whereas error should).

>Index: content/xml/content/src/nsXMLElement.cpp
>+nsXMLElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)

>+          rv = TriggerLink(aVisitor.mPresContext, verb, uri,
>+                           target, PR_TRUE, PR_TRUE);

I know the code didn't use to null-check... but this will crash if the prescontext is null.  Add a check up where we check the event status (this is the handling of NS_MOUSE_LEFT_CLICK).

>     case NS_KEY_PRESS:

Mmm... Perhaps we should push DispatchClickEvent up from nsGenericHTMLElement to nsGenericElement and reuse it here!  Followup bug on that, I guess.

>     case NS_MOUSE_ENTER_SYNTH:

Should probably null-check aVisitor.mPresContext here too.

>     case NS_MOUSE_EXIT_SYNTH:

And here.

I'm up to the beginning of nsXULElement.cpp; I'll need to be more awake to review that one.  :(  With any luck, I should be able to finish this review up on Sunday; I'm sorry it's been so slow, but this code is hellish.  I'm so glad to see it becoming saner!

	neil@parkwaycc.co.uk
	Comment 65 • 18 years ago

FYI focus and blur events don't bubble, except that documents are a special case; all document events bubble to the window (compatibility I guess).

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 66 • 18 years ago

We used to bubble focus/blur events sometimes. Particularly, it seemed like we bubbled them whenever focus wasn't given to a text-field (textarea or input typ=text).

Additionally, in DOM Level 3 they will bubble. But that is of course not something that should be taken into account in this bug.

	Boris Zbarsky [:bzbarsky]
	Comment 67 • 18 years ago

So...  per DOM2, focus/blur do not bubble, right  What we could probably do is that events that don't bubble should actually bubble across anon content boundaries (so a focus on an anon textfield inside a binding would fire focus on the bound element, but not on anything between the textfield and the bound element or on anything above the bound element)... At least for some event types this would be the case.

That should probably happen in a followup bug, if we decide to do it.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 68 • 18 years ago

Hmm.. interesting, that should probably be added to the xbl2 spec.

	neil@parkwaycc.co.uk
	Comment 69 • 18 years ago

(In reply to comment #67)
>What we could probably do is that events that don't bubble should actually
>bubble across anon content boundaries (so a focus on an anon textfield inside
>a binding would fire focus on the bound element, but not on anything between
>the textfield and the bound element or on anything above the bound element)
This would be better than textbox's current xbl:inherits="onfocus,onblur" hack.

	Boris Zbarsky [:bzbarsky]
	Comment 70 • 18 years ago

I'm not quite sure yet whether this behavior should be for all non-bubbling events (eg onload?) or just onfocus/onblur.  In any case, it's followup bug fodder.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 71 • 18 years ago

(In reply to comment #64)
> nsDOMEvent::DuplicatePrivateData is still pretty broken (eg doesn't copy the
> NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY flag). 

It does copy flags. or do miss something...

> Did we use to not bubble blur events?  If not, could we change behavior in a
> separate bug?

I'll open a bug for focus/blur events. Need to sort out whether to bubble or not. I did try to not to change anything.
 
> >@@ -4225,43 +4251,44 @@ nsEventStateManager::SendFocusBlur(nsPre
> >+      nsEventDispatcher::Dispatch(temp, gLastFocusedPresContext, &event, nsnull,
> >+                                  &status);
> ...
> >+      nsEventDispatcher::Dispatch(window, gLastFocusedPresContext, &event,
> >+                                  nsnull, &status);
> 
> Don't we need to null out event.target here?

No. In the current code Document is the target when dispatching to window.

> Also, do things like originalTarget, etc not need to be nulled out when
> nsEvents are reused?

No need to null originalTarget. This is event documented in nsEventDispatcher.

> Frankly, I think we should disallow reuse of nsEvents.
> I guess a followup bug is ok for this,

yes, a followup bug.

> I'm not sure what "Disable the code for now" means here.  In any case, file a
> followup bug to look into this; reference bug# here.

Oops, leftover from a previous patch ;)

> >Index: content/html/content/src/nsHTMLFormElement.cpp
> 
> >+nsHTMLFormElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)
> 
> >+      if (mGeneratingSubmit) {
> >+        aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
> >+        return nsGenericHTMLElement::PreHandleEvent(aVisitor);
> 
> So... This isn't what the code used to do.  The event used to not propagate at
> all; that's not what your code does.  Please restore the old functionality. 
> Same for reset.  Would just setting aVisitor.mCanHandle to false work here? 
> Ideally, we wouldn't even enter PostHandleEvent for these events; see below.

Yes, aVisitor.mCanHandle = PR_FALSE is the right thing here, and
then PostHandleEvent isn't called.

> Or is the assumption that the superclass shouldn't
> be doing default processing?

There has been a such assumption ;) It is really up to each implementation
to decide whether to forward to superclass.

> >Index: content/html/content/src/nsHTMLTextAreaElement.cpp
> >+nsHTMLTextAreaElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)
> >+{
> >+  if (aVisitor.mEvent->message == NS_FORM_SELECTED) {
> >     mHandlingSelect = PR_FALSE;
> 
> How do we know that's the NS_FORM_SELECTED that triggered mHandlingSelect, not
> one we bailed early on?
> 

Because PreHandleEvent sets aVisitor.mCanHandle = PR_FALSE
if mHandlingSelect is PR_TRUE.

New patch coming.

	Boris Zbarsky [:bzbarsky]
	Comment 72 • 18 years ago

> It does copy flags. or do miss something...

Er... I must have been looking at the first version of the patch there.  Or something.  :(

> No. In the current code Document is the target when dispatching to window.

Add a comment making it clear that this is on purpose, please?

> It is really up to each implementation to decide whether to forward to
> superclass.

Sure, but I feel that the implementations in question should forward....  Maybe it's just me.

> Because PreHandleEvent sets aVisitor.mCanHandle = PR_FALSE

Ah, ok.  Worth a comment in PostHandleEvent, perhaps.

	Boris Zbarsky [:bzbarsky]
	Comment 74 • 18 years ago

Comment on attachment 214098 [details] [diff] [review]
fixing comments, adding some documentation

We should document that setting mCanHandle to false will prevent all event dispatch, including PostHandleEvent calls.

>Index: content/events/src/nsEventStateManager.cpp

So the changes here still make us not bubble focus/blur events.  Right now we DO bubble such events.  We shouldn't change that as part of this patch, in my opinion.  Please undo that change.

>Index: content/html/content/src/nsHTMLInputElement.cpp
>+nsHTMLInputElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)
>+  //FIXME Allow submission etc. also when there is no prescontext?
>+  if (disabled || !aVisitor.mPresContext) {

File a bug to address this and reference the bug# here, please.  Note that this is blocking _all_ events (mutation events, user-dispatched DOM events, etc) when there is no prescontext.  I'm pretty sure we do NOT want to ship that in 1.9a, hence the request for a bug so there's something I can mark blocking+.  ;)

>Index: content/html/content/src/nsHTMLLabelElement.cpp
>+          event.flags |= NS_EVENT_FLAG_CANT_BUBBLE;

Again, this is changing behavior.  Please do that in a separate bug, ok?

>Index: content/html/document/src/nsHTMLDocument.cpp
> nsHTMLDocument::ReleaseEvents(PRInt32 aEventFlags)
>+  nsCOMPtr<nsIEventListenerManager> manager;
>+  nsresult rv = GetListenerManager(PR_FALSE, getter_AddRefs(manager));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return manager->ReleaseEvent(aEventFlags);

Crash if there wasn't an existing listener manager.  GetListenerManager can return null and NS_OK when PR_FALSE is passed.

>Index: content/xul/content/src/nsXULElement.cpp
>+nsXULElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)

>+    if (aVisitor.mEvent->message == NS_XUL_CLICK) {
>+        aVisitor.mEvent->message = NS_MOUSE_LEFT_CLICK;
>     }

This was only needed because this code needed to munge the target for NS_XUL_CLICK, right?  Please file a followup bug to remove NS_XUL_CLICK altogether and jusr use NS_MOUSE_LEFT_CLICK in nsXULElement::Click.

>Index: dom/public/base/nsPIDOMWindow.h

>+  
> class nsPIDOMWindow : public nsIDOMWindowInternal

Why add the blank line?

>+  virtual nsresult PreHandleEvent(nsEventChainPreVisitor& aVisitor) = 0;

I'm kinda sort wondering whether it would make sense to have another interface for the event stuff -- one that would, say, inherit from nsIDOMGCParticipant and that naINode and nsGlobalWindow could both inherit from....  At least worth thinking about for the future?

>Index: dom/src/base/nsGlobalWindow.cpp

>+nsGlobalWindow::PreHandleEvent(nsEventChainPreVisitor& aVisitor)

>+  PRUint32 msg = aVisitor.mEvent->message;

Why not do it earlier?  We've already checked ->message a few times by this point.

>+  // XXX The chrome can not handle this, see bug 51211
>+  if (mChromeEventHandler && msg != NS_IMAGE_LOAD) {

Hmm.  This looks wrong to me (eg, what about NS_SCRIPT_LOAD)?  See also bug 51211 comment 37, which it looks like I never followed up on.  Let's file a followup bug to sort out the exact behavior we want the *LOAD events (should we even have more than one?) to have during the capturing phase, ok?

>+nsGlobalWindow::PostHandleEvent(nsEventChainPostVisitor& aVisitor)
>-    // Execute bindingdetached handlers before we tear ourselves
>-    // down.

This comment got lost.  Please put it back?  We should probably file a followup bug on bfcache, since I doubt this works right with bfcache...

> nsGlobalWindow::CaptureEvents(PRInt32 aEventFlags)
> nsGlobalWindow::ReleaseEvents(PRInt32 aEventFlags)

Same comments here as I had earlier for the nsDocument methods of the same names.

>Index: dom/src/base/nsWindowRoot.cpp

HandleChromeEvent here kept mWindow alive through event dispatch.  How are we dealing with that now?  Should we set mWindow as the data on aVisitor or something perhaps?

>Index: layout/base/nsIChromeEventHandler.idl
>+	[noscript] void PreHandleChromeEvent(in nsEventChainPreVisitorRef aVisitor);

Fix the indent here?

>Index: layout/base/nsIPresShell.h

>   NS_IMETHOD HandleDOMEventWithTarget(nsIContent* aTargetContent,
>+                                        nsEvent* aEvent,
>+                                        nsEventStatus* aStatus) = 0;

And this indent is broken too.

>Index: layout/base/nsPresShell.cpp
>+  //public because nsPresShellEventCB needs this.

s/public/protected/ right?

Also, you need a forward-decl of nsPresShellEventCB before this class -- otherwise the friend decl gets confused and dies on some of our compilers.

>Index: layout/xul/base/src/nsImageBoxFrame.cpp
> HandleImageOnerrorPLEvent(PLEvent *aEvent)
>-  HandleImagePLEvent(content, NS_IMAGE_ERROR, NS_EVENT_FLAG_INIT);
>+  HandleImagePLEvent(content, NS_IMAGE_ERROR, NS_EVENT_FLAG_NONE);

This used to not bubble, but with this patch it will.  Should restore the old behavior...

>+++ content/events/public/nsEventDispatcher.h	2006-03-05 18:34:38.000000000 +0200
>+ * Portions created by the Initial Developer are Copyright (C) 2005

2006 by now.  :(

>+ * created. nsEventDispatcher (or internally nsEventTargetChainItem) creates

I don't think there's need to mention nsEventTargetChainItem here.

>+ * target and the creation continues until either mCanHandle member of the

"the mCanHandle member"

>+ * nsEventChainPreVisitor object is PR_FALSE or the mParentTarget does not

"is set to PR_FALSE"

>+ * point to a new target. Event target chain is created in stack.

"The event target chain is created on the stack"

>+ * If event needs retargeting mEventTargetAtParent must be set in

"If the event needs retargeting, mEventTargetAtParent must be set in PreHandleEvent or PreHandleChromeEvent.  In that case, the event will be dispatched to mEventTargetAtParent instead of mParentTarget."

>+ * Capture, target and bubble phases of the event dispatching are handled

"The capture, ... of event dispatch are ..."

>+ * While dispatching the event for the system evnt group PostHandleEvent

s/evnt/event/

>+ * (or PostHandleChromeEvent) is called right after calling event listener for

"calling event listeners"?

>+class nsEventChainVisitor {

>+   * The prescontext, possibly be nsnull.

"possibly nsnull"

>+   * @note Do not change this after creating the visitor object.
>+   */
>+  nsPresContext* mPresContext;

How about declaring it as:

  nsPresContext * const mPresContext;

and removing the comment?  Similar for the other "do not change" members?

>+  nsIDOMEvent*   mDOMEvent;

I really don't like this being public and settable....  Can we fix that, please?  I guess followup bug ok, but please make sure it happens.

>+   * Usually set in PreHandleEvent() and used in PostHandleEvent().

When would that not be the case?  If never, then s/usually//.

>+  PRUint16       mItemFlags;

Why PRUint16 and not PRUint32?  Because that's what they are for nsEventTargetChainItem?  If so, document that please.  It's very non-obvious.

>+   * Usually set in PreHandleEvent() and used in PostHandleEvent().

Again, is this really just "usually"?  Or any time it's used?

>+class nsEventChainPreVisitor : public nsEventChainVisitor {

>+   * Tells whether event target which is handling this Visitor object
>+   * can handle the event.

Maybe more like:

  "Member that must be set in PreHandleEvent or PreHandleChromeEvent by event targets.  If set to false, indicates that this event target will not be handling the event and construction of the event target chain is complete.  The target that sets mCanHandle to false is NOT included in the event target chain."

>+  PRPackedBool          mCanHandle;
>+  /**

Newline between those, please.  Same elsewhere in this class.

>+   * Mark this PR_TRUE iff the parent event target is a
>+   * chrome event handler.

  "Member that is set to PR_TRUE in PreHandleEvent or PreHandleChromeEvent if and only if mParentTarget is set to a chrome event handler".

>+  /**
>+   * If the event needs to be retargetted, this is the event target,

s/retargetted/retargeted/

>+  nsCOMPtr<nsISupports> mEventTargetAtParent;

So... the one and only one time this is used mEventTargetAtParent == mParentTarget, no?  Is there really a reason to have this not be a boolean indicating that mParentTarget should be used as the target?

>+ * If an nsDispatchingCallback object is used in the event dispatching,

 "If an nsDispatchingCallback object is passed to Dispatch,"

>+ * but before system event group.

  "before handling the system event group"

>+ * The generic class for event dispatching.
>+ * Must not be used outside Gecko!

Could enforce this somewhat by making it #ifdef MOZILLA_INTERNAL_API.  In fact, as much as possible of this header should be #ifdef MOZILLA_INTERNAL_API.   If you can do it with the whole header, great.

Better yet would be to put the visitors in an exported header (since those are needed outside of content due to being included in various interface files), but put nsEventDispatcher itself in a non-exported header...

That is, don't rely on comments to ensure good behavior if it can be enforced by the compiler.  People like to ignore comments.

>+   * Target should be nsINode, nsPIDOMWindow or nsIChromeEventHandler.

s/Target/aTarget/.  And perhaps s/should be/QI/ (since you don't actually require that the nsISupports* passed in be the nsISupports* that any of the above inherits from/

>+   * If the target of the aEvent is set before calling this method, that
>+   * one is used as the target (unless there is event


s/of the/of/

s/that one/the target of aEvent/.

>+   * retargeting) and the originalTarget of the DOM Event.

I'm not sure I follow the part about event retargeting....

>+   * Event target chain however uses aTarget always as the top of the list
>+   * during event dispatching.

  "aTarget is always used as the starting point for constructing the event target chain, no matter what the value of aEvent->target is."

>+   * aTarget or aEvent must never be nsnull!

You mean "and", not "or", no?  So:

  "Neither aTarget nor aEvent is allowed to be nsnull."

>+   * @note Use this method when dispatching nsEvent.

"dispatching an nsEvent".

>+  static nsresult Dispatch(nsISupports* aTarget,
>+                           nsPresContext* aPresContext,

How should aPresContext be related to aEvent?

>+                           nsIDOMEvent* aDOMEvent = nsnull,

Are there callers who don't pass this arg?  I'd argue that this arg should be required...

>+                           nsEventStatus* aEventStatus = nsnull,

Possibly this one too, though I guess we might have more callers that don't pass this one.

>+   * If aDOMEvent is not nsnull, it is used for dispatching
>+   * (aEvent can be then nsnull) and (if aDOMEvent is not |trusted| already),

s/can be then/can then be/

>+++ content/events/src/nsEventDispatcher.cpp	2006-03-05 17:48:45.000000000 +0200

>+ * Portions created by the Initial Developer are Copyright (C) 2005

2006.

>+class nsEventTargetChainItem

Toss in a two-line comment that this is a stack-allocated object represeting a single item in the event target chain or something like that?

>+    return !!(mFlags & NS_TARGET_CHAIN_TYPE_MASK);

I think I'd prefer "!= 0", but either way.

Document what GetNewTarget() actually returns (or alternately what mNewTarget actually stores)?  I think that would make the retargeting code _much_ more understandable.

>+    return !!(mFlags & NS_TARGET_CHAIN_FORCE_CONTENT_DISPATCH);

Same.

>+  nsresult CreateChainAndHandleEvent(nsEventChainPreVisitor& aVisitor,
>+                                     nsDispatchingCallback* aCallback);
>+
>+  nsresult HandleEventTargetChain(nsEventChainPostVisitor& aVisitor,
>+                                  PRUint32 aFlags,
>+                                  nsDispatchingCallback* aCallback);
>+  nsresult PreHandleEvent(nsEventChainPreVisitor& aVisitor);
>+
>+  nsresult HandleEvent(nsEventChainPostVisitor& aVisitor, PRUint32 aFlags);
>+
>+  nsresult PostHandleEvent(nsEventChainPostVisitor& aVisitor);

Document these?

>+nsEventTargetChainItem::nsEventTargetChainItem(nsISupports* aTarget,
>+                                               PRBool aTargetIsChromeHandler,
>+                                               nsEventTargetChainItem* aChild)
>+: mChild(aChild), mParent(nsnull), mFlags(0), mItemFlags(0)

Fix indent?

>+    nsCOMPtr<nsIChromeEventHandler> ceh = do_QueryInterface(aTarget);
>+    if (ceh) {
>+      NS_ADDREF(mChromeHandler = ceh);

How about ceh.swap(mChromeHandler)?  That does require setting it to null in the initializers, though (which  I think you should do).

Similar for elsewhere in this method.

Add an NS_POSTCONDITION that only one of the type flags is set?

>+nsEventTargetChainItem::CurrentTarget()

If this can return null, it should be GetCurrentTarget(), imo.

So I wonder whether it would be even simpler to store the target to use for the event in every nsEventTargetChainItem (as opposed to only storing it when it changes).  I guess that may require more addref/release....  Let's stick with what we have for now.  ;)

>+nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor& aVisitor,

>+  // There is something which should be added to the event target chain.
>+  if (aVisitor.mParentTarget) {

That comment should be inside the if, no?

>+    nsEventTargetChainItem targetEtci(aVisitor.mParentTarget,

How about naming this parentEtci?

>+      // so that also the next retargetting works.

s/retargetting/retargeting/

>+      // The new event target chain item can handle the event. Continue

s/new/parent/ perhaps?

>+    // Parent can't handle the event, so make sure it is not in the chain
>+    this->mParent = nsnull;

Wouldn't this Just Work if ~nsEventTargetChainItem nulled out mChild->mParent if mChild is non-null?  That seems like the safer way to go anyway to me (prevents dangling pointers).

>+nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor& aVisitor, PRUint32 aFlags,

>+  aVisitor.mEvent->flags |= NS_EVENT_FLAG_CAPTURE;

Shouldn't you unset NS_EVENT_FLAG_BUBBLE here?  Otherwise the event's GetEventPhase method is going to be confused....

>+    if ((!(aVisitor.mEvent->flags & NS_EVENT_FLAG_NO_CONTENT_DISPATCH) ||
>+        item->ForceContentDispatch()) &&

Fix the indent, please?  It's very confusing right now.

>+    // item is at anonymous boundary. Need to retarget for the child items.
>+    if (item->GetNewTarget()) {

That comment belongs inside the if, not outside.

>+      while (nextTarget) {
...
>+        nextTarget = nextTarget->mChild;
>+      }

Assert that nextTarget is non-null here?  Otherwise you never found the original chain item that got a newtarget forced on it...

>+  if (!(aVisitor.mEvent->flags & NS_EVENT_FLAG_STOP_DISPATCH) &&
>+      (!(aVisitor.mEvent->flags & NS_EVENT_FLAG_NO_CONTENT_DISPATCH) ||
>+      item->ForceContentDispatch())) {

Fix the indent here.

>+    // XXX Should use aFlags & NS_EVENT_BUBBLE_MASK because capture phase
>+    //     event listeners should not be fired. But it breaks at least
>+    //     <xul:dialog>'s buttons.

Whoa.  We have bugs on this, don't we?  If so, please cite the bug#.  If not, please make sure a bug is filed (and blocking bug 235441).  And then cide the bug#.  I suppose you could just make bug 235441 be the bug that covers this....

>+      // Item is at anonymous boundary. Need to retarget for the current item
>+      // and for parent items.
>+      nsISupports* newTarget = item->GetNewTarget();
>+      if (newTarget) {

Again, comment belongs inside the if here.  Or it should start with "If the item ...."

>+      if ((!(aVisitor.mEvent->flags & NS_EVENT_FLAG_NO_CONTENT_DISPATCH) ||
>+          item->ForceContentDispatch()) &&

Fix the indent, please.

>+          aVisitor.mEventStatus != nsEventStatus_eConsumeNoDefault &&

Should aVisitor.mEventStatus really affect dispatch to bubbling handlers in the default event group?  That seems doubtful to me...

>+    aVisitor.mEvent->flags |= NS_EVENT_FLAG_CAPTURE;

(after bubbling).  Why?  That seems odd to me.

>+    // Setting back the original event.
>+    aVisitor.mEvent->target = aVisitor.mEvent->originalTarget;
....
>+    // retarget for system event group (which does the default handling too).
>+    aVisitor.mEvent->target = firstTarget;

Might want to toss in a comment about firstTarget being the first thing to dispatch to during capture, so the "parent-most" target, and not at all the same as mEvent->originalTarget.

>+nsEventDispatcher::Dispatch(nsISupports* aTarget,

>+  NS_ENSURE_TRUE(!(aDOMEvent &&
>+                   aEvent->flags & NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY),

Parens around the '&' expression, please?

>+#ifdef DEBUG
>+  if (aEvent && aDOMEvent) {

aEvent isn't going to be null anyway (certainly if aDOMEvent is non-null, since we dereferenced it for the NS_ENSURE_TRUE above in that case).  So just check aDOMEvent here.

>+  PRBool externalDOMEvent = !!(aDOMEvent);

I'd prefer "!= nsnull", but either way.

>+  // At least the original target can handle the event.
>+  if (preVisitor.mCanHandle) {

Comment should be inside the if.

>+    // Setting the retarget to the |target| simplifies retargetting code.

s/retargetting/retargeting/

>+  // An nsDOMEvent was created while dispatching the event.
>+  // Duplicate private data if someone holds a pointer to it.
>+  if (!externalDOMEvent && preVisitor.mDOMEvent) {

Again, put the comment inside the if.

>+    NS_RELEASE2(preVisitor.mDOMEvent, rc);
>+    if (0 != rc) {

rc will be nonzero if and only if preVisitor.mDOMEvent is non-null.  Since you're QIing that anyway, no need to check rc; the null-check of privateEvent is enough.

>+nsEventDispatcher::DispatchDOMEvent(nsISupports* aTarget,

Should this really return NS_OK if aDOMEvent isn't an nsIPrivateDOMEvent?  Wouldn't throwing NS_ERROR_ILLEGAL_VALUE make more sense?

>+  nsresult rv = NS_OK;

This is unused.... why's it even here?  That is, this method always returns NS_OK.

>+  } else if (aEvent) {

And if not?  Should we be throwing?  Or something?

sr=bzbarsky with the comments addressed and the followup bugs filed as needed.  Again, thanks for doing this -- this is great stuff!

	timeless
	Comment 75 • 18 years ago

i need a cc on any bugs changing event behavior. we generally rely on bubbling behavior and would be seriously harmed if any of that stuff changed.

	neil@parkwaycc.co.uk
	Comment 76 • 18 years ago

(In reply to comment #74)
>So the changes here still make us not bubble focus/blur events.
>Right now we DO bubble such events.
We do?

	Boris Zbarsky [:bzbarsky]
	Comment 77 • 18 years ago

Load

data:text/html,<div onfocus="document.getElementById('x').innerHTML='focus'"><a href="http://x">tab to me</a><span id="x">

Tab to the <a>.  Watch onfocus bubble to the <div>.

	neil@parkwaycc.co.uk
	Comment 78 • 18 years ago

(In reply to comment #77)
> data:text/html,<div onfocus="document.getElementById('x').innerHTML='focus'"><a
> href="http://x">tab to me</a><span id="x">
So what makes <input> different? I was trying data:text/html,<div onfocus="document.getElementById('x').innerHTML='focus'"><input value="tab to me"><span id="x">

	Boris Zbarsky [:bzbarsky]
	Comment 79 • 18 years ago

> So what makes <input> different?

Editor explicitly prevents bubbling.  See bug 4033 comment 56.

	neil@parkwaycc.co.uk
	Comment 80 • 18 years ago

(In reply to comment #79)
>>So what makes <input> different?
>Editor explicitly prevents bubbling.  See bug 4033 comment 56.
How on earth do you remember all this stuff?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 81 • 18 years ago

Btw, turns out I remember wrong. focus/blur will *not* bubble in DOM L3 (at least in the current draft)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 82 • 18 years ago

Didn't notice any problems with Tp, Txul, Ts etc, but
Zdiff and mZdiff went up ~8500. That is probably because of the
new members in nsEvent. That should be fixed if/when merging nsEvent and 
nsDOMEvent. Marking this bug fixed, but all regressions should depend on this.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 83 • 18 years ago

See the codesize output here: (search for "Output from EmbedCodesizeTest")

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1141752300.19479.gz&fulltext=1

	Boris Zbarsky [:bzbarsky]
	Comment 84 • 18 years ago

Actually, I'd mark regressions as blocking this, so we can tell them apart from just planned followups, etc.

	Håkan Waara
	Comment 85 • 18 years ago

Do you think this could have caused regression bug 329810 (middle-click doesn't work at all in Camino)?  Please have a look.

	Stefan Sitter [:ssitter]
	Comment 86 • 18 years ago

The check in from 2006-03-07 has probably regressed Bug 330020.

	Scott MacGregor
	Comment 87 • 18 years ago

I think this may have caused Bug 330360.

	Martijn Wargers (dead)
	Comment 88 • 18 years ago

This bug can be resolved fixed, can't it?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 89 • 18 years ago

yes.