Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Allow mutating <input disabled type=checkbox/radio>#5805

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
domenic merged 2 commits intowhatwg:masterfromsaschanaz:input-mutation
Aug 13, 2020

Conversation

saschanaz
Copy link
Member

@saschanazsaschanaz commentedAug 11, 2020
edited by pr-previewbot
Loading

Closes#5000

(SeeWHATWG Working Mode: Changes for more details.)


/input.html (diff )

Copy link
Member

@domenicdomenic left a 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.

@saschanaz
Copy link
MemberAuthor

I think my change incorrectly allows all mouse clicks to mutate checkboxes/radios. The intention here is:

  • Physical mouse click (trusted click event): no mutation
  • .click() (untrusted event): no mutation
  • click from<label> (untrusted event): no mutation
  • direct.dispatchEvent(new MouseEvent("click")) (untrusted event): mutation

I'll think more about this.

@domenic
Copy link
Member

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

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.

saschanaz reacted with thumbs up emoji

@domenic
Copy link
Member

I think my change incorrectly allows all mouse clicks to mutate checkboxes/radios. The intention here is:

So, is this intention met by the current PR? Let me try to summarize; please correct anything I get wrong.

  • Physical mouse click (trusted click event): no mutation

Handled byhttps://html.spec.whatwg.org/#enabling-and-disabling-form-controls:-the-disabled-attribute

Not yet tested inweb-platform-tests/wpt#24975.

  • .click() (untrusted event): no mutation

Handled byhttps://html.spec.whatwg.org/#dom-click

Tested inweb-platform-tests/wpt#24975.

  • click from<label> (untrusted event): no mutation

The spec is not precise about label activation behavior one way or another.https://html.spec.whatwg.org/#the-label-element says

The label element's exact default presentation and behavior, in particular what its activation behavior might be, if anything, should match the platform's label behavior.

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 fromweb-platform-tests/wpt#24975.

direct.dispatchEvent(new MouseEvent("click")) (untrusted event): mutation

This is what this PR fixes, by adding an exception in the activation behavior for checkboxes and radios.

This is tested inweb-platform-tests/wpt#24975.

saschanaz reacted with thumbs up emoji

@saschanaz
Copy link
MemberAuthor

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.

@saschanaz
Copy link
MemberAuthor

saschanaz commentedAug 12, 2020
edited
Loading

Does the current spec define what should be done here? (Edit: fixed the example to disable the element)

varinput=document.createElement("input");input.type="checkbox";input.disabled=true;input.onclick=console.log// Firefox: a log, Chrome: no loginput.dispatchEvent(newMouseEvent("click"));

There are proses to prevent dispatching events as you mentioned, but I'm not seeing things to prevent onclick for already dispatched events.

@domenic
Copy link
Member

The spec currently says that should be logged. I believe not-logging would require changes to event dispatch, which I'd like to avoid.

@domenic
Copy link
Member

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.

@saschanaz
Copy link
MemberAuthor

saschanaz commentedAug 12, 2020
edited
Loading

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.

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:

varinput=document.createElement("input");input.type="checkbox";input.disabled=true;input.dispatchEvent(newMouseEvent("click"));input.checked// true

I'll remove the onclick tests for now and file a new issue. Actually I'll just file browser bugs since we hope we don't need to modify the spec here.

Edit: Filedhttps://bugs.chromium.org/p/chromium/issues/detail?id=1115661 andhttps://bugs.webkit.org/show_bug.cgi?id=215461

@saschanaz
Copy link
MemberAuthor

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

A normal instance:

image

python wpt run --binary "C:/Program Files (x86)/Google/Chrome/Application/chrome.exe" chrome dom/events/Event-dispatch-click.html:

image

I'd like to merge this and file a relevant Chromium bug, does that sound good?

@domenic
Copy link
Member

Yes, that's great; thanks! Please leave a comment here when you do file the bug and I can help get it triaged.

domenic pushed a commit to web-platform-tests/wpt that referenced this pull requestAug 13, 2020
Seewhatwg/html#5805; this tests the scenario there plus others.
@domenicdomenic merged commit3c52fe1 intowhatwg:masterAug 13, 2020
@saschanazsaschanaz deleted the input-mutation branchAugust 13, 2020 20:36
@saschanaz
Copy link
MemberAuthor

saschanaz commentedAug 13, 2020
edited
Loading

