facebook / react

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

Add 'onClickoutside' event #579

Closed hojberg closed 9 years ago

hojberg commented 10 years ago

Add the 'clickoutside' event similar to this jquery plugin: https://github.com/cowboy/jquery-outside-events

Very useful sugar for components like dropdowns and modals that need to close/hide when you click out side the container.

petehunt commented 10 years ago

I think the right fix for this is layers. Basically adding a transparent div on top of the entire viewport and capturing clicks on that.

Here's a very simple way to do that: http://jsfiddle.net/LHmwd/

hojberg commented 10 years ago

That is pretty much how I do that now as well.

The suggestion here is to create sugar around a common ui pattern. This could be done with a listener on document instead of a transparent div (which would need styling).

jordwalke commented 10 years ago

Cool idea. We should make it as easy as adding:

'react-click-outside-plugin': 'github.com/someone/react-click-outside-plugin'

To you package.json, and then in the entrypoint of your app

require('react/event/EventPluginHub').injectPluginsByName({
   'react-click-outside-plugin': require('react-click-outside-plugin')
});

@petehunt: Sound good? Let's make sure that our sharable components work supports not only components but also react plugins in general.

sophiebits commented 10 years ago

Right now I think this isn't practical because there's no way to get a list of all components that have the event handler attached, so currently the only way to create the plugin would be to have it dispatch the event to literally every element on the page.

jordwalke commented 10 years ago

We could pretty easily add an additional event "propagator" in EventPropagators.js. If you look at how all the registered events are stored inside of CallbackRegistry, they're conveniently stored by their registration name (search for listenerBank[registrationName]). That way, when we get a click on some ID, we only need to search through all the previously registered onClickOutside ids, and match it against the parent path of where the click occurred.

jordwalke commented 10 years ago

It might even be possible with only a single change to the core:

  forEachListener: function(registrationName, cb) {
     var listenerIDs = listenerBank[registrationName] && listenerBank[registrationName];
     forEachKeyVal(listenerIDs, cb);
  },

That would give you access to all of the listeners of a particular type. Then anyone can build their own custom propagators. That match each listener against (not being in the parent path of where a click occurred).

felixakiragreen commented 10 years ago

+1

stopachka commented 10 years ago

+1

syranide commented 10 years ago

:-1: I don't think this is the right solution to the problem. There exists much more flexible (but simple) solutions, that would also be applicable to all events, and not just click.

srcspider commented 10 years ago

+1

stopachka commented 10 years ago

@syranide do you have some refs I could take a look at for better ways to handle this problem? Recently implemented a custom select component, and this was the only solution I came up with for opening and closing a dropdown.

chenglou commented 10 years ago

I'm with @syranide. onClickOutside is a really brute-forced approach and it feels really wrong. There should be a general topOnClick && !clickedOnNode kind of thing.

oriSomething commented 10 years ago

@hojberg why don't you just create a mixin for this?

willdady commented 10 years ago

@petehunt Correct me if I'm wrong, but for the layer approach to work with a dropdown the layer would need to sit behind the dropdown and cover every other element on the page. The issue here is that if the dropdown exists inside a stacking context other that the root document this simply won't work.

syranide commented 10 years ago

@willdady I don't have (an explanation of) the code right now (it's not at all complex), but I made a (for us) reusable self-contained drop-down component for React, anchored in place too (not dynamically positioned by JS). It's all React, no direct DOM access and it works as intended from anywhere within our app, including popups, and closes if you click outside (+eats the click) the drop-down. Works in all browsers React supports.

So unless there's something we're NOT doing, that you are, that would break our component. It seems that it shouldn't be a problem.

willdady commented 10 years ago

@syranide I was able to solve it by simply attaching a click handler to the document inside componentDidMount so when it fires it simply closes the dropdown. I then simply attach an onClick handler to my dropdown element which calls event.nativeEvent.stopImmediatePropagation();.

eddievlagea commented 9 years ago

@willdady can you please provide an example of how you solved it? It's still not clear to me what's the best practice for doing this.

willdady commented 9 years ago

@creativityhurts The below pseudo-code is how to do it. The general idea is that if the user clicks anywhere on the page the dropdown will close EXCEPT if they click anywhere within the dropdown.

