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

fix(Dropdown): interrupts IME input on controlled search #18478

Merged
merged 13 commits into from Jun 29, 2021

Conversation

YuanboXue-Amber
Copy link
Contributor

@YuanboXue-Amber YuanboXue-Amber commented Jun 8, 2021

Pull request checklist

Description of changes

When Dropdown is controlled, it does not accept IME input properly. IME input were interrupted
beforefix

After fix:
afterfix

Reason for this fix:
IME doesn't play well with controlled input.
When using controlled input, having input's value as state,
<input value={value} onChange={(e) => setValue(e.target.value)} - has no problem with IME
but something like <input value={value} onChange={(e) => setValue(e.target.value + '-')} - breaks IME
codesandbox

During investigation, I saw this issue in downshift, where it mentions:

This looks like the setState call replaces the value of the text field after typing has finished, essentially overwriting what's in the field...

Downshift js moved onInputValueChange to be called before its setState to fix this issue(https://github.com/downshift-js/downshift/blob/ce6c4e1a434bb483cf1f0386937cb9aa313220ab/src/downshift.js#L318)

So I moved our onSearchQuery to be called onInputValueChange, instead of onStateChange and it resolves our issue with IME.

Focus areas to test

(optional)

@size-auditor
Copy link

size-auditor bot commented Jun 8, 2021

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-northstar-Dropdown 216.743 kB 216.836 kB ExceedsBaseline     93 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 70af664e45b84cee236c56e0ce272f32234d898e (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 8, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 818 800 5000
BaseButton mount 926 896 5000
Breadcrumb mount 2693 2600 1000
ButtonNext mount 525 523 5000
Checkbox mount 1520 1561 5000
CheckboxBase mount 1316 1307 5000
ChoiceGroup mount 4746 4776 5000
ComboBox mount 964 966 1000
CommandBar mount 10301 10460 1000
ContextualMenu mount 6284 6367 1000
DefaultButton mount 1113 1124 5000
DetailsRow mount 3740 3759 5000
DetailsRowFast mount 3796 3818 5000
DetailsRowNoStyles mount 3626 3532 5000
Dialog mount 2108 2119 1000
DocumentCardTitle mount 141 153 1000
Dropdown mount 3537 3337 5000
FocusTrapZone mount 1847 1824 5000
FocusZone mount 1827 1819 5000
IconButton mount 1748 1884 5000
Label mount 345 357 5000
Layer mount 1832 1800 5000
Link mount 468 479 5000
MakeStyles mount 1859 1860 50000
MenuButton mount 1434 1493 5000
MessageBar mount 2059 2111 5000
Nav mount 3273 3227 1000
OverflowSet mount 1107 1037 5000
Panel mount 2073 2096 1000
Persona mount 825 856 1000
Pivot mount 1532 1421 1000
PrimaryButton mount 1279 1242 5000
Rating mount 7783 7589 5000
SearchBox mount 1346 1312 5000
Shimmer mount 2561 2582 5000
Slider mount 1978 1952 5000
SpinButton mount 5080 4915 5000
Spinner mount 445 410 5000
SplitButton mount 3146 3193 5000
Stack mount 507 512 5000
StackWithIntrinsicChildren mount 1576 1563 5000
StackWithTextChildren mount 4544 4610 5000
SwatchColorPicker mount 10243 10261 5000
Tabs mount 1425 1413 1000
TagPicker mount 2393 2431 5000
TeachingBubble mount 11877 11920 5000
Text mount 444 418 5000
TextField mount 1388 1407 5000
ThemeProvider mount 1187 1182 5000
ThemeProvider virtual-rerender 620 607 5000
ThemeProviderNext mount 7211 7230 5000
Toggle mount 786 803 5000
buttonNative mount 120 110 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
FlexMinimalPerf.default 303 278 1.09:1
GridMinimalPerf.default 353 323 1.09:1
BoxMinimalPerf.default 369 343 1.08:1
ChatWithPopoverPerf.default 370 344 1.08:1
ListWith60ListItems.default 643 600 1.07:1
ButtonMinimalPerf.default 171 161 1.06:1
CardMinimalPerf.default 569 541 1.05:1
MenuMinimalPerf.default 870 829 1.05:1
HeaderMinimalPerf.default 366 352 1.04:1
ListNestedPerf.default 547 527 1.04:1
RadioGroupMinimalPerf.default 461 442 1.04:1
HeaderSlotsPerf.default 771 750 1.03:1
TooltipMinimalPerf.default 997 972 1.03:1
DropdownMinimalPerf.default 3125 3060 1.02:1
ImageMinimalPerf.default 378 369 1.02:1
LayoutMinimalPerf.default 357 350 1.02:1
ListMinimalPerf.default 517 506 1.02:1
PortalMinimalPerf.default 187 183 1.02:1
IconMinimalPerf.default 635 624 1.02:1
TableMinimalPerf.default 404 395 1.02:1
CustomToolbarPrototype.default 3861 3798 1.02:1
ButtonOverridesMissPerf.default 1694 1684 1.01:1
DatepickerMinimalPerf.default 5595 5523 1.01:1
FormMinimalPerf.default 398 394 1.01:1
MenuButtonMinimalPerf.default 1590 1576 1.01:1
PopupMinimalPerf.default 586 580 1.01:1
ProviderMergeThemesPerf.default 1684 1667 1.01:1
SplitButtonMinimalPerf.default 3760 3730 1.01:1
StatusMinimalPerf.default 675 670 1.01:1
ToolbarMinimalPerf.default 946 937 1.01:1
ButtonSlotsPerf.default 545 545 1:1
CheckboxMinimalPerf.default 2732 2737 1:1
DividerMinimalPerf.default 352 353 1:1
EmbedMinimalPerf.default 4118 4107 1:1
ListCommonPerf.default 609 606 1:1
LoaderMinimalPerf.default 703 705 1:1
ProviderMinimalPerf.default 976 976 1:1
ReactionMinimalPerf.default 367 367 1:1
RefMinimalPerf.default 238 237 1:1
AttachmentMinimalPerf.default 156 157 0.99:1
AttachmentSlotsPerf.default 1084 1096 0.99:1
CarouselMinimalPerf.default 455 458 0.99:1
ChatDuplicateMessagesPerf.default 289 293 0.99:1
DropdownManyItemsPerf.default 673 680 0.99:1
ItemLayoutMinimalPerf.default 1226 1238 0.99:1
LabelMinimalPerf.default 380 385 0.99:1
SegmentMinimalPerf.default 341 344 0.99:1
AccordionMinimalPerf.default 158 162 0.98:1
ChatMinimalPerf.default 652 664 0.98:1
DialogMinimalPerf.default 728 741 0.98:1
InputMinimalPerf.default 1248 1274 0.98:1
SkeletonMinimalPerf.default 356 365 0.98:1
TreeMinimalPerf.default 783 802 0.98:1
TreeWith60ListItems.default 173 177 0.98:1
SliderMinimalPerf.default 1555 1606 0.97:1
TableManyItemsPerf.default 1863 1921 0.97:1
TextAreaMinimalPerf.default 480 493 0.97:1
VideoMinimalPerf.default 634 652 0.97:1
AlertMinimalPerf.default 267 278 0.96:1
TextMinimalPerf.default 340 353 0.96:1
RosterPerf.default 1111 1180 0.94:1
AnimationMinimalPerf.default 415 445 0.93:1
AvatarMinimalPerf.default 191 207 0.92:1

@YuanboXue-Amber YuanboXue-Amber changed the title fix(Dropdown): cannot use IME input on controlled search fix(Dropdown): interrupts IME input on controlled search Jun 8, 2021
const handleInputValueChange = (inputValue: string) => {
if (multiple) {
setStateAndInvokeHandler(['onSearchQueryChange'], null, {
searchQuery: '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work. searchQuery is used to filter list of items, see getFilteredValues(). It should be cleared only on Esc and when an item is selected (getSelectedItemAsString() in Downshift.stateChangeTypes.clickItem which you removed).

To repro, use keyboard to filter items in Search Multiple example.

It scares me that no test is failing :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Miro for catching this. I made a change and tested for both selecting item and escape key.
Added the filtering case for multiple search in unit test L1413.

Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is breaking Multiple Search, see the comment.

Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, would appreciate one more pair of eyes - @ling1726?

Copy link
Member

@ling1726 ling1726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the variants of the dropdown in the docsite and can't identify any obvious issues. I confess that even though the code changes look simple Downshift is still a big mystery to me

@YuanboXue-Amber
Copy link
Contributor Author

I tested the variants of the dropdown in the docsite and can't identify any obvious issues. I confess that even though the code changes look simple Downshift is still a big mystery to me

Thanks I feel the same. I'll leave this open until it gets verified in TMP

@YuanboXue-Amber
Copy link
Contributor Author

YuanboXue-Amber commented Jun 10, 2021

Reason for commit 59ddffa

Here's a minimum repro of a usage in TMP

import React from 'react';
import { Dropdown } from '@fluentui/react-northstar';

const inputItems = [
  'Robert Tolbert',
  'Wanda Howard',
  'Tim Deboer',
  'Amanda Brady',
  'Ashley McCarthy',
  'Cameron Evans',
  'Carlos Slattery',
  'Carole Poland',
  'Robin Counts',
];

const PPContainer = () => {
  const [items, setItems] = React.useState(inputItems);

  const [searchQuery, setSearchQuery] = React.useState('');
  const onSearchChange = (_: any, { searchQuery }) => {
    setSearchQuery(searchQuery);
    if (searchQuery.length === 0) {
      setItems([]);
    }
  };

  const [selectedUsers, setSelectedUsers] = React.useState([]);
  const onSelectionChange = (_: any, { selectedUsers }) => {
    setItems([]);
    setSelectedUsers(selectedUsers);
  };

  return (
    <PPRenderer
      items={items}
      searchQuery={searchQuery}
      onSearchChange={onSearchChange}
      onSelectionChange={onSelectionChange}
      selectedUsers={selectedUsers}
    />
  );
};

const PPRenderer = ({ items, searchQuery, onSearchChange, onSelectionChange, selectedUsers }) => {
  const onSelectedChange = (_, { value }) => {
    if (items.length > 0) {
      onSelectionChange(_, {
        selectedUsers: value,
      });
    }
  };

  return (
    <Dropdown
      search
      multiple
      searchQuery={searchQuery}
      onSearchQueryChange={onSearchChange}
      onChange={onSelectedChange}
      value={selectedUsers}
      items={items}
      placeholder="Start typing a name"
      noResultsMessage="We couldn't find any matches."
      getA11ySelectionMessage={{
        onAdd: item => `${item} has been selected.`,
      }}
    />
  );
};

export default PPContainer;

Before this commit, in this example, when an item is selected:

  1. Downshift onInputValueChange invoke Dropdown onSearchQueryChange callback
  2. onSearchChange is called in PPContainer and sets items to empty array
  3. Downshift onStateChange invoke Dropdown onChange
  4. onSelectedChange is called in PPRender, but since items is empty, it does not set any item to be selected

After this commit, in this example, when an item is selected, step 1 and 2 do not happen. So when step 4 is executed, items array is not empty, so the item can be selected normally

@miroslavstastny miroslavstastny self-requested a review June 10, 2021 18:03
@YuanboXue-Amber
Copy link
Contributor Author

Confirmed by people team this is a good fix

@YuanboXue-Amber YuanboXue-Amber enabled auto-merge (squash) June 29, 2021 07:47
@YuanboXue-Amber YuanboXue-Amber merged commit 44683f4 into microsoft:master Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown component's Search Selection in controlled mode has issue with typing in language using certain IME
5 participants