Skip to content
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

[bug] click not disabled on <fieldset disabled><button onClick={() => alert('clicked')}><span>click me</span></button></fieldset> #7711

Open
brillout opened this issue Sep 13, 2016 · 14 comments

Comments

@brillout
Copy link
Contributor

bug

In the following

const Component = () =>
        <fieldset disabled>
            <button
              onClick={() => alert('clicked by React')}
            >click me here and <span style={{color: 'red'}}>here</span></button>
        </fieldset>;

clicking on click me here and will not trigger alert('clicked by React') whereas clicking on the red here will trigger alert('clicked by React').

Demo: https://jsfiddle.net/ropbvL3y/

Thanks for React, it's an incredibly well designed tool.

@aweary
Copy link
Contributor

aweary commented Sep 13, 2016

Thanks for the report @brillout, and for the great test case. I was able to reproduce using a build of the current master branch http://jsfiddle.net/jxx65yhy/

@YutamaKotaro
Copy link

I'm fixing this issue

@YutamaKotaro
Copy link

1.I fixed this issue, but Is this way is correct?
YutamaKotaro@094124c
if parent tag is form or fieldset having disabled, button will not fire in the case of clicking children span or div.
but even if parent div has disabled, children button will fire.

if click span, onClick will not fire

<fieldset disabled>
    <button onClick={() => alert('hello');}>
        click <span> here </span>
     </button>
</fieldset>

but in this case, if click span, onClick will fire

<div disabled>
    <button onClick={() => alert('hello');}>
        click <span> here </span>
     </button>
</div>

and more, if click span, onClick will fire

<fieldset disabled>
    <button>
        click <span onClick={() => alert('hello');}> here </span>
     </button>
</fieldset>

2.This issue didnt occuered in firefox.But Chrome, Safari has this issue.
This issue should leave it as specification of browser ?

3.I have question in the test.
This test give ReactComponent Object to traverseTwoPhase method. But in browser, traverseTwoPhase method receive Simple Object ( have tag, type ...), not receive ReactComponent Object.
How to write test case, should i do?

    it('should traverse two phase at shallowest node', () => {
      var parent = renderParentIntoDocument();
      var target = getInst(parent.refs.P);
      var expectedCalls = [['P', 'captured', ARG], ['P', 'bubbled', ARG]];
      ReactTreeTraversal.traverseTwoPhase(target, callback, ARG);
      expect(mockFn.mock.calls).toEqual(expectedCalls);
    });

@acdlite
Copy link
Collaborator

acdlite commented Sep 15, 2017

Seems to be fixed in latest master.

@acdlite acdlite closed this as completed Sep 15, 2017
@aweary
Copy link
Contributor

aweary commented Sep 15, 2017

@acdlite have there been changes merged to master since 16.0.0.rc-3 that would have resolved this? I'm seeing that the issue still occurs with the latest RC.

@raybooysen
Copy link

raybooysen commented Jan 3, 2018

I'm seeing this in 16.2.0, can we reopen?

@mqklin
Copy link

mqklin commented Dec 21, 2018

Still reproducible, makes fieldset useless

@andomain
Copy link

This is still present in 16.8.0

@FezVrasta
Copy link

FezVrasta commented Oct 21, 2019

I suppose React should use something like element.matches(':disabled') before calling onClick so that it's really sure the button is disabled.

But this would probably be an expensive operation to perform on every button each time it's clicked.

Is the core team working on a solution or should the community send a PR?

For anyone looking for a quick workaround, you can change your onClick function to be:

<button
  onClick={evt => !evt.currentTarget.matches(':disabled') ? handleOnClick(evt) : undefined}
/>

@n8io
Copy link

n8io commented Jan 20, 2020

Here's a codesandbox illustrating the bug. You can easily toggle between the React versions to troubleshoot. I have also confirmed that @FezVrasta's suggestion ☝️ does patch the bug for anyone looking for an interim solution.

image

  • Still present in 16.12.0
  • Chrome only

@n8io
Copy link

n8io commented Jan 21, 2020

@brillout can you update the title of this issue so that it might get more exposure? I am thinking it has been ignored due to its cryptic title.

@RobinMalfait
Copy link

Hey!

We are seeing this issue being reported in Headless UI, (tailwindlabs/headlessui#194).
I made a reproduction in codesandbox that showcases the issue for React 17.

I noticed that there was a PR for this (#17909), but the PR got closed because it had been reported as a bug for Mozilla #17909 (comment)

However, I am seeing this issue in the following browsers (all tested on macOS Big Sur):

  • Safari - Version 14.0.1 (16610.2.11.51.8)
  • Brave - Version 1.18.75 Chromium: 87.0.4280.101 (Official Build) (x86_64)
  • Google Chrome Canary - Version 89.0.4389.0 (Official Build) canary (x86_64)

What would be the next steps? Is there anything you want me to do (re-open that PR for example)?

@rickhanlonii
Copy link
Member

Thanks for the repos @RobinMalfait.

Looks like the browser is firing the click event for the span in both the React and Vanilla examples. The difference is, React bubbles that click up to the button, which it shouldn't do according to the spec:

A form control is disabled if... The element is a descendant of a fieldset element whose disabled attribute is specified, and is not a descendant of that fieldset element's first legend element child, if any... 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.

I've added a test for this in #20612 which should be failing. Unfortunately, a bug in JSDOM prevents click events from firing on any child of a disabled fieldset (see here), so the test passes in our test suite.

I believe the handling should be added somewhere around here but @gaearon @trueadm or @sophiebits may have more context.

@kwburnett
Copy link

I'm not sure how performant this is, but one workaround is to add a check somewhere of whether the target element is disabled. For example, e.target.closest(':disabled') will return the target itself for the children that still bubble up the event of disabled fieldset elements.

I forked @RobinMalfait 's repo to share an example of the workaround.
https://codesandbox.io/s/peaceful-heisenberg-u6n4g?file=/src/App.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests