facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.57k stars 46.79k forks source link

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

Open brillout opened 8 years ago

brillout commented 8 years ago

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 commented 8 years ago

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 commented 7 years ago

I'm fixing this issue

YutamaKotaro commented 7 years ago

1.I fixed this issue, but Is this way is correct? https://github.com/YutamaKotaro/react/commit/094124c8efa89e538578bb50c97ec4b93f5eab0d 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 commented 7 years ago

Seems to be fixed in latest master.

aweary commented 7 years ago

@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 commented 6 years ago

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

mqklin commented 5 years ago

Still reproducible, makes fieldset useless

andomain commented 5 years ago

This is still present in 16.8.0

FezVrasta commented 5 years ago

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 commented 4 years ago

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

n8io commented 4 years ago

@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 commented 3 years ago

Hey!

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

I noticed that there was a PR for this (https://github.com/facebook/react/issues/17909), but the PR got closed because it had been reported as a bug for Mozilla https://github.com/facebook/react/issues/17909#issuecomment-579449775

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

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

rickhanlonii commented 3 years ago

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 https://github.com/facebook/react/pull/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 commented 3 years ago

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