-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Allow mutating <input disabled type=checkbox/radio> #5805
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great with nits, thank you!
Would you also be willing to work on web platform tests? That should also help clarify the implementer-interest situation; if 2+ browsers pass them then we can merge straightaway.
I think my change incorrectly allows all mouse clicks to mutate checkboxes/radios. The intention here is:
I'll think more about this. |
I think prohibiting physical mouse clicks is done at a different layer: https://html.spec.whatwg.org/#enabling-and-disabling-form-controls:-the-disabled-attribute says
|
So, is this intention met by the current PR? Let me try to summarize; please correct anything I get wrong.
Handled by https://html.spec.whatwg.org/#enabling-and-disabling-form-controls:-the-disabled-attribute Not yet tested in web-platform-tests/wpt#24975.
Handled by https://html.spec.whatwg.org/#dom-click Tested in web-platform-tests/wpt#24975.
The spec is not precise about label activation behavior one way or another. https://html.spec.whatwg.org/#the-label-element says
and discusses examples of firing clicks on the element. So hmm. My suggestion is that we don't touch this area for now. We could consider improving the requirements there in general, but as-is, we can't make strong assertions. As such, I think we should remove the label tests from web-platform-tests/wpt#24975.
This is what this PR fixes, by adding an exception in the activation behavior for checkboxes and radios. This is tested in web-platform-tests/wpt#24975. |
Thanks for the details! Yes, the current PR should be sufficient then. I agree that the label thing should be solved separately. I'll file another bug. |
Does the current spec define what should be done here? (Edit: fixed the example to disable the element) var input = document.createElement("input");
input.type = "checkbox";
input.disabled = true;
input.onclick = console.log // Firefox: a log, Chrome: no log
input.dispatchEvent(new MouseEvent("click")); There are proses to prevent dispatching events as you mentioned, but I'm not seeing things to prevent onclick for already dispatched events. |
The spec currently says that should be logged. I believe not-logging would require changes to event dispatch, which I'd like to avoid. |
Based on the test results (Firefox, Chrome, Safari it seems that this patch's semantics match Firefox and Safari, but not Chrome. Could you file a Chrome bug and update the OP? Then this should be ready to merge, as it has 2 implementer support. /cc @mfreed7 since it seems like not all browsers agree here, despite his comment in #5000 (comment). Your web platform tests also add tests for the bug discussed in #5805 (comment), which show Firefox aligned to the spec while WebKit and Chrome are not. If you were up for filing separate bugs for those, that would be ideal, but it isn't directly tied to this PR. Alternately we could hold off, and have a separate spec discussion about whether we want to modify the event dispatching algorithm in that way... I hope we don't modify it though, so icky. |
That's very weird, as it passes on my local environment, on both Ubuntu and Windows. You can copy-paste this to try it yourself: var input = document.createElement("input");
input.type = "checkbox";
input.disabled = true;
input.dispatchEvent(new MouseEvent("click"));
input.checked // true
Edit: Filed https://bugs.chromium.org/p/chromium/issues/detail?id=1115661 and https://bugs.webkit.org/show_bug.cgi?id=215461 |
@domenic I found that the Chrome failures are something to do with WPT or WebDriver or whatever, as the results are different between a normal Chrome instance and the instance started by A normal instance:
I'd like to merge this and file a relevant Chromium bug, does that sound good? |
Yes, that's great; thanks! Please leave a comment here when you do file the bug and I can help get it triaged. |
See whatwg/html#5805; this tests the scenario there plus others.
BTW, not sure how to add tests for physical mouse clicks as #2368 (comment) is a blocker. Since What does the spec say about it today? |
Hmm, but later in that thread in #2368 (comment) it looked like Gecko changed their behavior. The spec says that physical mouse clicks on disabled elements should not trigger any events. So I was thinking a test that does something like However now that I write that out, it seems kind of obvious that it will pass. It's such a basic feature of disabling form controls that users clicking on them won't change them; writing a test isn't very necessary. I guess I lost sight of that. |
Does it mean the parent should not get any bubbled events either? Currently it does get one on Chrome, should I file a bug? |
The exact wording is
which indicates to me that it is OK to dispatch to the parent. It's not very precise though. But I think full interop here is waiting on a "hit testing" specification, which determines how to translate raw OS mouse clicks into DOM events and takes into account all sorts of things like disabledness, CSS boxes (including rounded corners, etc.), CSS transforms, and the like. Not an easy job. |
Filed https://bugs.chromium.org/p/chromium/issues/detail?id=1116161 for the failures. |
<li><p>If this element is not <i data-x="concept-fe-mutable">mutable</i>, then return.</p></li> | ||
<li><p>If this element is not <i data-x="concept-fe-mutable">mutable</i> and is not in <span | ||
data-x="attr-input-type-checkbox">Checkbox</span> nor <span data-x="attr-input-type-radio"> | ||
Radio</span>, then return.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a late second look, and this specific change now looks redundant as they are now always mutable per the first change anyway. What do you think? @domenic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe we can keep this and revert the first change because disabled checkboxes are not user-mutable. (And AFAICT "mutable" is defined as being mutable via user interface)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point, yes, we should keep this and revert the first change. Thank you for double-checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, although mutability also impacts legacy-pre-activation behavior and legacy-canceled-activation behavior. So maybe we need to add checks there too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those behaviors only apply for radio/checkboxes so maybe the mutability check can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, great point, yes, I think they can be removed.
This is a followup to #5805, removing some now-redundant checks while fixing the condition introduced there to be a bit less far-reaching. (In particular, we want disabled checkboxes and radio buttons to still be immutable, even if that doesn't impact their activation behavior.)
Thanks for the spec and testing work here. I'll take a look at the Chromium implementation. |
…estonly Automatic update from web-platform-tests Add tests for disabled input clicks See whatwg/html#5805; this tests the scenario there plus others. -- wpt-commits: ed728bc7b01fed9c0cf7f4794d592fb2f1e151fa wpt-pr: 24975
This is a followup to whatwg#5805, removing some now-redundant checks while fixing the condition introduced there to be a bit less far-reaching. (In particular, we want disabled checkboxes and radio buttons to still be immutable, even if that doesn't impact their activation behavior.)
Causes the failure of [0] on FF as it expects that clicking a disabled button does nothing, which is what happens for Chrome, but the event is dispatched for Firefox. Asking the internet it looks like Firefox is in the right here: click() ultimately calls dispatchEvent (directly), dispatchEvent should go through even on disabled event. This was specifically fixed in Firefox[1], and there is an issue opened against Chrome[2] (cf also: spec discussion[3]). There's an other issue which mentions inconsistencies between the actual browser and WPT[4], but for us Chrome always 100% does the "wrong" thing. Anyway add a disabled flag in click, though I don't know that it's the right fix, and it may need to be added to other events as well? [0] https://github.com/odoo/odoo/blob/c89cdcf11c66e80c33cd77edceaee7eb59a704b3/addons/web/static/tests/fields/relational_fields/field_many2one_tests.js#L2044 [1] https://bugzilla.mozilla.org/show_bug.cgi?id=329509 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=1115661 [3] whatwg/html#5805 (comment) [4] https://bugs.chromium.org/p/chromium/issues/detail?id=1116161
Causes the failure of [0] on FF as it expects that clicking a disabled button does nothing, which is what happens for Chrome, but the event is dispatched for Firefox. Asking the internet it looks like Firefox is in the right here: click() ultimately calls dispatchEvent (directly), dispatchEvent should go through even on disabled event. This was specifically fixed in Firefox[1], and there is an issue opened against Chrome[2] (cf also: spec discussion[3]). There's an other issue which mentions inconsistencies between the actual browser and WPT[4], but for us Chrome always 100% does the "wrong" thing. Anyway add a disabled flag in click, though I don't know that it's the right fix, and it may need to be added to other events as well? [0] https://github.com/odoo/odoo/blob/c89cdcf11c66e80c33cd77edceaee7eb59a704b3/addons/web/static/tests/fields/relational_fields/field_many2one_tests.js#L2044 [1] https://bugzilla.mozilla.org/show_bug.cgi?id=329509 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=1115661 [3] whatwg/html#5805 (comment) [4] https://bugs.chromium.org/p/chromium/issues/detail?id=1116161
Causes the failure of [0] on FF as it expects that clicking a disabled button does nothing, which is what happens for Chrome, but the event is dispatched for Firefox. Asking the internet it looks like Firefox is in the right here: click() ultimately calls dispatchEvent (directly), dispatchEvent should go through even on disabled event. This was specifically fixed in Firefox[1], and there is an issue opened against Chrome[2] (cf also: spec discussion[3]). There's an other issue which mentions inconsistencies between the actual browser and WPT[4], but for us Chrome always 100% does the "wrong" thing. Anyway add a disabled flag in click, though I don't know that it's the right fix, and it may need to be added to other events as well? [0] https://github.com/odoo/odoo/blob/c89cdcf11c66e80c33cd77edceaee7eb59a704b3/addons/web/static/tests/fields/relational_fields/field_many2one_tests.js#L2044 [1] https://bugzilla.mozilla.org/show_bug.cgi?id=329509 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=1115661 [3] whatwg/html#5805 (comment) [4] https://bugs.chromium.org/p/chromium/issues/detail?id=1116161
Closes #5000
(See WHATWG Working Mode: Changes for more details.)
/input.html ( diff )