reactjs / react-autocomplete

WAI-ARIA compliant React autocomplete (combobox) component
MIT License
2.17k stars 532 forks source link

fix #287 - checks itemNode is not null in maybeScrollItemIntoView #293

Open SkinyMonkey opened 6 years ago

SkinyMonkey commented 6 years ago

When an async function is loading the data:

The itemNode is sometime null in this function : https://github.com/reactjs/react-autocomplete/blob/master/lib/Autocomplete.js#L245.

In the ref function of the renderMenu member function (https://github.com/reactjs/react-autocomplete/blob/master/lib/Autocomplete.js#L437), the ref parameter is null too and sets the refs[item- + index] to null

I tried to not set the key in the ref function if the value was null but it didn't help.

So ultimately I decided to protect the call to scrollIntoView to avoid the exception.

I hope it can fix the problem until you find a better solution if there is one.

PS : sorry, i forgot to read the contribution rules before commiting. My commit is prefixed by fix though.

eduduardo commented 6 years ago

Same as #281

SkinyMonkey commented 6 years ago

I'll close it as soon as the other pull request is merged : it's been needed for a long time, a little reminder for the admins/maintainers doesn't hurt.

eduduardo commented 6 years ago

That's right 😄

nikmilson commented 6 years ago

What about merge this PR? I'm using 1.5.5 version of react-autocomplete with rendering menu to body (due to issues with overflow: hidden blocks) and everything is working great. But from 1.5.7 I get error with nullable itemNode on first render with highlighted item (e. g. arrow down press). I need an update because of renderInput is useful feature for me.

/cc @CMTegner

reduxdj commented 6 years ago

I am still having issues with this always being null, so quickly i monkey-patched this function, by using inheritance ( fast and dirty). checkout the component scroll-into-view, it's pretty sweet.

I also don't show scrollbars in my app because they are king of unnecessary, this stops the outer window form bouncing.

 maybeScrollItemIntoView() {
    const stopMainContentFromScrolling = () => {
      const mainContent = window.document.getElementsByClassName('main_content')[0]
      const contentY = mainContent.scrollTop
      if (mainContent.style) {
        mainContent.style.overflowY = 'hidden'
        mainContent.style.top = contentY
        setTimeout(() => {
          mainContent.style['overflow-y'] = 'auto'
          mainContent.style.top = 0
        }, 1000)
      }
    }

    if (this.isOpen() && this.state.highlightedIndex !== null) {
      const { className } = this.props
      const { highlightedIndex } = this.state
      const elements = window.document.getElementsByClassName(className)
      const itemNode = elements[highlightedIndex]
      stopMainContentFromScrolling()
      scrollIntoView(itemNode)
    }
  }
CMTegner commented 6 years ago

As I've mentioned before, this is most likely due to a mismatch between what you are passing in as props.items and the render-tree that your renderMenu method is returning.

This is documented:

Ensure the returned tree includes every entry in items or else the highlight order and keyboard navigation logic will break.

If you think that this is not the case I'd be happy to look over a reduced test case to confirm that there is in fact a different bug in play.

kaskazurek commented 6 years ago

I got this error (and several others) when I was trying to use a functional component in the render* prop. I did not know that this library uses refs. As you know, functional components do not support refs. I think it should be mentioned in the documentation.

Example (react 15.6.1): https://codesandbox.io/s/32lz0wmmm1 Example (react 16.2.0): https://codesandbox.io/s/8xxq2l5910

With react 16 there is additional warning:

Warning: Stateless function components cannot be given refs. Attempts to access this ref will fail.