329509 - Do not prevent event dispatching via dispatchEvent even if there is no prescontext or (form) element is disabled

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P1)

Description

[Olli Pettay [:smaug][[e-mail]]]

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

[[e-mail]]

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][[e-mail]]]

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

[[e-mail]]

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][[e-mail]]]

(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

[[e-mail]]

Is there a way to indicate that only chrome event handlers should be called?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(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

[[e-mail]]

(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][[e-mail]]]

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]

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 [:bzbarsky]]

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

[[e-mail]]

(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][[e-mail]]]

Attachment: fixes mutation events and explicit event dispatching

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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)]

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][[e-mail]]]

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 [:bzbarsky]]

I'm not going to have time for this review until after Sept 3.  :(

Comment 17

[Boris Zbarsky [:bzbarsky]]

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][[e-mail]]]

Attachment: 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][[e-mail]]]

Attachment: 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][[e-mail]]]

Attachment: -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 25

[Arthur Stolyar [:nekr]]

So, people still need the fix.

Comment 27

[Thomas Darling]

Olli, you've been assigned to this bug for 10 years now - will you please take your responsibility seriously and get this **** fixed?

I'm so incredibly tired of having to spend hours debugging, only to realize that Firefox, once again, has a bug that essentially makes several days of work useless. Now I have to come up with an entirely different solution, just because of this bug. Yes, I'm **** right now.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Thomas, Olli does take his responsibilities seriously, as you can see by the number of bugs he fixes if you cared to look.  Unfortunately, he has more on his plate than anyone could possibly have time for, so he has to prioritize.

Please refrain from personal abuse in this bug database in the future; that sort of behavior is not acceptable here.

Comment 29

[Thomas Darling]

Boris, I fully respect the amount of work he, and everyone else for that matter, does - but it's pretty disappointing when I have my precious time wasted due to bugs that have been ASSIGNED for 10+ years to the same person without progress. Your own guidelines state that you should not mess with bugs assigned to other people, so maybe such bugs should then be unassigned and left for others to pick up? Instead of having one person just holding on to it for a decade, while everyone else ignores it?

That's my opinion, and it's not a personal assault - it's a comment on what appears to be a broken issue tracking strategy, for a browser that is causing me and other web developers ever increasing pain.
End of discussion.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Feel free to take this bug.
And sorry that I've been busy with other stuff.
(I'll try to find time for this if no one else picks this up.)

Comment 31

[ilisepe1]

Hello all,

This bug is really driving me crazy. Imagine the following situation:
- You have a selectbox field that's disabled (since readonly attribute doesn't work as per html spec), I have no other standard option.
- You need to have this value updated depending on what the user will do with the rest of the form's elements

Check.

Your only alternative is to try to emulate the disabled look n feel like onfocus=this.blur() along with a css.
The only problem is that this currently doesn't work (bug https://bugzilla.mozilla.org/show_bug.cgi?id=1334155).
There might also be some other work-around, but image that this has to apply to any other element too.
And again, you never can be sure that the work-around won't be broken with the next version.

So, checkmate.

Please correct me if I'm missing something.

Btw, chrome doesn't have problem dispatching events to disabled items. On the other hand the onfocus=this.blur() doesn't work at all and seems like they won't fix it.

Comment 35

[Marcos Caceres [:marcosc]]

Ok, this is fairly trivial to fix... but there are a few caveats: 

  * We don't fire the event when `HTMLOptGroupElement` is false. Chrome also does not fire the events on `HTMLOptGroupElement`. However, in Safari, does fire the events. I don't see any reason not to fire the event. 
  
  * We have a non-conforming IDL-attribute `.disabled` on HTMLLinkElement that disables a style sheet (bug 1502712). However, that attribute is not a HTML attribute. 

I'll send an initial patch up for review.

Comment 36

[Marcos Caceres [:marcosc]]

* We don't fire the event when `HTMLOptGroupElement`'s `disabled` is false, I mean.

Comment 37

[Marcos Caceres [:marcosc]]

Attachment: Bug 329509 - allow dispatching event when element is disabled. r=smaug, annevk

Looking for feedback.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

I would expect that to break for example click handling? Do other browsers dispatch click on disabled controls?

Comment 39

[Marcos Caceres [:marcosc]]

