excludeScrollbar does not exclude scrollbar clicks for non-top-level elements

[jameslaneconkling]

This likely isn't a bug, as the docs don't say it's expected behavior. But perhaps could be an enhancement.

It looks like the HOC scrollbar click check only works if the scrollbar is on an element with the same dimensions as the page. This means it wouldn't work if the scrollbar is on an element somewhere further down the DOM tree, e.g. below:

Should scrolling the above John Doe element trigger the click event when excludeScrollbar is set to true? Is it even possible to determine if the click is on a scrollbar?

Would be willing to put together a PR if it's desired functionality.

[Pomax]

The scrollbar fix was actually purely for the document scrollbars, it does not check any elements as such. If you want to extend it so that it covers scrollbars on "anything", I'll be happy to review and merge it in when it does what it should do while also passing tests!

[jameslaneconkling]

OK, I'll take a swing at it. A quick google search returns approaches similar/identical to what's currently implemented. But I did notice that bootstrap's typeahead component (which closes on outside clicks) didn't close on non-document scroll bar clicks, so I might dig in there for other possible approaches.

[jameslaneconkling]

Hmmm, interesting.

I was looking at twitter bootstrap's typeahead component to figure out how they were able to implement the functionality (close on click outside the component, but not on scrollbars), and realized they didn't capture onClick events on external elements at all, but rather monitored the onBlur event of the component itself. An onBlur event is triggered whenever focus is lost on the element, and you can use the blur event's relatedTarget property to determine whether the new focused element is a child of the decorated component, or whether it is external.

e.g.

const closeOnOutsideClick = () => (BaseComponent) => {
  return (
    class CloseOnOutsideClick extends React.Component {
      constructor(props) {
        super(props);
        this.onBlur = this.onBlur.bind(this);
      }

      componentDidMount() {
        this.input.focus();
      }

      onBlur({ currentTarget, relatedTarget }) {
        // currentTarget is HOC Wrapper
        // relatedTarget is whatever element is gaining focus, if any
        // if element gaining focus is w/i Wrapper, refocus on Wrapper
        // otherwise, bubble the onClose event

        if (!relatedTarget || !currentTarget.contains(relatedTarget)) {
          this.props.onClose();
        } else {
          this.input.focus();
        }
      }
      
      render() {
        return (
          <span
            ref={(input) => { this.input = input; }}
            tabIndex="0"
            onBlur={this.onBlur}
          >
            <BaseComponent {...this.props} />
          </span>
        );
      }
    }
  );
};

This effectively prevents you from having input elements (or anything that requires a focus state) w/i the decorated component. But it solves my problem of closing the decorated component when clicking on a non-top-level scrollbar w/o having to estimate scrollbar width (which I tried, without much success). Also note, might not work in IE <= 11.

In any case, probably not applicable to your use case (?). But could be a useful alternative strategy.

[Pomax]

Note that that universal onblur and onfocus only works when you give elements a tabindex, which is an incredibly web-breaking idea if you want to offer a universal library, because it ruins the page experience for people with disabilities who have to tab through your UI to get to the parts they need. Basically this solution abuses a property that's intended for accessibility, by using it in an accessibility-breaking way. Great if that's not a thing you want to care about, not so great if you do =)

As we don't know where this HOC ends up being used, baking in a worse experience for less than capable users is probably a bad idea. Let's stick with monitoring click.

[jameslaneconkling]

because it ruins the page experience for people with disabilities who have to tab through your UI to get to the parts they need.

True in your general case, maybe. In my case (a typeahead autocomplete), being able to tab into/out of the component is the intent, rather than a hack. But yeah, this wouldn't apply to the broader "onClickOutside" case. (I suppose I should rename the above HOC closeOnFocusOutside())

[Andarist]

Closing due to inactivity. If your issue is still unresolved, please comment back and I will reopen this.