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

<Dropzone> cause onClick to fire twice on parent <div> #252

Closed
aaronbeall opened this issue Oct 12, 2016 · 47 comments · Fixed by #437
Closed

<Dropzone> cause onClick to fire twice on parent <div> #252

aaronbeall opened this issue Oct 12, 2016 · 47 comments · Fixed by #437
Labels

Comments

@aaronbeall
Copy link

I ran into an issue with click handlers and drag handlers being fired twice on a <div>, and found that it happens when I have <Dropzone> as a child of the <div>.

Before <Dropzone>:

<div onClick={e => console.log("click", e)}>
</div>

Clicking the <div> results in:
image

After <Dropzone>:

<div onClick={e => console.log("click", e)}>
  <Dropzone className="upload-drop-area"
            activeClassName="upload-drop-area-accept"
            rejectClassName="upload-drop-area-reject"
            multiple={false}
            onDragEnter={this.onDragEnter}
            onDragLeave={this.onDragLeave}
            onDropAccepted={this.onDropAccepted}>
  </Dropzone>
</div>

Clicking the <div> results in two events:
image

I've never seen this kind of thing in React before. Any idea what's going on?

@aaronbeall
Copy link
Author

aaronbeall commented Oct 13, 2016

A few more observations:

  • If I replace the <Dropzone> with equivalent content <div><input/></div> the problem doesn't exist. Only when I use <Dropzone> does it happen.
  • The problem happens for onClick but not onMouseOver.
  • If I delete the Dropzone rendered <input> from DOM (using chrome elements inspector) clicking no longer causes two events.

@okonet
Copy link
Collaborator

okonet commented Oct 13, 2016

Dropzone handles clicks and it will bubble the event up to its parents.

https://github.com/okonet/react-dropzone/blob/master/src/index.js#L276

You should prevent the event going down the tree in your handler if you want to prevent this behavior.

@aaronbeall
Copy link
Author

I'm not sure I understand. Bubbling shouldn't result in multiple events.... you're saying that because Dropzone has an onClick handler that causes two callbacks to fire on the parent?

@aaronbeall
Copy link
Author

aaronbeall commented Oct 13, 2016

I think this is the problem:

https://github.com/okonet/react-dropzone/blob/master/src/index.js#L182