(In reply to Olli Pettay [:smaug] (@TPAC, slower reviews) from comment #38)
> I would expect that to break for example click handling?

Will try.

> Do other browsers
> dispatch click on disabled controls?

Yes. Except Chrome follows us on HTMLOptGroupElement. But the rest all fire in Chrome and in Safari... no idea about Edge, will try once I'm back in AU.

Comment 40

[Marcos Caceres [:marcosc]]

(In reply to Marcos Caceres [:marcosc] from comment #39)
> (In reply to Olli Pettay [:smaug] (@TPAC, slower reviews) from comment #38)
> > I would expect that to break for example click handling?
> 
> Will try.
> 
> > Do other browsers
> > dispatch click on disabled controls?
> 
> Yes. Except Chrome follows us on HTMLOptGroupElement. But the rest all fire
> in Chrome and in Safari... no idea about Edge, will try once I'm back in AU.

Sorry I misread. Will try click in other browsers too.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Chrome doesn't dispatch click for example with
data:text/html,<input onclick="console.log(event)" disabled>

Comment 42

[Marcos Caceres [:marcosc]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58940966a1340d5200a5ce7b6fab9d3214891bae

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24d820f46375
allow dispatching event when element is disabled. r=annevk,smaug

Comment 44

[Marcos Caceres [:marcosc]]

Last try run found some issues. This one is happy:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd9edb968beeb7d3bd019533bd824525b55bea6c

Comment 45

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13826 for changes under testing/web-platform/tests

Comment 46

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/24d820f46375

Comment 47

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 48

[Chris Mills [:cmills]]

I've documented this by adding a note to the Fx 65 rel notes:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#HTML

Let me know if you think this needs anything else. I always assumed this was generally expected behaviour, and am surprised we didn't do this until now.

Comment 49

[Kohei Yoshino]

Posted site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/events-are-now-dispatched-on-disabled-form-widgets/

Comment 50

[ZeroUnderscoreOu]

I'd like to clarify if this is related to issue I'm facing: disabled inputs block CSS transition events.
Behavior is the same in FF 56 & FF 66 (Build ID 20190211185957).

Comment 51

[Marcos Caceres [:marcosc]]

(In reply to ZeroUnderscoreOu from comment #50)

I'd like to clarify if this is related to issue I'm facing: disabled inputs block CSS transition events.
Behavior is the same in FF 56 & FF 66 (Build ID 20190211185957).

If you have a reduced test case, I'd be happy to have take a look.

Comment 52

[ZeroUnderscoreOu]

Attachment: Transitions.html

Testcase file. It has 4 buttons (2 <Inputs>, 2 <Buttons>, 2 enabled, 2 disabled). Fifth button triggers styling changes, which leads to transitions. Handler logs events info to console.

In Firefox, transition events on disabled elements get "eaten". This is highlighted by a separate listener for an element which is inside a disabled button.

Console output in Firefox:

transitionend from Input_Enabled to Window
transitionend from Button_Enabled to Window
transitionend from Span_Enabled to Window
transitionend from Span_Disabled to Span_Disabled

In Chrome:

transitionend from Input_Enabled to Window
transitionend from Input_Disabled to Window
transitionend from Button_Enabled to Window
transitionend from Span_Enabled to Window
transitionend from Button_Disabled to Window
transitionend from Span_Disabled to Span_Disabled
transitionend from Span_Disabled to Window

I expect Firefox to behave the same way Chrome does, in this particular case.
Tested on Firefox 66.0b8 (Build ID 20190214195736) & Chrome 72.0.3626.109 64 bit, Windows 7 x64.

Comment 53

[Marcos Caceres [:marcosc]]

@smaug, I'm unsure where to find what controls the transition dispatching - but it's indeed not dispatching "transitionend" for disabled input elements? Any pointers would be helpful.

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Add the needed events to https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/dom/html/nsGenericHTMLElement.cpp#1962

(I wonder if we'll change that list to be black list including only eClick or so)

Comment 55

[Marcos Caceres [:marcosc]]

(In reply to Olli Pettay [:smaug] from comment #54)

Add the needed events to https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/dom/html/nsGenericHTMLElement.cpp#1962

Thanks!

(I wonder if we'll change that list to be black list including only eClick or so)

Yeah, might make sense. There is also that other more bogus stuff we might be able to get rid of:

  // FIXME(emilio): This poking at the style of the frame is slightly bogus
  // unless we flush before every event, which we don't really want to do.
  if (aFrame && aFrame->StyleUI()->mUserInput == StyleUserInput::None) {
    return true;
  }

Anyway, will add the event cases to the switch and we can wait and see other web compat issues come up.

Comment 56

[Marcos Caceres [:marcosc]]

ZeroUnderscoreOu, the patch in bug 1530239 will be landing soon to fix the transitions issue (just doing final checks before merge). Thank you for reporting the issue and for the test case - it was quite helpful!

Comment 57

[ZeroUnderscoreOu]

Glad I was able to help. Thank you for your work!

Comment 58

[Marcos Caceres [:marcosc]]

Also fixing for CSS Animations 🎉 They had the same issue (bug #1531605).

Comment 59

[robbendebiene]

I may be doing something wrong, but for me (Firefox 66.0.5 - Linux) the issue still persists. Events are not dispatched when the element is disabled. Example here: https://codepen.io/anon/pen/vWMNjV

Comment 60

[Boris Zbarsky [:bzbarsky]]

This bug was specifically about manually-dispatched (via dispatchEvent) events. That testcase is using actual click events generated by user interaction, which is a very different situation.

Comment 61

[Marcos Caceres [:marcosc]]

robbendebiene, please follow:
https://bugzilla.mozilla.org/show_bug.cgi?id=1540995