var Dropdown = React.createClass({

  componentDidMount: function() {
    document.addEventListener("click", this.documentClickHandler);
  },

  componentWillUnmount: function() {
    document.removeEventListener("click", this.documentClickHandler);
  },

  documentClickHandler: function() {
    this.setState({
      isOpen: false
    });
  },

  triggerClickHandler: function() {
    this.setState({
      isOpen: true
    });
  },

  dropdownClickHandler: function(e) {
    e.nativeEvent.stopImmediatePropagation();
  },

  render: function() {

    var className = "dropdown";
    className += this.state.isOpen ? " open" : "";

    return (
      <div className={className}>
        <a href="#" onClick={this.triggerClickHandler}>Dropdown trigger</a>
        <ul onClick={this.dropdownClickHandler}>
          <li>Item</li>
          <li>Item</li>
          <li>Item</li>
          <li>Item</li>
        </ul>
      </div>
    );
  }

});
Pomax commented 9 years ago

I'm currently doing this with a mixin, using the same "listen for 'click' globally, then hand off locally if necessary" that React does, so perhaps this is easier to port into React as an onClickOutside={this.someHandler}:

// mixins/onclickoutside.js
var OnClickOutside = {
  registerOutsideClickListener: function(handler) {
    if(!handler) return;
    var localNode = this.getDOMNode();
    document.addEventListener("click", function(evt) {
      var source = evt.target;
      var found = false;
      // if source=local then this event came from "somewhere" inside and should be ignored.
      while(source.parentNode) {
        found = (source === localNode);
        if(found) return;
        source = source.parentNode;
      }
      // not found: genuine outside event. Handle it.
      handler(evt);
    });
  }
};

I'm mixing this into my components and then using it in the componentDidMount function as a this.registerOutsideClickListener(this.handler) call.

That works exactly as expected, but I'd much rather be able to use that as a native React onClickOutside={this.handler} of course =)

hasPatrickC commented 9 years ago

@Pomax I think I've found a context where your mixin doesn't quite behave as expected. If you have a sub-component that has it's own click action that ends up removing that component from the DOM, the source.parentNode loop breaks before it makes it's way back up to the actual parent component.

So in this case, if you have a dropdown that has a list of items that have (x) icons that delete items from the dropdown list, whenever you delete an item the outside click will be registered and the dropdown will close.

Pomax commented 9 years ago

interesting - if you have a jsbin that I can drop this into, we can maybe do a bit of iterative improvement until we have a mixin that's robust enough to just npm publish as react-onclickoutside or the like.

willdady commented 9 years ago

@Pomax you're trying to manually reinvent event propagation with that loop. This is why I used stopImmediatePropagation in my example. You don't need to check if the event belongs to a specific element because the event is "stopped" before it bubbles up to the handler listening on the document.

Pomax commented 9 years ago

@willdady I'll take your if it's available in mixin form - anything that I can point other people at and go "solved problem, even if not in React (yet?)"

chiefGui commented 9 years ago

Hey guys, something new so far?

I was thinking about the subject and something simpler comes out: your component is contained by .container class, as instance; if you click in something else which isn't .container, then unmount. Sounds good?

Maybe CSS can resolve our problem, I just have to confirm if :not() selector can do this for us.

Pomax commented 9 years ago

Even if that were an option, it's not the one I'd use: it would rely on an implementation detail, without that detail being part of the React API. It also relies on a uses CSS classes as if they're functional roles, which is literally the reason I like React so much: it doesn't rely on query selecting everything =)

I'll try to publish a react-onclickoutside mixin to npm today based on the code I had mixed up with the improvement @willdady pointed out.

gaearon commented 9 years ago

Layered approach works fine for me, so that's what I normally use. Sometimes you also want to fade that layer and make it dark instead of transparent (e.g. for modals) so layer is natural IMO for solving this problem.

Yes this presumes dropdown is in top stacking context but I think you're just making problems for yourself if you try to keep them somewhere else. For example, if dropdowns lived somewhere else, they would be clipped by parents' overflow: hidden. Also z-index woes, good luck with that. I did this mistake and regretted it.

If you really really want to declare dropdown props, etc, inside components deep in hierarchy you can always use portals, so dropdown is still physically hosted at the top.

onClickOutside seems like a weird thing to me because you'll want to these clicks to not have any effect if *Outside handler says so. Clicking outside context menu should only hide context menu and not execute any actions underneath even if there was a button there. That would only be possible if *Outside ran before actual handlers so it could stop immediate propagation. This feels like a hack to me..

So -1 on this, I think it's a wrong solution to the problem. Embrace layers.

