-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Event bubbling on disabled form elements #5886
Comments
It'd be good to solve this in tandem with #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. |
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>
@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? |
See whatwg/dom#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. |
This comment has been minimized.
This comment has been minimized.
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! |
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. |
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. |
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. |
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:
I think it is indeed too late. 😞 Edit: See also web-platform-tests/interop#27. |
Have other browsers gotten bug reports about the behavior they have? |
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. |
I've seen several bugs about child elements of
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:
Here is where the bugs stand on these options:
Based on these, I think that option 2, the firefox behavior, is best. Hopefully implementing this doesn't result in too many regression bugs.
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 web-platform-tests/wpt#32381 |
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: webcompat/web-bugs#65773 (But well, if Blink can implement it then Gecko will not be alone at least 😁) |
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. |
This bug made me realize that there is a significant difference in chrome with After skimming the rest of the bugs in this list, it looks like they are all about parent nodes of
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.
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 |
I'll add a UseCounter to chromium to see if we can fire events on parent nodes of |
I debugged the funny behavior with buttons vs buttons with children vs inputs and I found that it comes down to this: 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:
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:
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.
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 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. |
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:
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. |
Thanks for the heads-up. Will fix on WebKit side: https://bugs.webkit.org/show_bug.cgi?id=258078 |
Also reverting the CSS selector WPT test changes in web-platform-tests/wpt#40547 unless there are objections. |
…disabled CSS selectors is incorrect https://bugs.webkit.org/show_bug.cgi?id=258078 Reviewed by Tim Nguyen. 264098@main updated HTMLFieldSetElement::isDisabledFormControl() to return false since a fieldset element can never be disabled per the specification. This had the side effect of affecting our behavior for :enabled / :disabled CSS selectors, which relied on this function. However, the behavior of these CSS selectors shouldn't have changed since they rely on the "actually disabled" state, not the "disabled" state [1]. There were two issues here, some of our CSS selector logic was relying on isDisabledFormControl() instead of isActuallyDisabled(). Also, 264098@main shouldn't have changed the value returned by isActuallyDisabled() for HTMLFieldsetElements. This was pointed out at: - whatwg/html#5886 (comment) [1] https://html.spec.whatwg.org/multipage/semantics-other.html#selector-enabled [2] https://html.spec.whatwg.org/multipage/semantics-other.html#selector-disabled [3] https://html.spec.whatwg.org/multipage/semantics-other.html#concept-element-disabled [4] https://html.spec.whatwg.org/multipage/form-elements.html#concept-fieldset-disabled * LayoutTests/imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/disabled.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/enabled.html: * Source/WebCore/css/SelectorCheckerTestFunctions.h: (WebCore::matchesEnabledPseudoClass): * Source/WebCore/html/HTMLElement.h: * Source/WebCore/html/HTMLFieldSetElement.cpp: (WebCore::HTMLFieldSetElement::isActuallyDisabled const): * Source/WebCore/html/HTMLFieldSetElement.h: Canonical link: https://commits.webkit.org/265166@main
I will make fieldset match :enabled and :disabled again in chromium to match the WPT, thanks for following up with the WPT changes |
In http://crrev.com/1122658 I made fieldset not disableable to get around some event dispatching issues, and also made it stop matching :disabled and :enabled. Due to feedback in whatwg/html#5886 (comment) I am making it match :disabled and :enabled again in this patch. Bug: 588760 Change-Id: I3793c02353fe0ffbd45e94d90b79e5f7b61b97dc Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4627174 Reviewed-by: Di Zhang <[email protected]> Commit-Queue: Di Zhang <[email protected]> Auto-Submit: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1160137}
…disabled CSS selectors is incorrect https://bugs.webkit.org/show_bug.cgi?id=258078 Reviewed by Tim Nguyen. 264098@main updated HTMLFieldSetElement::isDisabledFormControl() to return false since a fieldset element can never be disabled per the specification. This had the side effect of affecting our behavior for :enabled / :disabled CSS selectors, which relied on this function. However, the behavior of these CSS selectors shouldn't have changed since they rely on the "actually disabled" state, not the "disabled" state [1]. There were two issues here, some of our CSS selector logic was relying on isDisabledFormControl() instead of isActuallyDisabled(). Also, 264098@main shouldn't have changed the value returned by isActuallyDisabled() for HTMLFieldsetElements. This was pointed out at: - whatwg/html#5886 (comment) [1] https://html.spec.whatwg.org/multipage/semantics-other.html#selector-enabled [2] https://html.spec.whatwg.org/multipage/semantics-other.html#selector-disabled [3] https://html.spec.whatwg.org/multipage/semantics-other.html#concept-element-disabled [4] https://html.spec.whatwg.org/multipage/form-elements.html#concept-fieldset-disabled * LayoutTests/imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/disabled.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/enabled.html: * Source/WebCore/css/SelectorCheckerTestFunctions.h: (WebCore::matchesEnabledPseudoClass): * Source/WebCore/html/HTMLElement.h: * Source/WebCore/html/HTMLFieldSetElement.cpp: (WebCore::HTMLFieldSetElement::isActuallyDisabled const): * Source/WebCore/html/HTMLFieldSetElement.h: Canonical link: https://commits.webkit.org/265166@main
The current consensus is to block such behavior; any click inside a disable button should not propagate to the button. You can still listen the event by adding listener directly to child elements. You can check the behavior by running Chrome with
I think that is a separate issue, and I'm not sure what's right there. @emilio? |
Mozilla is considering to apply our changes to beta (currently Nightly only), has there some notable negative webcompat feedback so far? @cdumez @josepharhar |
Not aware of any negative feedback on WebKit side. This has been enabled in Safari Technology Preview for a while. |
If my calculations are correct, this should have been enabled on chrome 116 which was released to stable a week ago today. I haven't seen any issues yet. |
That was blazingly quick, thank you all! |
An issue has cropped up where someone wants click events to fire on their disabled textarea, and pointer-events:none isn't good enough for them because that makes selection stop working in the textarea: https://bugs.chromium.org/p/chromium/issues/detail?id=1474913 I recommended that they listen to pointerup and fire click events in response to pointerup on the parent of the disabled textarea |
we were relying on click events bubbling from a disabled input in our web application and came across https://www.reddit.com/r/HTML/comments/163vrs2/event_bubbling_on_disabled_input_broken_in_last/ and https://support.google.com/chrome/thread/231546184/click-event-not-working-on-disabled-input?hl=en when we got bug reports about that no longer working. i also came across this suggestion to apply the sentiment expressed in the reddit thread is an expectation that this is a regression that they hope will be patched in the future. their workaround is creating an empty div covering the input that captures and propagates mouse events. EDIT GitHub had collapsed comments about this from earlier. i just found #5886 (comment) where @josepharhar recommends listening to |
Another regression bug was filed: https://bugs.chromium.org/p/chromium/issues/detail?id=1477379 |
This chrome updated did scare me a lot. <div onClick={onClick}>
<textarea disabled/>
</div> So after workaround <div onPointerUp={onClick}>
<textarea disabled/>
</div> |
I'll try shipping this on Firefox beta. If we get enough frustration reports then maybe we could discuss whether we can disable click only for button-like elements (including checkboxes and radio buttons) as blocking click makes best sense there and less for others. |
…_disable_only_descendants on beta r=dom-core,edgar Addressing whatwg/html#5886 (comment). Differential Revision: https://phabricator.services.mozilla.com/D188601
…_disable_only_descendants on beta r=dom-core,edgar Addressing whatwg/html#5886 (comment). Differential Revision: https://phabricator.services.mozilla.com/D188601
…_disable_only_descendants on beta r=dom-core,edgar Addressing whatwg/html#5886 (comment). Differential Revision: https://phabricator.services.mozilla.com/D188601 UltraBlame original commit: 478e09e8806ba30c005861fc55a970ae5d0c91bc
…_disable_only_descendants on beta r=dom-core,edgar Addressing whatwg/html#5886 (comment). Differential Revision: https://phabricator.services.mozilla.com/D188601 UltraBlame original commit: 478e09e8806ba30c005861fc55a970ae5d0c91bc
…_disable_only_descendants on beta r=dom-core,edgar Addressing whatwg/html#5886 (comment). Differential Revision: https://phabricator.services.mozilla.com/D188601 UltraBlame original commit: 478e09e8806ba30c005861fc55a970ae5d0c91bc
@KajSzy However, the event occurs, but the state does not change when disabled due to React event delegation. I use the code below to circumvent this. I hope my answer helps others who have this problem. const [isOpen, setOpen] = useState(false);
const ref = useRef<HTMLDivElement>(null);
return (
<div
ref={ref}
onClick={() => {
// here click event code..
setOpen((prev) => !prev);
}}
>
<button
disabled
onPointerDown={() => {
ref.current?.click();
}}
>
버튼
</button>
</div>
) |
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:
In the case that there is no element inside the button, there is no bubbling that occurs in any of the browsers:
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:
cc @jakearchibald @tkent-google
The text was updated successfully, but these errors were encountered: