[Dropdown] - OnChange is called if notifyOnReselect is false end selectedKey is used

[droulmegus]

We are currently using the Dropdown component in a controlled implentation and we saw that when the user reselects the same option onChange was invoked, even with notifyOnReselect explicitly set to false.

If we use an uncontrolled dropdown, in the same scenario, onChange isn't invoked.

We wanted to know if it was an intended behavior.

Environment Information

• Package version(s): 7.97.1
• Browser and OS versions: Chrome/Windows

Please provide a reproduction of the bug in a codepen:

https://codesandbox.io/s/dropdown-reselect-c8fq5

Actual behavior:

The onChange is called when reselecting an option if selectedKey is set and notifyOnReselectis false.

Expected behavior:

The onChange is not called when reselecting an option if selectedKey is set and notifyOnReselectis false.

Priorities and help requested:

Requested priority: Low

Thanks

[xugao]

@rhublard - here is the description for the notifyOnReselect

Optional preference to have onChanged still be called when an already selected item is clicked in single select mode. Default to false

notifyOnReselect is not designed for not firing onChange if it's set to false.

[fildahui]

Optional preference to have onChanged still be called when an already selected item is clicked in single select mode. Default to false

Looking at the props info the onChanged prop is deprecated so notifyOnReselect prop should be too, shouldn't it? If they're strictly related I don't see why deprecating one and not the other.

But I was looking at the code... when an item is clicked (even if it's already selected) and the selectedKey prop is undefined there's a check for multiSelect and notifyOnReselect to be false: if both are and the selectedIndex has not changed the function returns. Otherwise the execution goes on and eventually the onChange callback is fired.

Think of this scenario I might have:

DropDown is not multiselect notifyOnReselect is set to true I click on an already selected item:

Let's look at the code:

// If this is a controlled component then no state change should take place.
if (selectedKey !== undefined || selectedKeys !== undefined) {
   this._onChange(event, options, index, checked, multiSelect);
   return;
}

if (!multiSelect && !notifyOnReselect && index === selectedIndices[0]) {

This check will prevent the onChange to be called when the dropdown is not multiSelect, the index has not changed and the notifyOnReselect is set to false. In my scenario the execution will continue because notifyOnReselect is true

   return;
} else if (multiSelect) {

In my scenario multiSelect is false so the execution will go to the last else

      newIndexes = selectedIndices ? this._copyArray(selectedIndices) : [];
      if (checked) {
        const position = newIndexes.indexOf(index);
        if (position > -1) {
          // unchecked the current one
          newIndexes.splice(position, 1);
        }
      } else {
        // add the new selected index into the existing one
        newIndexes.push(index);
      }
} else {
      // Set the selected option if this is an uncontrolled component
      newIndexes = [index];
}

event.persist();
// Call onChange after state is updated
this.setState(
  {
     selectedIndices: newIndexes
  },
  () => {
        this._onChange(event, options, index, checked, multiSelect);
  }
);

So the onChange will be called and the reason is because notifyOnReselect was set to true (in my scenario).

You said "notifyOnReselect is not designed for not firing onChange if it's set to false" but as I showed you it does prevent (or not) the onChange to be called but only when the component is uncontrolled.

This behavior should be the same in a controlled scenario too. @xugao

[msft-github-bot]

This issue has been automatically marked as stale because it has marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. Thank you for your contributions to Fabric React!

[droulmegus]

@fildahui gave better details on our current issue. I hope it explains better than I did

[xugao]

@rhublard -

This behavior should be the same in a controlled scenario too

I think the prop was designed for uncontrolled example because user doesn't know about the selected key (since it's handled internally). Therefore they have no way to achieve notifyOnReselect in uncontrolled case without the prop.

but in controlled example, you can simply check the changed value against the selected value and make a decision on what to do there.

does that make sense? we should certainly update the documentation to be more accurate

[msft-github-bot]

This issue has been automatically marked as stale because it has marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. Thank you for your contributions to Fabric React!