(Note that Flux makes it super easy to trigger actions that affect something on top.)

Pomax commented 9 years ago

While it's great for that purposes, it also seems exclusively intended for dealing with modals. That's a level of specialization I don't want to have to abuse for genuine "I need to listen for clicks outside this element" behaviour that has nothing to do with modal dialogs in the slightest (like blur behaviour at the component level, switching between "an element" and an editor for its content, etc)

chiefGui commented 9 years ago

@pomax Good point! But certainly, using CSS is a fast & easy way to go — it worked for me! But you are right, doesn't make any sense unleash this approach since I'm using the powerfulness of React.

I removed completely the "outsideClick" approach of my app and by now I am waiting for your mixin. Meanwhile, users should to press escape to close their component. (lol)

gaearon commented 9 years ago

Yeah I see your point.

I use layers for anything "global" that can only have single instance and has to be on top (e.g. modal, context menu, alert, sidebar). In this case, I only have one "dimming" layer that listens to ModalStore, ContextMenuStore, AlertStore and decides whether to show overlay (transparent in some cases) or not.

For rare cases I really need "click outside" at component level and can't use layers for that, I put e.preventPropagation in component's node's onClick handler and set up a document.addEventListener('click', this.handleBodyClick) handler separately. This solves the problem you described for me.

I don't even think this needs a mixin: it's little code and still very explicit.

  componentDidMount: function () {
    document.body.addEventListener('click', this.handleBodyClick);
  },

  componentWillUnmount: function () {
    document.body.removeEventListener('click', this.handleBodyClick);
  },

  render: function () {
    return <div onClick={this.handleClick}...</div>;
  }

  handleClick: function (e) {
    e.preventPropagation();
  },

  handleBodyClick: function () {
    // do something
  }
chiefGui commented 9 years ago

@gearon I think a mixin it's more DRY and reachable than a code snippet. Also, your solution is wise and fresh, man. I'll give it a shot soonish and come back with the results. Yet, still waiting for @Pomax's mixin if he still is meant to keep his promise.

Pomax commented 9 years ago

A promise I made 3 hours ago on a work day =)

Although I did publish https://www.npmjs.com/package/react-onclickoutside so have at it and if you run into breakage, issues are tracked on github

gaearon commented 9 years ago

I think a mixin it's more DRY and reachable than a code snippet.

Kinda but.. you'd need to not forget to do stopPropagation in your own click handler (which you need to remember to attach to root node). So it's like half of the code is outside the mixin anyway. Certainly you can implement it with a while loop instead of setPropagation but this rubs me in a wrong way, maybe it's just aesthetics though and I'm just being boring.

@Pomax

Wouldn't it be better if mixin stored functions in a private map and removed them automatically in componentWillUnmount? Prevents accidental leaks & removes the need for private function.

Pomax commented 9 years ago

if you can point me to the docs on triggering functions automatically when the owning component reaches certain life cycle points, I'll be more than happy to work that in. It would be nicer if it automatically hooked into both componentDidMount and componentWillUnmount, simply binding and unbinding this.onClickOutside (which can then do whatever additional function routing required).

chiefGui commented 9 years ago

you'd need to not forget to do stopPropagation in your own click handler

I was thinking about this. Faced this problem just right now, lol. But you're not being boring — you're thinking in the best fashion to get it done.

@Pomax I was kidding about the pressure, lmao. But man, thank you so far — you were fast as hell.

gaearon commented 9 years ago

It would be nicer if it automatically hooked into both componentDidMount and componentWillUnmount, simply binding and unbinding this.onClickOutside (which can then do whatever additional function routing required).

The fun part about React mixins is that this is what happens when you define componentDidMount and componentWillUnmount in the mixin itself ;-) See example in docs.

Pomax commented 9 years ago

excellent. I'll update the mixin.

Pomax commented 9 years ago

updated mixin as v0.1.0 since it has a different implementation now: simply add the mixin and define an onClickOutside: function(evt) { ... } in the component using the mixin.

gaearon commented 9 years ago

I'd call it handleClickOutside because that's the convention for function names (on* is convention for prop names)

Pomax commented 9 years ago

fair point. updated (v0.2.0)

chiefGui commented 9 years ago

I've never seen a npm module being updated so much and so faster — have we a new record here? Lol.

Already using your solution, @Pomax. You, @gaearon and all those who collaborate for this mixin are my heroes. :heart_eyes_cat:

