Last Comment Bug 329509 - Do not prevent event dispatching even if there is no prescontext or (form) element is disabled
: Do not prevent event dispatching even if there is no prescontext or (form) el...
Status: ASSIGNED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: ---
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
: 534510 561784 867418 880689 1107929 (view as bug list)
Depends on: 234455
Blocks: 541110 963529 1067886 218093 274626
  Show dependency treegraph
 
Reported: 2006-03-06 08:33 PST by Olli Pettay [:smaug] (vacation Aug 25-28)
Modified: 2015-07-29 19:48 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fixes mutation events and explicit event dispatching (30.17 KB, patch)
2006-08-22 03:52 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
jst: review+
bzbarsky: superreview-
Details | Diff | Splinter Review
some tests (synthetic event dispatch and also manually testable) (2.56 KB, text/html)
2007-07-19 04:13 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details
Disable event handling only when doing "full event dispatch" via ESM/Presshell (13.67 KB, patch)
2007-07-19 04:20 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
-w (12.16 KB, patch)
2007-07-19 04:23 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (vacation Aug 25-28) 2006-03-06 08:33:30 PST
Do not prevent event dispatching even if there is no prescontext or (form) element is disabled. Currently for example mutation events don't work if form elements are
disabled.
Comment 1 neil@parkwaycc.co.uk 2006-03-08 03:36:23 PST
How would you prefer to handle it?
a) change the early return NS_OK; to return super::PreHandleEvent(aVisitor);
b) change the sense of the conditions and indent all the code
c) extract the control-specific code into a separate function (named?)
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-03-08 14:23:20 PST
Would it be enough to move the 
(disabled || !aVisitor || uiStyle->mUserInput == NS_STYLE_USER_INPUT_NONE ||
 uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED)
