html icon indicating copy to clipboard operation
html copied to clipboard

Event bubbling on disabled form elements

Open josepharhar opened this issue 5 years ago • 71 comments

I came across this issue while re-triaging this chrome bug.

When clicking an element inside a disabled button element, chrome firefox and safari all don't dispatch click events on the disabled form element itself. However, chrome and safari both allow the click event to keep bubbling up the dom, whereas firefox stops the bubbling after the disabled button:

<div id=parent>
  <button disabled>
    <span id=child>click me</span>
  </button>
</div>
chrome safari firefox
#parent click click none
button none none none
#child click click click

In the case that there is no element inside the button, there is no bubbling that occurs in any of the browsers:

<div id=parent>
  <button disabled>click me</button>
</div>
chrome safari firefox
#parent none none none
button none none none

Considering that in the case where there is no element inside the button the bubbling stops, I think that we should probably follow the firefox behavior - why should adding an element inside the button change what events the parent elements of the dom see...?

The spec says that "A form control that is disabled must prevent any click events that are queued on the user interaction task source from being dispatched on the element." The chrome implementation of this just checks if the target element is a disabled form control before dispatching events on any element in general inside the event dispatcher, which more or less seems to follow the wording of the spec since it doesn't say anything about stopping the bubbling.

Should I change the chrome behavior to be like firefox? Should we change what the spec says? If so, what should it say?

Some posts I found about this behavior by searching:

  • https://github.com/Polymer/lit-element/issues/681
  • https://jakearchibald.com/2017/events-and-disabled-form-fields/

cc @jakearchibald @tkent-google

josepharhar avatar Sep 01 '20 22:09 josepharhar

It'd be good to solve this in tandem with https://github.com/whatwg/html/issues/2368. It looks like there @rniwa was in agreement with you that the Firefox behavior is better.

It seems like the proposal is to truncate the event path for all mouse events (and pointer events?) to remove disabled form controls, and any of their (shadow-including??) parents.

@annevk, can you think of any way to spec this without modifying event dispatch?

The current spec seems to be advocating for a third behavior, which is to don't dispatch the event when the target would be a disabled form control. This would result in clicking on children of a disabled button firing the event, but in that case the event should reach the disabled form control (since dispatch was not prevented). And no browsers seem to follow that.

The current spec is nice and simple and does not need to get into the innards of event dispatch; it just serves as input into the question of whether or not to dispatch. But, it's probably not good for web developers, and it's not what anyone implements, so oh well.

domenic avatar Sep 02 '20 20:09 domenic

Thanks, I didn't know about #2368, it sounds like it has the same root cause as this bug.

I found the SendMouseEventsDisabledFormControls flag in chrome, and I found that it has behavior different from what I've documented so far. I also looked and .click() and physical clicks separately:

<div id=parent>
  <button disabled>
    <span id=child>click me</span>
  </button>
</div>
physical click firefox safari chrome chrome Experimental Web Platform features
#parent none click click none
button none none none none
#child click click click none
.click() firefox safari chrome chrome Experimental Web Platform features
#parent click click click click
button click none none click
#child click click click click

@dtapuska it looks like you added the flag, what are your thoughts on this behavior? Do you think this flag will become enabled by default?

josepharhar avatar Sep 03 '20 01:09 josepharhar

See https://github.com/whatwg/dom/issues/687. It seems that a custom get the parent for elements might be a way of achieving this, but we should probably first sort out where we want everyone to end up.

annevk avatar Sep 07 '20 10:09 annevk

hey, can i contribute?

pruthvi3007 avatar Oct 03 '20 07:10 pruthvi3007

Sorry about the noise... I was just wondering if there was progress in this; it affects my code -- I have workaround code, and I would like to figure out when to task the deletion of the workaround. Thanks!

mercmobily avatar Mar 02 '22 22:03 mercmobily

I believe that the stable chrome and safari behavior is best here. I'll try to make sure that experimental chrome doesn't have the weird behavior where none of the event handlers are fired. I think that https://github.com/web-platform-tests/wpt/pull/32381 includes tests for this behavior. cc @saschanaz

josepharhar avatar Jun 24 '22 17:06 josepharhar