You are calling click() on the input from an onClick handler, the input click event bubbles so you end up with two onClick callbacks getting fired (one for the user's click, one for the input.click()).

Similarly for the drag event handlers such as onDragStart you explicitly call props.onDragStart but the original event will bubble to the parent so you end up invoking the parent callback a second time:

https://github.com/okonet/react-dropzone/blob/master/src/index.js#L41

I think in both cases Dropzone should stop propagation or not explicitly invoke the callback and let bubbling work.

@okonet
Copy link
Collaborator

okonet commented Oct 14, 2016

im not quite sure about the behavior. Can you create an isolated example with a vanilla JS and DOM? We probably should stop propagation as you said. Mind creating a PR?

@okonet okonet reopened this Oct 14, 2016
@aaronbeall
Copy link
Author

Sure I'll give this a shot next week.

@aaronbeall
Copy link
Author

@okonet Looking into this. Just curious about the workflow (I don't maintan any libraries myself), how do you go about visually testing this? Do you just create a separate project and add react-dropzone as a dependency?

@okonet
Copy link
Collaborator

okonet commented Nov 8, 2016

Yes, I'd do something like it. It would be great to get the isolated case in the end if not the PR with a the bug fix.

@aaronbeall
Copy link
Author

As far as an isolated case, a fresh project (using create-react-app) with this code:

import React from 'react';
import ReactDOM from 'react-dom';
import Dropzone from "react-dropzone";

ReactDOM.render(
  <div onClick={e => console.log("click", e)}>
    <Dropzone>
      <div>Click me.</div>
    </Dropzone>
  </div>,
  document.getElementById('root')
);

Gives this output when you click the dropzone:

image

Notice the first event target is input and the second event target is div.

If I stop the input event from propagating by modifying Dropzone rendering of the input to this:

<input
  {...inputProps/* expand user provided inputProps first so inputAttributes override them */}
  {...inputAttributes}
  onClick={e => e.stopPropagation()}
/>

Then I just get the single div click as expected. I think this is the right way to handle it, I can't think of any case you'd expect to see the input click event, since this is basically an implementation detail of how Dropzone spawns the browser file picker dialog. I'll add a PR for this change.

As for the drag events firing twice, that does not happen in the above simple test, so I'll have to dig deeper to find out why that was happening in my real app. Maybe nothing to do with Dropzone.

@okonet
Copy link
Collaborator

okonet commented Dec 14, 2016

I've tried to add a test but the bug isn't really introducible. This is what I've used:

it('invoke onClick only once', () => {
      const onClickSpy = spy();

      const component = TestUtils.renderIntoDocument(
        <div id="content" onClick={onClickSpy}>
          <Dropzone>
            <div>Click me</div>
          </Dropzone>
        </div>
      );
      const content = TestUtils.find(component, '#content')[0];
      TestUtils.Simulate.click(content);
      expect(onClickSpy.callCount).toEqual(1);
    });

Interestingly, even if I remove your fix, it is still 1

@aaronbeall
Copy link
Author

You need to simulate the click on the inner div (or possibly the inner Dropzone though I'm not sure that's an accurate test since it's not a DOM element.) I couldn't work out how to do it, all my attempts to select the inner element with TestUtils led to invariants about trying to find the wrong thing, or simply gave me 0 results.

@okonet
Copy link
Collaborator

okonet commented Dec 14, 2016

No matter where I simulate the click, it only triggers 1 call :/

@aaronbeall
Copy link
Author

Hm. Does my original example not exhibit the behavior for you? It could be that simulate click doesn't behave the same (intentionally?) as a real DOM click. It's certainly a weird test case: testing the behavior of a wrapping component when the component under test is put inside it...

@okonet
Copy link
Collaborator

okonet commented Dec 15, 2016

I'm trying Enzyme today and will see if I can repro it on JSDom. Ill let you know

okonet pushed a commit to aaronbeall/react-dropzone that referenced this issue Dec 22, 2016
@okonet okonet closed this as completed in 0ea3c19 Dec 22, 2016
@pouu69
Copy link

pouu69 commented Dec 23, 2016

i have same issue , but not working yet

@okonet
Copy link
Collaborator

okonet commented Dec 23, 2016

It's been fixed in https://github.com/okonet/react-dropzone/releases/tag/v3.8.0. Please upgrade

@pouu69
Copy link

pouu69 commented Dec 23, 2016

already upgrade v3.8.0

@okonet
Copy link
Collaborator

okonet commented Dec 23, 2016

@pouu69 I'm not sure what you're saying. Is it still reproducible in 3.8?

@pouu69
Copy link

pouu69 commented Dec 23, 2016

I am already version 3.8.
but not working click event.
This is the same situation as the above issue.

@pouu69
Copy link

pouu69 commented Dec 23, 2016

<div
    className="block-img-wrapper"
    onClick={this.changeRootSlide}
>
    <div>
        <Dropzone
            multiple={false}
            className="block-dropzone"
            disableClick={true}
            accept="image/*"
            ref='dropzone'
            style={styles.dropzone}
            onDrop={this.html5FileHandling}
        >
            <div className="trigger">
            </div>
        </Dropzone>
    </div>
</div>

@okonet
Copy link
Collaborator

okonet commented Dec 23, 2016

disableClick={true} will disable the click event on dropzone. I'm not sure what you expect and what's the actual result you're getting so please create a repo where I can reproduce it.

@ghost
Copy link

ghost commented Jan 10, 2017

With current fix an IconMenu from MaterialUI stopped working properly as it is not hiding after clicking on a DropZone.
I deeply believe that e.stopPropagation() shouldn't be invoked when disableClick is true.

@okonet okonet added bug and removed question labels Mar 13, 2017
@LaustAxelsen
Copy link

Just add a pointer-events: none; css-rule on the dropzone DIV dom-node.. that fixed it for me.

@okonet
Copy link
Collaborator

okonet commented May 2, 2017

That's a bad advice since it will break click to open file dialog functionality AFAIK. Also it seems that it was resolved or is it still happening?

@LaustAxelsen
Copy link

@okonet it does not break click to open file dialog functionality. The bug is still happening, just tried it yesterday on v3.13.1.

I know it sounds way off and ugly with the css-fix, but the alternative for me is that the "open dialog" hit's twice - where the first file selection is dropped.

@okonet okonet reopened this May 2, 2017
@okonet
Copy link
Collaborator

okonet commented May 2, 2017

@LaustAxelsen the reason for me saying this is that I tried it and it broke something for me. I can't remember what was it though.

I think this issue should be fixed in this package. Anyone?

@okonet
Copy link
Collaborator

okonet commented May 2, 2017

Can someone create a reproducible example?

@fetzig
Copy link

fetzig commented May 5, 2017

Same issue here. Newest version. Use dropzone in a semantic-ui project, and the <Dropzone> node triggers 3 times the click event - opens the file dialog - reloads the page.

Behaviour seems consistent across browsers. Use newest release (3.11).

It's obviously an edge case, thus reproducing this bug might headache on its own.

I will try to find the time and make a proper Bug Report and maybe PR.

My current assumption is, this happens when Dropzone has children like input/button/a. Clicking these results in bubbling the event upwards, and for some reason this results in multiple click events. First event handled properly by <Dropzone>, and others bubble further up.

@fetzig
Copy link

fetzig commented May 5, 2017

fixed my bug. really obvious, sorry for the noise:

<Dropzone
    multiple={false}
    className="dropzone"
    activeClassName="dropzone-active"
    onDrop={this.onDrop}
    onDragEnter={() => this.setState({ isDragging: true })}
    onDragLeave={() => this.setState({ isDragging: false })}
    ref={(node) => { dropzoneRef = node; }}
    >
        <Button
            onClick={(e) => {
                e.preventDefault(); // --> without this onClick fires 3 times
                dropzoneRef.open();
            }}>
            <Icon name="upload" />Click here
        </Button>
</Dropzone>

@kylepotts
Copy link

I am also getting this same issue. Version 3.13.1. Even with e.preventDefault()

@TigerWolf
Copy link

I am also getting this on 3.13.2. I only have text as a child of the Dropzone element.

@okonet
Copy link
Collaborator

okonet commented Jun 6, 2017

Can someone create a reproducible example here http://react-dropzone.netlify.com/ and paste code sample here?

@kylepotts
Copy link

kylepotts commented Jun 6, 2017

@okonet https://github.com/kylepotts/react-dropzone-bug-example

Just do the normal npm install npm start. First dropzone will console.log twice. Second dropzone will console.log once.

import React, { Component } from 'react';
import Dropzone from 'react-dropzone';

let dropzoneRef1;
let dropzoneRef2;

class App extends Component {

  render() {
    return (
      <div>
        <Dropzone ref={(node) => { dropzoneRef1 = node; }} onDrop={(accepted, rejected) => { alert(accepted) }}>
          <p>Drop files here.</p>
        </Dropzone>
        <button type="button" onClick={() => { console.log('this will execute twice'); dropzoneRef1.open() }}>
          Open File Dialog
      </button>
      <Dropzone ref={(node) => { dropzoneRef2 = node; }} onDrop={(accepted, rejected) => { alert(accepted) }}>
        <p>Drop files here.</p>
      </Dropzone>
      <button type="button" onClick={() => { console.log('this will execute once');}}>
        Open File Dialog
      </button>
    </div>
    );
  }
}

export default App;

@kylepotts
Copy link

I've created a bugfix here #437. It should still respect the onClick event in inputProps if that is set.

@okonet
Copy link
Collaborator

okonet commented Jun 14, 2017

I've spent some time on this one today and I still can't reproduce it the way the issue title says. Here is what I tried:

<div
 className="block-img-wrapper"
 onClick={(evt) => {
   console.log('parent')
 }}
>
 <Dropzone
   disableClick={false}
   >
 </Dropzone>
</div>

This is example code from one of the comments. If you paste it on https://react-dropzone.js.org, depending on disableClick prop being true or false you'll see either a file dialog or parent in the console but not both. I've tried the same code with CRA setup and the behavior was exactly the same. So, if you experience this issue, I still encourage you to create a minimal reproducible example as a GitHub repository and share it with me.

As for @kylepotts example, it's less trivial. I've tested the code you provided at https://react-dropzone.js.org/ and couldn't reproduce the issue with invoking twice. But I could reproduce it when checked out from GH locally! Looking at your example, it seems to be a different from the described here since you don't nest the button element in the Dropzone.

Changing the example to the following (note how I nest a button in dropzone):

let dropzoneRef1;

     <div>
        <Dropzone ref={(node) => { dropzoneRef1 = node; }} onDrop={(accepted, rejected) => { alert(accepted) }}>
          <p>Drop files here.</p>
                 <button type="button" onClick={() => { console.log('this will execute twice'); dropzoneRef1.open() }}>
          Open File Dialog
      </button>
        </Dropzone>
    </div>

(You can paste it into one of the example's code on https://react-dropzone.js.org/ to test)

This will open the file dialog twice but it will log once. That is expected behavior. Since by default Dropzone will open File Dialog when you click on it. See https://react-dropzone.js.org/#proptypes. If you don't want this behavior, you have to use disableClick prop on it.

@kylepotts I think there is another issue that causes this but I don't understand what it is. I've tried to reproduce it on webpackbin: https://www.webpackbin.com/bins/-Km_p_Qa7aTNt4sn5PxC but it works as expected there. ~~~Any idea?~~~

There is a React issue about this: facebook/react#1691

@hoodsy
Copy link

hoodsy commented Feb 12, 2018

@okonet I'm still experiencing this – the "Choose File" dialogue appears twice. If I exit after the first dialogue, no file is uploaded.

EDIT: Worked around this by adding onClick={e => e.preventDefault()} to my Dropzones

@jasan-s
Copy link

jasan-s commented May 2, 2018

adding this did not help onClick={e => e.preventDefault()}

@twelve17
Copy link

twelve17 commented Mar 13, 2019

I also got the "Choose File" dialog twice, but only if the <input> and the <div> have other elements between them (I am using Grommet):

          <div {...getRootProps()}>
            <Box align="center" pad="large">
              <Label size="small">
                <input {...getInputProps()} />
                Drop an image or click to select a file to upload.
              </Label>
            </Box>
          </div>

If I modify the above code like this, then I only get one upload dialog:

           <div {...getRootProps()}>
            <input {...getInputProps()} />
            <Box align="center" pad="large">
              <Label size="small">
                Drop an image or click to select a file to upload.
              </Label>
            </Box>
          </div>

@rolandjitsu
Copy link
Collaborator

Could be helpful to open a new issue with a bit of description and a reproducible example because it's hard to figure out from all the comments what the issue is.

@mweibel
Copy link

mweibel commented Nov 27, 2019

Maybe a few more pointers:
I experienced the same issue as here but it only seems to happen in Chrome. Firefox handles it correctly (IMO).
Code to paste on https://react-dropzone.js.org:

import React from 'react';
import {useDropzone} from 'react-dropzone';

function Basic(props) {
  const {acceptedFiles, getRootProps, getInputProps, open} = useDropzone();

  const files = acceptedFiles.map(file => (
    <li key={file.name}>
      {file.name} - {file.size} bytes
    </li>
  ));

  return (
    <section className="container">
      <div {...getRootProps()}>
        <input {...getInputProps()} />
        <p>Drag 'n' drop some files here, or click to select files</p>
        <button onClick={open}>Browse Files</button>
      </div>
      <aside>
        <h4>Files</h4>
        <ul>{files}</ul>
      </aside>
    </section>
  );
}

<Basic />

Reproducing:

  1. click on the dropzone: all works well. File dialog only opens once
  2. click on browse files button: FF: all works well. Chrome: file dialog opens twice, and only the second one actually uploads the file

Using e.preventDefault() doesn't help:

<button onClick={(e) => { e.preventDefault(); open(e)}>Browse Files</button>

only removing the onClick handler seems to help and all still works.

@Nickadiemus
Copy link

Can confirm this issue still persists

@bondia
Copy link

bondia commented Oct 8, 2021

Working in an old project and landed with this issue. Someone got that solved?

Like that seems to work:

  const onOpen = useCallback<MouseEventHandler>(
    e => {
      e.preventDefault();
      e.stopPropagation();
      open();
    },
    [open],
  );

<div className={classes.dropzone} {...getRootProps()} onClick={onOpen}>

@webdiego
Copy link

webdiego commented Mar 8, 2022

Working in an old project and landed with this issue. Someone got that solved?

Like that seems to work:

  const onOpen = useCallback<MouseEventHandler>(
    e => {
      e.preventDefault();
      e.stopPropagation();
      open();
    },
    [open],
  );

<div className={classes.dropzone} {...getRootProps()} onClick={onOpen}>

Adding in onClick the functions: e.stopPropagation(), e.preventDefault() and open(); it fixed the issue

@HabardiT
Copy link

HabardiT commented Jun 9, 2022

it might not be a solution for other developers who commented here.
but in my case I had it this way.

<label>
   ...
    <div {...getRootProps()}>
        <input {...getInputProps()} />
    </div>
</label>

what happens here is that the label links to the closest input child.
and that's why it triggered twice in my case.

I just changed the label element to a div. even though I prefer label for these cases.

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

Successfully merging a pull request may close this issue.