BTW, not sure how to add tests for physical mouse clicks as#2368 (comment) is a blocker. Sinceonclick on input won't work I thought I would add onclick for the parent element, but it does not bubble at all on Gecko 🤷‍♀️

What does the spec say about it today?

@domenic
Copy link
Member

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 likeawait test_driver.click(element); assert_equals(element.checked, false).

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.

saschanaz reacted with thumbs up emoji

@saschanaz
Copy link
MemberAuthor

The spec says that physical mouse clicks on disabled elements should not trigger any events.

Does it mean the parent should not get any bubbled events either? Currently it does get one on Chrome, should I file a bug?

@domenic
Copy link
Member

The exact wording is

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.

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.

saschanaz reacted with heart emoji

@saschanaz
Copy link
MemberAuthor

<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>
Copy link
MemberAuthor

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

Copy link
MemberAuthor

@saschanazsaschanazAug 13, 2020
edited
Loading

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)

Copy link
Member

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.

Copy link
Member

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?

Copy link
MemberAuthor

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.

Copy link
Member

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.

domenic pushed a commit that referenced this pull requestAug 17, 2020
This is a followup to#5805, removing some now-redundant checks whilefixing the condition introduced there to be a bit less far-reaching.(In particular, we want disabled checkboxes and radio buttons to stillbe immutable, even if that doesn't impact their activation behavior.)
@mfreed7
Copy link
Contributor

Thanks for the spec and testing work here. I'll take a look at the Chromium implementation.

saschanaz reacted with heart emoji

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull requestAug 25, 2020
…estonlyAutomatic update from web-platform-testsAdd tests for disabled input clicksSeewhatwg/html#5805; this tests the scenario there plus others.--wpt-commits: ed728bc7b01fed9c0cf7f4794d592fb2f1e151fawpt-pr: 24975
mfreed7 pushed a commit to mfreed7/html that referenced this pull requestSep 11, 2020
This is a followup towhatwg#5805, removing some now-redundant checks whilefixing the condition introduced there to be a bit less far-reaching.(In particular, we want disabled checkboxes and radio buttons to stillbe immutable, even if that doesn't impact their activation behavior.)
This was referencedMar 15, 2021
xmo-odoo added a commit to odoo-dev/odoo that referenced this pull requestApr 21, 2021
Causes the failure of [0] on FF as it expects that clicking a disabledbutton does nothing, which is what happens for Chrome, but the eventis dispatched for Firefox.Asking the internet it looks like Firefox is in the right here:click() ultimately calls dispatchEvent (directly), dispatchEventshould go through even on disabled event. This was specifically fixedin Firefox[1], and there is an issue opened against Chrome[2] (cfalso: spec discussion[3]).There's an other issue which mentions inconsistencies between theactual 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 theright 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
robodoo pushed a commit to odoo/odoo that referenced this pull requestApr 23, 2021
Causes the failure of [0] on FF as it expects that clicking a disabledbutton does nothing, which is what happens for Chrome, but the eventis dispatched for Firefox.Asking the internet it looks like Firefox is in the right here:click() ultimately calls dispatchEvent (directly), dispatchEventshould go through even on disabled event. This was specifically fixedin Firefox[1], and there is an issue opened against Chrome[2] (cfalso: spec discussion[3]).There's an other issue which mentions inconsistencies between theactual 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 theright 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
zel-odoo pushed a commit to odoo-dev/odoo that referenced this pull requestMay 12, 2021
Causes the failure of [0] on FF as it expects that clicking a disabledbutton does nothing, which is what happens for Chrome, but the eventis dispatched for Firefox.Asking the internet it looks like Firefox is in the right here:click() ultimately calls dispatchEvent (directly), dispatchEventshould go through even on disabled event. This was specifically fixedin Firefox[1], and there is an issue opened against Chrome[2] (cfalso: spec discussion[3]).There's an other issue which mentions inconsistencies between theactual 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 theright 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
bhearsum pushed a commit to mozilla-releng/staging-firefox that referenced this pull requestMay 1, 2025
…estonlyAutomatic update from web-platform-testsAdd tests for disabled input clicksSeewhatwg/html#5805; this tests the scenario there plus others.--wpt-commits: ed728bc7b01fed9c0cf7f4794d592fb2f1e151fawpt-pr: 24975
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@domenicdomenicdomenic approved these changes

Assignees
No one assigned
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

input[type=checkbox] is mutable even when disabled on every browser
4 participants
@saschanaz@domenic@mfreed7@cdumez

[8]ページ先頭

©2009-2025 Movatter.jp