checks to PostHandleEvent and do default handling only if that
expression evaluates to false.
Comment 3 neil@parkwaycc.co.uk 2006-03-08 14:39:43 PST
Sadly form controls have to do some prehandling too. For instance, checkboxes need to update their value before the event propagates, but then need to reset their value if the event is subsequently cancelled.
Comment 4 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-03-09 08:37:10 PST
(In reply to comment #3)
> Sadly form controls have to do some prehandling too. For instance, checkboxes
> need to update their value before the event propagates, but then need to reset
> their value if the event is subsequently cancelled.
> 
Of course. :(
Well, then a) but perhaps adding flag to mItemFlags to indicate
that posthandling should not be done?

Comment 5 neil@parkwaycc.co.uk 2006-03-09 09:55:32 PST
Is there a way to indicate that only chrome event handlers should be called?
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-03-09 10:06:29 PST
(In reply to comment #5)
> Is there a way to indicate that only chrome event handlers should be called?
> 

What is a "chrome event handler"? Atm there is no way to detect that an event
listener was added in chrome. We could perhaps (at least in some cases) use
'trusted' flag for that, that is set by default when registering listeners in chrome.
Comment 7 neil@parkwaycc.co.uk 2006-03-09 12:55:38 PST
(In reply to comment #6)
>(In reply to comment #5)
>>Is there a way to indicate that only chrome event handlers should be called?
>What is a "chrome event handler"?
That's a good question. I was going to say it was an event handler on a event target in an object managed by a docshell with chrome type. However your idea of firing only trusted event handlers is equally valid.

For people not following the discussion on IRC the issue is that content does not expect to see events fired for disabled form controls. However this then causes bugs because our browser chrome (e.g. tooltips) does want to see them.
Comment 8 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-03-27 11:21:05 PST
Some events should be fired, I think. mousemove, mouseover, mouseout? but not click, keypress?
Jonas, I sent a mail to webapi list, but didn't get any comments.
Could you raise the issue on a conf call?
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-27 15:10:00 PST
I'm not really sure what we're doing with HTML specific event issues, i've raised that issue with the WG.
Comment 10 Boris Zbarsky [:bz] 2006-03-28 13:52:59 PST
If our only concern is chrome vs content, don't we have the NO_CONTENT_DISPATCH flag for that?

Note that I agree that we should fire mousemove....  click is a weird one; what do other browsers do?
Comment 11 neil@parkwaycc.co.uk 2006-03-28 15:40:45 PST
(In reply to comment #10)
>click is a weird one; what do other browsers do?
IE seems not to generate a click event when you click on a disabled input.
Comment 12 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-08-22 03:52:25 PDT
Created attachment 234919 [details] [diff] [review]
fixes mutation events and explicit event dispatching
Comment 13 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-08-22 10:31:43 PDT
Comment on attachment 234919 [details] [diff] [review]
fixes mutation events and explicit event dispatching

I think I want this :)
Makes DOM Events (when dispatched using dispatchEvent) to work always.
Also mutation events work and I think it is quite clear when an event is "presentation event", at least I hope so.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2006-08-23 15:08:31 PDT
Comment on attachment 234919 [details] [diff] [review]
fixes mutation events and explicit event dispatching

- In content/events/public/nsEventDispatcher.h:

   /**
+   * mPresentationDispatch is PR_TRUE whenever event is dispatched by native

Add "an" in front of "event".

r=jst
Comment 15 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-08-24 04:54:25 PDT
Comment on attachment 234919 [details] [diff] [review]
fixes mutation events and explicit event dispatching

Boris, what do you think about this?
Comment 16 Boris Zbarsky [:bz] 2006-08-24 22:01:00 PDT
I'm not going to have time for this review until after Sept 3.  :(
Comment 17 Boris Zbarsky [:bz] 2006-09-10 20:57:07 PDT
Comment on attachment 234919 [details] [diff] [review]
fixes mutation events and explicit event dispatching

>Index: content/events/public/nsEventDispatcher.h
>@@ -98,16 +100,25 @@ public:
>+   * mPresentationDispatch is PR_TRUE whenever event is dispatched by native
>+   * code which handles the UI and layout.

So it doesn't have to have anything to do with the presentation, then?  For example, should DOMContentLoaded events have mPresentationDispatch true or false?  What about DOMLinkAdded?  If they're dispatched via nsContentUtils::DispatchTrustedEvent they won't default to the same thing as if they're dispatched via Dispatch() or DispatchEvent()....

It feels like we need better docs here at least.

>   static nsresult Dispatch(nsISupports* aTarget,

Document what the new arg means?  For that matter, what does aCallback mean?  And aEventStatus?  How is aDOMEvent related to aEvent?  I feel like I asked some of these when I was reviewing this code....

Can we possibly make the new arg not optional?  It seems like the sort of thing where people will totally forget it's there, otherwise.

>   static nsresult DispatchDOMEvent(nsISupports* aTarget,


Same here, esp. because this function doesn't have any optional args to start with...  And again, document the arg.

I'd like to see a diff -w of content/html/content/src; that should be much more reviewable.
Comment 18 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-07-19 04:13:38 PDT
Created attachment 272948 [details]
some tests (synthetic event dispatch and also manually testable)

Opera doesn't prevent synthetic events when controls are disabled, Safari 
does seem to prevent (our current behavior which doesn't make sense). 
In Opera if click() is called on disabled control, click event is dispatched - not sure whether we want to do that.
Opera has some bug with disabled controls. After pressing some keys, disabled 
controls start to get keyup/down/press events.
Comment 19 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-07-19 04:20:21 PDT
Created attachment 272949 [details] [diff] [review]
Disable event handling only when doing "full event dispatch" via ESM/Presshell

(Mutation event handling without prescontext is bug 388746.)
Comment 20 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-07-19 04:23:43 PDT
Created attachment 272950 [details] [diff] [review]
-w

I don't know how else we could easily recognize that the event is really coming
from the user, not a synthetic event dispatched by some script.
The first patch for this bug had pretty much the same idea, but the implementation
was much uglier.
Comment 21 Boris Zbarsky [:bz] 2009-12-15 13:02:20 PST
*** Bug 534510 has been marked as a duplicate of this bug. ***
Comment 22 Boris Zbarsky [:bz] 2010-04-26 10:12:59 PDT
*** Bug 561784 has been marked as a duplicate of this bug. ***
Comment 23 Olli Pettay [:smaug] (vacation Aug 25-28) 2013-05-01 15:05:45 PDT
*** Bug 867418 has been marked as a duplicate of this bug. ***
Comment 24 :Ms2ger (⌚ UTC+1/+2) 2013-06-07 23:51:04 PDT
*** Bug 880689 has been marked as a duplicate of this bug. ***
Comment 25 Arthur Stolyar [:nekr] 2013-06-08 01:59:26 PDT
So, people still need the fix.
Comment 26 Boris Zbarsky [:bz] 2014-12-05 08:24:14 PST
*** Bug 1107929 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.