Closed Bug 329509 Opened 18 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: smaug, Assigned: marcosc)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [webcompat:p2][wptsync upstream])

Attachments

(5 files, 1 obsolete file)

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.
Blocks: 274626
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?)
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.
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.
(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?

Is there a way to indicate that only chrome event handlers should be called?
(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.
(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.
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?
I'm not really sure what we're doing with HTML specific event issues, i've raised that issue with the WG.
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?
(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.
Assignee: events → Olli.Pettay
Status: NEW → ASSIGNED
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.
Attachment #234919 - Flags: review?(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
Attachment #234919 - Flags: review?(jst) → review+
Comment on attachment 234919 [details] [diff] [review]
fixes mutation events and explicit event dispatching

Boris, what do you think about this?
Attachment #234919 - Flags: superreview?(bzbarsky)
I'm not going to have time for this review until after Sept 3.  :(
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.
Attachment #234919 - Flags: superreview?(bzbarsky) → superreview-
Summary: Do not prevent event dispatching even if there is no prescontext or (form) element is disabled → Do not prevent event dispatching even if (form) element is disabled
Summary: Do not prevent event dispatching even if (form) element is disabled → Do not prevent event dispatching even if there is no prescontext or (form) element is disabled
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.
(Mutation event handling without prescontext is bug 388746.)
Attachment #234919 - Attachment is obsolete: true
Attachment #272949 - Flags: review?(jst)
Attached patch -wSplinter Review
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.
Attachment #272950 - Attachment is patch: true
Attachment #272950 - Attachment mime type: text/x-patch → text/plain
Attachment #272949 - Flags: review?(jst)
QA Contact: ian → events
Blocks: 218093
So, people still need the fix.
Blocks: 1067886
Blocks: 541110
Blocks: 963529
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.
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.
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.)
Assignee: bugs → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(bugs)
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.
Priority: -- → P2
Assignee: nobody → mcaceres
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.
Priority: P2 → P1
* We don't fire the event when `HTMLOptGroupElement`'s `disabled` is false, I mean.
I would expect that to break for example click handling? Do other browsers dispatch click on disabled controls?
(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.
(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.
Chrome doesn't dispatch click for example with
data:text/html,<input onclick="console.log(event)" disabled>
Attachment #9020598 - Attachment description: Bug 329509 - allow displatching event when element is disabled. r=smaug → Bug 329509 - allow dispatching event when element is disabled. r=smaug, annevk
Pushed by mcaceres@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24d820f46375
allow dispatching event when element is disabled. r=annevk,smaug
Flags: webcompat?
Whiteboard: [webcompat:p2]
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13826 for changes under testing/web-platform/tests
Whiteboard: [webcompat:p2] → [webcompat:p2][wptsync upstream]
https://hg.mozilla.org/mozilla-central/rev/24d820f46375
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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.
Keywords: site-compat

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

Flags: needinfo?(mcaceres)

(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.

Flags: needinfo?(mcaceres)
Attached file 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.

@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.

Flags: needinfo?(bugs)

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)

Flags: needinfo?(bugs)

(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.

Blocks: 1530239

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!

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

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

Depends on: 1540995

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

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.

Summary: Do not prevent event dispatching even if there is no prescontext or (form) element is disabled → Do not prevent event dispatching via dispatchEvent even if there is no prescontext or (form) element is disabled
Regressions: 1550873
Depends on: 1610821
No longer depends on: 1540995
Regressions: 1540995
Regressions: 1693454
You need to log in before you can comment on or make changes to this bug.