palantir / blueprint

A React-based UI toolkit for the web
https://blueprintjs.com/
Apache License 2.0
20.75k stars 2.18k forks source link

[Suggest] Controlled activeItem breaks with new created items #3751

Open bocong opened 5 years ago

bocong commented 5 years ago

Environment

Repro: https://codesandbox.io/s/dry-frog-404t9

Steps to reproduce

  1. Load sandbox for repro: https://codesandbox.io/s/dry-frog-404t9
  2. In the Suggest input, type anything that will create a new element (anything in this case, since no filtering is implemented).
  3. Use arrow keys in an attempt to make the created item the active item.

Actual behavior

Component rerenders and forces an additional onActiveItemChange event to fire, with the first list item as the event's first parameter.

Anything that triggers the parent component to re-render seems to fire this additional event.

This makes it impossible to control activeItem, as doing so would require storing activeItem as state and updating that state in onActiveItemChange's handler, and such a state update causes this issue.

Expected behavior

Active item should move to the created new item.

Possible solution

This (partial) stack trace from the second/unexpected onActiveItemChange event firing might be helpful in debugging where the error exists.

onActiveItemChange  @ Test.tsx:166
safeInvoke  @ utils.js:101
QueryList.setActiveItem @ queryList.js:444
QueryList.setQuery  @ queryList.js:356
QueryList.componentDidUpdate  @ queryList.js:276
bocong commented 5 years ago

Some further digging:

  1. Inside componentDidUpdate, setQuery is nearly always called because of shallow comparison of props.

  2. Within setQuery, we decide whether we should update active item, and if so, setActiveItem to the first enabled item. I don't have full context around this, but it seems incorrect that we always set it to the first enabled item.

        if (shouldUpdateActiveItem) {
            this.setActiveItem(getFirstEnabledItem(filteredItems, props.itemDisabled));
        }

Source

3) We ALWAYS decide to update active item if the current activeItem is a created item

        const shouldUpdateActiveItem =
            resetActiveItem ||
            activeIndex < 0 ||
            isItemDisabled(getActiveItem(this.state.activeItem), activeIndex, props.itemDisabled);

Source

    private getActiveIndex(items = this.state.filteredItems) {
        const { activeItem } = this.state;
        if (activeItem == null || isCreateNewItem(activeItem)) {
            return -1;
        }

Source

I feel like this info should be enough to pinpoint the issue. I'm new to using blueprint, so without much of the necessary context, I don't feel 100% confident yet to suggest where exactly this fix belongs.