Pomax commented 9 years ago

this is pretty usual for a first hour module push, in my experience, because that first hour has all the immediate bug reports from people you just linked it to =)

yiminghe commented 9 years ago

this problem should be solved by focus and blur event.

if click document, dropdown lose focus then hide dropdown menu

Pomax commented 9 years ago

focus events are not the same as click events: a drop down that disappears on a "click outside" will stay open even if you change tabs or switch out/back in to the browser application itself. However, both those actions will cause blur events, so if you were listening to those your drop down would have closed. The two event types trigger differently.

chiefGui commented 9 years ago

What @yiminghe said it's a good idea, IMHO. I didn't think this way, but your mixin @Pomax saved lives.

hnqso commented 9 years ago

For anyone interested I'm using a different approach with signals where in my case if you click outside of a component is because you're interacting with another.

ObjectSignal.js

var Signal = require('signals');
var ObjectSignal = (function(){
  return {
    componentClicked: new Signal()
  }
}());
module.exports = ObjectSignal;

Component1.js

handleClick: function(e) {
  ObjectSignal.componentClicked.dispatch(this.state.id);
}

Component2.js

componentDidMount: function() {
  ObjectSignal.componentClicked.add(this.componentClicked);
},

componentWillMount: function() {
  ObjectSignal.componentClicked.remove(this.componentClicked);
},

componentClicked: function(id) {
  console.log('componentClicked:', id);
  // check if dropdown is opened, etc
}

Cheers 🍺

johnarnfield commented 9 years ago

I don't like the idea of stopping event propagation on such a scale.

Unfortunately, React makes it very hard to handle events consistently when you mix it in with libraries that expect events to work in a normal fashion.

How about circumventing React all together?

    componentDidMount: function () {
        document.addEventListener('click', this.onDocumentClick);
    },

    componentWillUnmount: function () {
        document.removeEventListener('click', this.onDocumentClick);
    },

    onDocumentClick: function(event) {
        var main_menu_button = React.findDOMNode(this.refs.main_menu_button),
            main_menu = React.findDOMNode(this.refs.main_menu);

        if (main_menu_button.contains(event.target)) {
            // Clicking on the button should toggle the overlay
            this.setState({main_menu_shown: !this.state.main_menu_shown});
        } else if (!main_menu.contains(event.target)) {
            // Clicking anywhere else, outside the overlay, should close it
            this.setState({main_menu_shown: false});
        }
    },
Pomax commented 9 years ago

@johnarnfield so, literally what the react-onclickoutside mixin does, then? =) (https://github.com/Pomax/react-onclickoutside/blob/master/index.js#L61)

ericsuh commented 9 years ago

@Pomax I implemented a component that relies a bit more on React/DOM event handling rather than assumptions about the component tree. I generally favor components over mixins for React: https://www.github.com/ericsuh/react-outsideclickhandler.git

Pomax commented 9 years ago

I prefer the complete opposite: I have no desire to see things like the following, if I need a reasonably functional component:

<div>
  <FunctionalityElement1/>
  <FunctionalityElement2/>
  <FunctionalityElement3/>
  <FunctionalityElement4/>
  { my actual content }
</div>

mixins solve this horrible XML hell by using the same JS syntax that we already use for any other functionality, by letting us say:

React.createClass({
  mixins: {
    require(...),
    require(...),
    ..
  },
  render() {
    return <div>{ my content, no spam around it}</div>
  }

It just feels so much better. Since most behaviour is not tied to "a component", but tied to "functions that effect that behaviour", mixins make far more sense, provided they are real mixins: if they don't tie into the React lifecycle functions, they are not mixins, they're just plain .js functionality and you should just require them the plain old require() way

ericsuh commented 9 years ago

To each his/her own. I consider mixins and inheritance in general to be used sparingly, especially in a type-unsafe language like JavaScript. Composition is almost always better, IMO.

On May 17, 2015, at 7:29 PM, Mike Kamermans notifications@github.com wrote:

I prefer the complete opposite: I have no desire to see things like the following, if I need a reasonably functional component:

{ my actual content }

mixins solve this horrible XML hell by just letting me say:

React.createClass({ mixins: { require(...), require(...), .. }, render() { return

{ my content, no spam around it}
} It just feels so much better.

— Reply to this email directly or view it on GitHub https://github.com/facebook/react/issues/579#issuecomment-102897363.