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

onClick firing for disabled buttons in v16 #11588

Closed
ClemDelp opened this issue Nov 17, 2017 · 16 comments
Closed

onClick firing for disabled buttons in v16 #11588

ClemDelp opened this issue Nov 17, 2017 · 16 comments

Comments

@ClemDelp
Copy link

ClemDelp commented Nov 17, 2017

Hello the React Team! I found a little event firing bug

When I click on an Icon element inside a disabled button, the onClick on the div element is triggered

<div onClick={() => ...}>
  <button disabled>
     <svg class="icon mr1">...</svg>
     Delete
  </button>
</div>
@rstims
Copy link

rstims commented Nov 18, 2017

I may be missing something, but i feel like this is expected behavior and can easily be fixed by tweaking your code.

@gaearon
Copy link
Collaborator

gaearon commented Nov 18, 2017

Does this happen with regular DOM listeners?

@rstims
Copy link

rstims commented Nov 18, 2017

@gaearon looks like it

https://jsfiddle.net/b4wycvwf/1/

and then if the onclick is attached to the disabled button the event isn't triggered.

https://jsfiddle.net/b4wycvwf/2/

@gaearon
Copy link
Collaborator

gaearon commented Nov 19, 2017

Sounds like expected behavior then.

@gaearon gaearon closed this as completed Nov 19, 2017
@rstims
Copy link

rstims commented Nov 20, 2017

FWIW, checked on Firefox, Chrome, and Safari. The onclick only triggers on Chrome and Safari. Didn't check IE.

@ClemDelp
Copy link
Author

ClemDelp commented Nov 20, 2017

Thanks guys, Yep i though it was a React problem like on #8308

But it's a normal html DOM behavior... I share a simple solution to hack this behavior on my project:

const buttonElement = (
    <button
      disabled={disabled}
      download={download}
      type={type}
      className={classes}
      onClick={onClick}
      onMouseOver={onMouseOver}
      tabIndex={tabIndex}
    >
      {children}
    </button>
  )

  if (disabled) {
    return (
      <span
        onClick={e => {
          e.preventDefault()
          e.nativeEvent.stopImmediatePropagation() // Prevent click bubbling
          e.stopPropagation()
        }}
        title={title}
      >
        {buttonElement}
      </span>
    )
  }
  return buttonElement

@thysultan
Copy link

I don't think it is expected behavior https://jsfiddle.net/b4wycvwf/1/.

The fiddle shown(using DOM events) does not trigger when you click on the icon within the disabled button, this being different from @ClemDelp 's observed behavior.

@gaearon gaearon reopened this Nov 20, 2017
@rstims
Copy link

rstims commented Nov 20, 2017

@thysultan are you by any chance checking on Firefox? Because if you check that same fiddle on Chrome or Safari you will see the event being triggered. So although it may not be correct, it's not a React issue.

@thysultan
Copy link

thysultan commented Nov 20, 2017

@rstims I observed the fiddle on Chrome 62.0.3202.94, OSX 10.12. Only safari emits the event on my device.

Edit: I'm unsure if there's a spec that defines what is the correct behavior for this.

@jquense
Copy link
Contributor

jquense commented Nov 20, 2017

Browsers are notoriously inconsistent on this. Generally disabled elements emit no events, but they "probably" should only prevent the event on the direct target, I think the spec is a bit unclear on it, or maybe its just legacy behavior. In any case I am pretty dang certain this is a browser difference not a React bug (could be wrong). I am seeing the click on the icon fire on Chrome 62.0.3202.94 and Mac 10.12 by the way @thysultan

@thysultan
Copy link

thysultan commented Nov 20, 2017

With all extensions disabled(just in case, b/c inline events) this is what i get on all the browsers i have.

  • Chrome 62 does not emit.

  • Firefox 57 does not emit.

  • Chrome Canary 64 does emit.

  • Safari 10.1 does emit.

  • Safari Technical Preview 11.0 does emit.

@jquense
Copy link
Contributor

jquense commented Nov 20, 2017

i definitely getting the alert popping up for Chrome so i'm not sure where the difference in observed behavior is in https://jsfiddle.net/b4wycvwf/1/

@thysultan
Copy link

thysultan commented Nov 20, 2017

Replacing "click" with mousedown works on all – Firefox, Chrome and Safari. However mouseup like click also doesn't emit on Firefox as well.

So on my machine Chrome and Firefox have different behavior on click from Safari.
Firefox has different behavior on "mouseup".

Not sure what could be causing the difference between me, @jquense and @rstims 's Chrome observations. Tried using addEventListener instead of inline events and removing the font-awesome.css dependency which i thought could be obscuring the result with pointer-events, but both changes also yield the same results.

@jquense
Copy link
Contributor

jquense commented Nov 20, 2017

in either case tho, I don't see much evidence that this is React related. It seems to be solely differences in how browsers treat disabled elements and mouse events yeah?

@thysultan
Copy link

Yes that is more likely. If @ClemDelp also observed the same differences(in Firefox) with the example in the original post, then that is probably the case.

@ClemDelp
Copy link
Author

I confirm that it's not a React issue but really how browsers treat disabled element. I only have the problem on chrome & safari

@jquense jquense closed this as completed Nov 21, 2017
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

5 participants