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

Is focusFirstSuggestion working #199

Closed
avjpl opened this issue Jul 14, 2016 · 12 comments
Closed

Is focusFirstSuggestion working #199

avjpl opened this issue Jul 14, 2016 · 12 comments

Comments

@avjpl
Copy link

avjpl commented Jul 14, 2016

By added focusFirstSuggestion: true break the selection. It only selects the first suggestion. Is there anything else apart from setting this to true that needs to be done?

This is the object I am paasing to autosuggest.

   const autosuggestProps = {
      suggestions: [].concat(children || [], suggestions),
      getSuggestionValue: identity,
      shouldRenderSuggestions: () => true,
      renderSuggestion: this.renderSuggestion.bind(this),
      onSuggestionsUpdateRequested: this.onSuggestionsUpdateRequested,
      onSuggestionSelected: this.onSuggestionSelected,
      focusFirstSuggestion: true,
      theme: this.props.theme,
      inputProps: {
        onChange: this.onChange,
        onBlur: this.onBlur,
        onKeyPress: this.onKeyPress,
        value: displayInput,
        placeholder: this.props.placeholder,
      }
    };

   <Autosuggest {...autosuggestProps}/>
@moroshko
Copy link
Owner

@avjpl I'm not sure I understand the issue you are experiencing.

Here is a Codepen that demonstrates focusFirstSuggestion={true}.

Could you please elaborate a bit more, and update the Codepen accordingly to demonstrate the problem?

@avjpl
Copy link
Author

avjpl commented Jul 15, 2016

I apologise for not being clear, but I wasn't sure why the behaviour I was seeing was happening.

After seeing your Codepen, thank for that, I went back to my codebase and narrowed down what was causing the problem.

It was caused by the following line.

suggestions: [].concat(suggestions)

But I am not quite sure why.

Here is a Codepen example.

I get the same behaviour with slice too

suggestions: suggestions.slice()

@moroshko
Copy link
Owner

Thanks @avjpl, now I see that Up/Down doesn't work properly.

@rickhanlonii Could you have a look?

@rickhanlonii
Copy link
Contributor

@moroshko @avjpl I'll take a look

@moroshko
Copy link
Owner

@rickhanlonii Any luck?

@rickhanlonii
Copy link
Contributor

@moroshko not yet, last week was pretty busy. I'll take a look this week.

@fixpunkt
Copy link

fixpunkt commented Aug 11, 2016

I've just hit this problem, too.

If you take a look at react-autosuggest/src/Autosuggest.js:

componentWillReceiveProps(nextProps) {
  if (nextProps.suggestions !== this.props.suggestions) {
    const {
      suggestions, inputProps, shouldRenderSuggestions,
      isCollapsed, revealSuggestions, lastAction
    } = nextProps;
    const { value } = inputProps;

    if (suggestions.length > 0 && shouldRenderSuggestions(value)) {
      this.maybeFocusFirstSuggestion();

      if (isCollapsed && lastAction !== 'click' && lastAction !== 'enter') {
        revealSuggestions();
      }
    }
  }
}

This code is doing an identity comparison on the list of suggestions. So if you compute the suggestions prop dynamically in the parent component's render() method, this code path will run on every render and always set the focus to the first element, even if the list of suggestions has not actually changed.

This code should probably do a deep array comparison instead.

@fixpunkt
Copy link

fixpunkt commented Aug 12, 2016

On second thought, a deep comparison is probably wrong. What the code should actually do is check whether either nothing is selected or whatever is selected is no longer in the list of suggestions, and only in this case select the first element.

That's still in O(n) though, so it's not really faster than doing a deep array comparison.

Also note that if your state management code mutates suggestions (which is a silly thing to do, of course), by doing e.g.

// suggestions is ['Hamburg', 'Havana']
suggestions.length = 0
suggestions.unshift('Bangkok', 'Berlin')

the implementation above will fail to detect the change.

@moroshko
Copy link
Owner

Probably makes sense to shallowly compare suggestions in componentWillReceiveProps instead of ===. I'll fix this.

@moroshko
Copy link
Owner

This is fixed in 5.0.1 now.

@fixpunkt
Copy link

Can confirm, 5.0.1 fixes the issue for me. Thank you for fixing this so quickly!

@rickhanlonii
Copy link
Contributor

Great diagnosis @fixpunkt!

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

No branches or pull requests

4 participants