What is stable Chrome behavior? Propagating click from child elements of a disable button to the ancestor of the button, but not firing on the button itself. That is just weird. I'm rather strongly against such behavior. I don't think we have other cases where we drop magically an item from the event path. (But I could add that implementing Chrome's behavior in Gecko should be very easy, one would just change the parent target, if the default parent was disabled)

smaug---- avatar Jun 28 '22 11:06 smaug----

What is stable Chrome behavior? Propagating click from child elements of a disable button to the ancestor of the button, but not firing on the button itself. That is just weird. I'm rather strongly against such behavior. I don't think we have other cases where we drop magically an item from the event path. (But I could add that implementing Chrome's behavior in Gecko should be very easy, one would just change the parent target, if the default parent was disabled)

I couldn't possibly agree more.

mercmobily avatar Jun 28 '22 12:06 mercmobily

Agreed.

It seems reasonable to not-dispatch a click event if it's performed on a disabled button. "click" is pretty much an activation event, but a disabled button cannot be activated.

Dispatching "click" but skipping the button in the event path is weird.

jakearchibald avatar Jun 28 '22 13:06 jakearchibald

Should we dupe this to #2368?

I totally agree that it's weird, but Mozilla has received multiple webcompat reports (see https://bugzilla.mozilla.org/show_bug.cgi?id=1653882). Basically websites are trying to observe click events for disabled form elements by observing from their parent elements. One of the examples:

image

but if it's too late to do this, the weird behaviour should probably become part of the spec.

I think it is indeed too late. 😞

Edit: See also https://github.com/web-platform-tests/interop-2022/issues/27.

saschanaz avatar Jun 28 '22 13:06 saschanaz

Have other browsers gotten bug reports about the behavior they have?

smaug---- avatar Jun 28 '22 14:06 smaug----

Looks like in Chrome disabled button is in the composedPath, but event listeners aren't called on it. So one can't rely on event.composedPath() to return the targets on which listeners will be called.

smaug---- avatar Jun 28 '22 14:06 smaug----

Have other browsers gotten bug reports about the behavior they have?

I've seen several bugs about child elements of <fieldset disabled>, which I suppose is similar enough to child elements of <button disabled>:

  • https://bugs.chromium.org/p/chromium/issues/detail?id=1245511
  • https://bugs.chromium.org/p/chromium/issues/detail?id=588760
  • https://bugs.chromium.org/p/chromium/issues/detail?id=1169799

What is stable Chrome behavior? Propagating click from child elements of a disable button to the ancestor of the button, but not firing on the button itself. That is just weird. I'm rather strongly against such behavior. I don't think we have other cases where we drop magically an item from the event path. (But I could add that implementing Chrome's behavior in Gecko should be very easy, one would just change the parent target, if the default parent was disabled)

Based on this and the supportive comments, I suppose that the chrome/webkit behavior should not be what we end up with.

Should we do the firefox thing where we fire event listeners up to the disabled button, or should we not fire any listeners at all if there are any disabled field controls in the event path? I'll spell this out a little more clearly:

  • Option 1: Fire listeners on parent and child elements of disabled form controls. This is what chrome and webkit currently do.
  • Option 2: Fire listeners on child elements of disabled form controls, but not parents. This is what firefox currently does.
  • Option 3: Don't fire any listeners on child or parents of disabled form controls.

Here is where the bugs stand on these options:

  • This crbug desires at least the child elements to get events, so option 1 or 2 would be work.
  • This crbug desires parent elements to not get events, so option 2 or 3 would be work.
  • This crbug desires child elements to get events, so option 1 or 2 would work.
  • This firefox bug that @saschanaz shared desired parent elements to get events, so only option 1 would work. Maybe they could look at pointer events instead...?

Based on these, I think that option 2, the firefox behavior, is best. Hopefully implementing this doesn't result in too many regression bugs.

Looks like in Chrome disabled button is in the composedPath, but event listeners aren't called on it. So one can't rely on event.composedPath() to return the targets on which listeners will be called.

Yep, I see that firefox doesn't include the nodes which weren't fired on in composedPath. We will have to fix this in chrome and webkit as well. Perhaps composedPath should be tested in https://github.com/web-platform-tests/wpt/pull/32381

josepharhar avatar Jun 28 '22 17:06 josepharhar

Based on these, I think that option 2, the firefox behavior, is best. Hopefully implementing this doesn't result in too many regression bugs.

I fear that it will. All the "See also" bugs in https://bugzilla.mozilla.org/show_bug.cgi?id=1653882 are the webcompat issues we got:

https://github.com/webcompat/web-bugs/issues/65773 https://github.com/webcompat/web-bugs/issues/73699 https://github.com/webcompat/web-bugs/issues/72988 https://github.com/webcompat/web-bugs/issues/24628 https://github.com/webcompat/web-bugs/issues/55413 https://github.com/webcompat/web-bugs/issues/55857 https://github.com/webcompat/web-bugs/issues/68347 https://github.com/webcompat/web-bugs/issues/105932

(But well, if Blink can implement it then Gecko will not be alone at least 😁)

saschanaz avatar Jun 28 '22 18:06 saschanaz

Another testcase where Chrome's behavior is rather surprising: https://mozilla.pettay.fi/moztests/disabled_button.html One may use a disabled button to trigger default handling but a listener in the button itself isn't triggered.

smaug---- avatar Jul 06 '22 09:07 smaug----

https://github.com/webcompat/web-bugs/issues/65773

This bug made me realize that there is a significant difference in chrome with <button disabled> vs <input disabled>. Parent nodes of <input disabled> will always get click events, but parent nodes of <button disabled> will only get click events if a child node of the <button> is clicked. Perhaps this is because <input> has a shadow root with child nodes which actually receive the click events, but I'm not certain. Up until now I was only looking at <button>.

After skimming the rest of the bugs in this list, it looks like they are all about parent nodes of <input disabled>.

I fear that it will. All the "See also" bugs in https://bugzilla.mozilla.org/show_bug.cgi?id=1653882 are the webcompat issues we got

Yeah this seems like a significant issue. I imagine this would break far too many websites. My preference is now to spec "option 1", what chromium and webkit currently do, even though it's not the ideal behavior.

Another testcase where Chrome's behavior is rather surprising: https://mozilla.pettay.fi/moztests/disabled_button.html One may use a disabled button to trigger default handling but a listener in the button itself isn't triggered.

Assuming that the form is submitted by the button's default event handler, I agree that is odd. Perhaps we should add a test for this separate from https://github.com/web-platform-tests/wpt/pull/32381

josepharhar avatar Jul 06 '22 20:07 josepharhar

Another testcase where Chrome's behavior is rather surprising: https://mozilla.pettay.fi/moztests/disabled_button.html One may use a disabled button to trigger default handling but a listener in the button itself isn't triggered.

Assuming that the form is submitted by the button's default event handler, I agree that is odd. Perhaps we should add a test for this separate from web-platform-tests/wpt#32381

It's now covered by event-propagate-disabled-by-parent.html. (Thanks, :smaug)

saschanaz avatar Jul 06 '22 20:07 saschanaz

I'll add a UseCounter to chromium to see if we can fire events on parent nodes of <disabled button> in the case where the button element itself is the event target

josepharhar avatar Jul 07 '22 16:07 josepharhar

I debugged the funny behavior with buttons vs buttons with children vs inputs and I found that it comes down to this: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/mouse_event.cc;l=373-386;drc=127616d9e079e19774925a56fdd29acd568d7329

Any time a disabled node is clicked, we don't dispatch the event at all. The reason this doesn't work for buttons with children or input elements is that the actual node you're clicking isn't a disabled form control. Input elements have a child node in their shadow dom which gets clicked.

The logic to skip disabled form controls in the event path is here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/node.cc;l=2905-2918;drc=033ae102a356d906af6f62d3a31588f0b2fc4b18

Just for fun, I also traversed the blame to find where these two lines of code were introduced. They come from 2003 and 2005:

  • https://chromium.googlesource.com/chromium/src/+/420c380f646feb11c39b3c3ef2730561b28fdeff%5E%21/
  • https://chromium.googlesource.com/chromium/src/+/17f7fe709006d20caf0ecef81896fc56b6056427%5E%21/

The first change, which makes disabled form controls get skipped in the event path, was made to prevent disabled submit buttons from submitting forms. It's not clear why all mouse events were prevented, but it was at least acknowledged:

Don't send mouse events for disabled elements. But we do want those events to pass through the bubble and capture phases, just avoid triggering any listeners on this node itself.

The second change, which prevents mouse events from getting dispatched at all when it starts exactly on a disabled form control element, appears to be part of a refactor. It's also not clear what the motivation is for preventing all mouse events.

mouseover and mouseout have been patched not to fire when they occur on a disabled control. More generally, no mouse event will be delivered to a disabled element. The current check only examines the target node, and this is not good enough (but can be improved in a later patch). Some nodes will be children of disabled ancestors (e.g., options or children of a button), and this is not yet taken into account.

I guess they already knew that button elements with children would act different, and they didn't end up improving in a later patch 😅

josepharhar avatar Jul 08 '22 17:07 josepharhar

I debugged the funny behavior with buttons vs buttons with children vs inputs and I found that it comes down to this:

@josepharhar this is the best, most comprehensive analysis I've seen about this issue. It should be linked by every relevant tickets on Chrome's codebase -- it's absolutely golden.

mercmobily avatar Jul 08 '22 17:07 mercmobily

In the interest of making consistent behavior for disabled form controls which do or don't have child elements, I think these are our options:

  • Option A: Always propagate click events even if the node which is being clicked is a disabled form control.
    • This is like "Option 1" from my previous comment but would also cause <button disabled>hello world</button> to propagate click events to parent nodes.
    • I imagine doing this in chrome would break fewer websites because it's more similar to what we are currently doing in chrome.
  • Option B: Never propagate click events to parents above disabled form controls.
    • This is identical to "Option 2" from my previous comment.

I agree that Option B is the most ideal, but I'm still concerned about how much will break if we take away those events and I don't really know whats acceptable or not. I'll try adding a use counter for this scenario.

josepharhar avatar Jul 09 '22 02:07 josepharhar

I added a UseCounter:

  • https://chromium-review.googlesource.com/c/chromium/src/+/3754929
  • https://chromestatus.com/metrics/feature/timeline/popularity/4296

It should reach stable starting on August 30th, so hopefully by September 6th or 13th we will have some data. If by some miracle the usage is low, then I'm happy to go ahead and do "Option B" in my last comment in chrome.

josepharhar avatar Jul 20 '22 05:07 josepharhar

Hi,

Let's cross our fingers for the miracle :D

Merc.

On Wed, 20 Jul 2022 at 13:58, Joey Arhar @.***> wrote:

I added a UseCounter:

  • https://chromium-review.googlesource.com/c/chromium/src/+/3754929
  • https://chromestatus.com/metrics/feature/timeline/popularity/4296

It should reach stable starting on August 30th, so hopefully by September 6th or 13th we will have some data. If by some miracle the usage is low, then I'm happy to go ahead and do "Option B" in my last comment in chrome.

— Reply to this email directly, view it on GitHub https://github.com/whatwg/html/issues/5886#issuecomment-1189861050, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQHWXR53IME3BPFYLBPHTTVU6IO7ANCNFSM4QSI5X2Q . You are receiving this because you commented.Message ID: @.***>

mercmobily avatar Jul 20 '22 21:07 mercmobily

It looks like the UseCounter from my last comment is at 0.1%, which isn't a huge number but it does go above the threshold for knowing that we can safely change it

josepharhar avatar Sep 20 '22 00:09 josepharhar

That seems surprisingly low number for this. Nothing here is of course safe. Leaving Chrome's behavior as it is will likely cause issues later (since it is so inconsistent) and is causing issues in Gecko atm. And changing the behavior at all may break sites too.

smaug---- avatar Sep 20 '22 03:09 smaug----

I am going to try shipping the firefox behavior of propagating events up to disabled form controls but not to their parent elements. Let's see how far I can get.

josepharhar avatar Sep 29 '22 23:09 josepharhar

I filed an intent to ship in chromium: https://groups.google.com/a/chromium.org/g/blink-dev/c/9i0H0J0BzE4

josepharhar avatar Oct 15 '22 22:10 josepharhar

I shipped a Nightly-only flag to Firefox to limit the behavior to click/mouseup/mousedown, matching Chrome's behind-the-flag behavior. https://bugzilla.mozilla.org/show_bug.cgi?id=1799565

saschanaz avatar Nov 15 '22 15:11 saschanaz

Woohoo! Thanks @saschanaz !

I am going to slowly enable the new behavior in chrome starting with chrome 110

josepharhar avatar Nov 15 '22 15:11 josepharhar

Thank you for driving the change from Chromium side, @josepharhar !

BTW, do we have any WebKit people interested here? Maybe @annevk knows?

saschanaz avatar Nov 15 '22 16:11 saschanaz