When the frame does not exist for a piece of content (as in the case of display: none), PresShell::HandleEventWithTarget() does not send the event to the content. This is wrong behavior, IMO. Some content has definite things it can do when it receives events, regardless of what frame it is using--forms with submit/reset events are a good example. When this is fixed, nsHTMLFormElement::Submit() and Reset() should be fixed to call HandleEventWithTarget() (they bypass HandleEventWithTarget() as of bug 125578).

I have changed my mind on this; the PresShell should not be the main place you send DOM events, period. You can still send DOM events to content in a display: none iframe and in those cases there will be no PresShell. Thus this shifts to "stop calling PresShell's HandleEventWithTarget() so much and provide an alternative."

This patch: (1) nsCOMPtr-izes and nsCOMArray-izes the event content in nsPresShell.cpp (2) makes HandleEventWithTarget() / HandleEvent() / HandleEventInternal() work when no frame exists (3) makes HandleDOMEventWithTarget() call HandleEventWithTarget() so that it has the second pass and Pre/PostHandleEvent and everything--in short, so that we only have one major event firing path in presshell (and perhaps can eventually collapse that with ESM) (4) tries to make frame / content setting more consistent, specifically ensure that content is immediately set (5) Moves the logic to figure out which frame the gui event refers to into its own function to make HandleEvent() more readable I have been running through a bunch of sites and no problems so far; haven't tested *every* possible function in Mozilla but I have run through a lot and it works as expected. Form submission and form controls work well; one thing I made sure to do was hit enter in an input to ensure that HandleDOMEventWithTarget() works.

CC'ing some events dudes. One thing that setting the content immediately does is it ensures that we always call GetContentForEvent() instead of GetContent(). GetContentForEvent() has several extra things it does, including the logic for image maps and canvas frame voodoo. I have not found a case where this matters yet, but I can guess there are a goodly number of edge cases where it does. The major thing this does is make HandleDOMEventWithTarget go through the same path as other stuff, which will aid in future element dispatch rewrite stuff by having fewer code paths to consolidate and test.

bryner and I spoke last night and the major potential objection to switching HandleDOMEventWithTarget() to use HandleEventWithTarget() is that retargeting would occur even if you do. Retargeting in HandleEventWithTarget() is pretty lame anyway--it should either (a) be done by the caller or (b) HandleEventWithTarget() should be named HandleGUIEventWithTarget() and call the real HandleEventWithTarget() after retargeting. It still has its merits: - event groups (system event group + normal) would happen for DOM events - PreHandleEvent / PostHandleEvent gets called (important when you fire mouse / key events at content)

Same patch, updated to tip. I had to merge in aaronl's keydead patch for bug 110718; I put that into HandleEventWithTarget() since it's not doing the same thing as SetRealEventInfo(). ctrl+T works fine while the document is in the in-between state. I will need to either (a) not have HandleDOMEventWithTarget call HandleEventWithTarget() for now and file another bug; or (b) fix the retargeting issues by moving retargeting to the callers. (b) is preferable but depends on time. This patch should not be reviewed, it's just for transport.

Comment on attachment 111407 [details] [diff] [review] Patch v1.0.1 So as it turns out, this patch *is* The One. HandleEventInternal() is the method being called, and that one doesn't have any retargeting.

Is there a real test case that this is fixing, or theoretical behavior? What does IE do in this case?

saari: this is theoretical behavior, and cleanup of the code. It is more of a stepping stone for other bugs I'm working on and helps pave the way to add event groups and do some of the other events cleanup. bryner has gone through it, btw, and was ok with it.

What this basically boils down to is that the major path in our code, the one in which we do rudimentary event groups (the system pass) and prehandle / posthandleevent stuff, *cannot* be used for many events and content. This fixes that and makes it so we have one place we can modify to do things like good event groups.

Matti, does that patch apply to current trunk?

Sorry, i don't know.. I just found it and I thought it would be nice to get itz in the tree if it still works....

Ah, I just tried... 9 of the 13 hunks don't apply. We do want this change (for 1.9, probably), but it'd basically have to be redone (with the patch as cribnotes, possibly).

Comment on attachment 111407 [details] [diff] [review] Patch v1.0.1 Removing request pending patch that actually applies and has been tested...

So... is this relevant in today's world? I only see the ESM calling HandleEventWithTarget now...