Closed Bug 234455 Opened 21 years ago Closed 19 years ago

Centralize event dispatch

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bryner, Assigned: smaug)

References

(Depends on 1 open bug, Blocks 11 open bugs)

Details

Attachments

(3 files, 16 obsolete files)

427.47 KB, patch
jst
: review+
Details | Diff | Splinter Review
435.23 KB, patch
bzbarsky
: superreview+
Details | Diff | Splinter Review
434.62 KB, patch
Details | Diff | Splinter Review
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.
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?
Blocks: 185758
Blocks: 202764
(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.
Blocks: 201236
Blocks: 235441
Should look into fixing bug 53118 with this too.
Blocks: 53118
Blocks: 59292
Blocks: 216841
Blocks: 242091
Blocks: 105280
Blocks: 245569
Blocks: 245571
No longer blocks: 245571
Blocks: 251197
Blocks: 218093
Blocks: 266958
Blocks: 267842
Blocks: 274626
Blocks: 272148
Blocks: 276948
Blocks: 191242
Blocks: 295340
Status: NEW → ASSIGNED
Assignee: bryner → smaug
Status: ASSIGNED → NEW
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.
Attached patch work in progress (obsolete) — — Splinter Review
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.
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!
(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).
Status: NEW → ASSIGNED
(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.
(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.
> 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.
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.
(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?
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!
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.
I don't have a strong preference on that...
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);
I'll move event handling methods from nsIContent/nsIDocument to nsINode.
Attached patch somewhat tested patch. (obsolete) — — Splinter Review
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.
Attachment #208601 - Attachment is obsolete: true
(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 :(
Does it work without this patch? Note bug 322565
Attached patch 'load' must not bubble. (obsolete) — — Splinter Review
> 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.
Attachment #209304 - Attachment is obsolete: true
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
Attached patch A fix to make it compile also on mac (obsolete) — — Splinter Review
Attachment #209328 - Attachment is obsolete: true
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.
Oh, definitely. A lot of the deps are just things that _should_ be fixable once this is fixed...
Attached patch better not to break bfcache (obsolete) — — Splinter Review
Attachment #209424 - Attachment is obsolete: true
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 ;)
Attachment #209474 - Attachment is obsolete: true
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).
(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.
Attached patch up-to-date (obsolete) — — Splinter Review
Attachment #209506 - Attachment is obsolete: true
Attached patch up-to-date (2) + a fix to colorpicker (obsolete) — — Splinter Review
The very latest patch is always also in http://www.cs.helsinki.fi/u/pettay/moztests/events/
Attachment #209750 - Attachment is obsolete: true
Attachment #209877 - Flags: review?(jst)
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.
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...
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 :(
Attachment #209877 - Attachment is obsolete: true
Attachment #210029 - Flags: review?(jst)
Attachment #209877 - Flags: review?(jst)
Attachment #210029 - Attachment description: keep the prescontext handling like it is in the old code → keep the prescontext handling (almost) like it is in the old code
Attached patch previous + null checks (obsolete) — — Splinter Review
Argh, sorry. I forgot few null checks.
Attachment #210029 - Attachment is obsolete: true
Attachment #210051 - Flags: review?(jst)
Attachment #210029 - Flags: review?(jst)
I'm going to hold my mutation events work until this bug is fixed. I can't keep up with all the patches. ;)
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)
> 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.
> both nsIContent and nsIDOMEventReceiver have GetListenerManager method Perhaps we can eliminate one of those methods in a followup bug or something?
Depends on: 325652
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)
> 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.
Blocks: 325652
No longer depends on: 325652
> /** > + * 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.
(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.
Because the mItemData needed some changes in nsEventDispatcher, I think it is better to update the patch now.
Attachment #210051 - Attachment is obsolete: true
Attachment #210495 - Attachment is obsolete: true
Attachment #210621 - Flags: review?(jst)
Attachment #210051 - Flags: review?(jst)
Blocks: 325818
(In reply to comment #45) > Created an attachment (id=210621) [edit] > Oops, just noticed there is a typo _CENTENT, should be _CONTENT ofc ;)
Attached patch ...and up-to-date — — Splinter Review
Attachment #210621 - Attachment is obsolete: true
Attachment #211162 - Flags: review?(jst)
Attachment #210621 - Flags: review?(jst)
Blocks: 254036
Blocks: 224741
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!
Attachment #211162 - Flags: review?(jst) → review+
(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.)
Attached patch + fixes (obsolete) — — Splinter Review
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 ;)
Attachment #211492 - Flags: superreview?(bzbarsky)
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.
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. :(
Attached patch up-to-date (obsolete) — — Splinter Review
Attachment #211492 - Attachment is obsolete: true
Attachment #212414 - Flags: superreview?(bzbarsky)
Attachment #211492 - Flags: superreview?(bzbarsky)
Attached patch up-to-date (obsolete) — — Splinter Review
Attachment #212414 - Attachment is obsolete: true
Attachment #213475 - Flags: superreview?(bzbarsky)
Attachment #212414 - Flags: superreview?(bzbarsky)
Blocks: 328885
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.
Blocks: 208427
> 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 :)
> 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. :)
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.
Blocks: 329112
Blocks: 329119
Blocks: 329122
Blocks: 329124
Blocks: 329125
Blocks: 329126
Blocks: 329127
(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().
No longer blocks: 329126, 329127
> 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.
Attached patch addressing bz's comments (obsolete) — — Splinter Review
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.
Attachment #213475 - Attachment is obsolete: true
Attachment #213816 - Flags: superreview?(bzbarsky)
Attachment #213475 - Flags: superreview?(bzbarsky)
jst-review complains still about some cases, but I don't think it makes sense to fix all of them.
Yeah, the output of jst-review is advisory. ;) Still reviewing the parts I hadn't gotten to before...
Blocks: 329192
Blocks: 238987
Blocks: 196057
Blocks: 190876
Blocks: 309348
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!
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).
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.
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.
Hmm.. interesting, that should probably be added to the xbl2 spec.
(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.
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.
(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.
> 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.
Attachment #213816 - Attachment is obsolete: true
Attachment #213816 - Flags: superreview?(bzbarsky)
Blocks: 329430
Blocks: 329435
Blocks: 329437
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!
Attachment #214098 - Flags: superreview+
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.
(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?
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>.
(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">
> So what makes <input> different? Editor explicitly prevents bubbling. See bug 4033 comment 56.
(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?
Blocks: 329509
Blocks: 329512
Blocks: 329514
Btw, turns out I remember wrong. focus/blur will *not* bubble in DOM L3 (at least in the current draft)
Attached patch This was checked in — — Splinter Review
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.
Actually, I'd mark regressions as blocking this, so we can tell them apart from just planned followups, etc.
Depends on: 329682
Do you think this could have caused regression bug 329810 (middle-click doesn't work at all in Camino)? Please have a look.
Depends on: 329810
Blocks: 227962
Blocks: 330002
The check in from 2006-03-07 has probably regressed Bug 330020.
Depends on: 330020
Depends on: 330190
Depends on: 330013
Depends on: 330089
Depends on: 330360
Blocks: 330494
Depends on: 330428
I think this may have caused Bug 330360.
Depends on: 331374
Blocks: 331630
Blocks: 331989
No longer blocks: 331989
Depends on: 331989
Blocks: 333262
No longer blocks: 333262
Depends on: 333262
This bug can be resolved fixed, can't it?
yes.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Blocks: 335223
Depends on: 335265
Depends on: 336576
Depends on: 337520
Blocks: 281859
Depends on: 343303
Blocks: 263240
Blocks: 57597
Blocks: 66172
Blocks: 229089
Depends on: 364219
Depends on: 378247
Depends on: 378369
Depends on: 385519
Depends on: 388746
Depends on: 390995
Depends on: 402089
Depends on: 428135
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: