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.

Sign up for GitHub

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

Jump to bottom

fix: Respecting user-provided ids in ComboBox options #25867

Merged
merged 2 commits into from
Dec 1, 2022
Merged

fix: Respecting user-provided ids in ComboBox options #25867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
664e5a0
fix: Respecting user-provided ids in ComboBox options.
Dec 1, 2022
865c7ea
Adding change file.
Dec 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@fluentui-react-a5c36bc6-3f4e-4de1-91a8-8d2407288c7c.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "fix: Respecting user-provided ids in ComboBox options.",
"packageName": "@fluentui/react",
"email": "makotom@microsoft.com",
"dependentChangeType": "patch"
}
22 changes: 22 additions & 0 deletions packages/react/src/components/ComboBox/ComboBox.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,28 @@ describe('ComboBox', () => {
expect(getByRole('combobox').getAttribute('value')).toEqual('');
});

it('Respects a user provided id for an option', () => {
const options: IComboBoxOption[] = [
{ key: 0, text: 'zero' },
{ key: 1, text: 'one', id: 'one' },
{ key: 2, text: 'two' },
];

const { getByRole, getAllByRole } = render(<ComboBox options={options} />);
const combobox = getByRole('combobox');
// open combobox
userEvent.click(combobox);

const optionArray = getAllByRole('option');
expect(optionArray.length).toEqual(3);

const regex = new RegExp(/ComboBox[0-9]+-list[0-9]+/);
expect(regex.test(optionArray[0].getAttribute('id')!)).toEqual(true);
expect(regex.test(optionArray[1].getAttribute('id')!)).toEqual(false);
expect(optionArray[1].getAttribute('id')).toEqual('one');
expect(regex.test(optionArray[2].getAttribute('id')!)).toEqual(true);
});

it('Applies correct attributes to the selected option', () => {
const { getByRole, getAllByRole } = render(<ComboBox options={DEFAULT_OPTIONS} defaultSelectedKey="2" />);

Expand Down
22 changes: 12 additions & 10 deletions packages/react/src/components/ComboBox/ComboBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ class ComboBoxInternal extends React.Component<IComboBoxInternalProps, IComboBox
onRenderList,
onRenderItem,
onRenderOption,
options: currentOptions.map((item, index) => ({ ...item, index: index })),
options: currentOptions.map((item, index) => ({ ...item, index })),
onDismiss: this._onDismiss,
},
this._onRenderContainer,
Expand Down Expand Up @@ -1493,7 +1493,7 @@ class ComboBoxInternal extends React.Component<IComboBoxInternalProps, IComboBox
const getOptionComponent = () => {
return !this.props.multiSelect ? (
<CommandButton
id={id + '-list' + item.index}
id={item.id ?? id + '-list' + item.index}
key={item.key}
data-index={item.index}
styles={optionStyles}
Expand All @@ -1520,7 +1520,7 @@ class ComboBoxInternal extends React.Component<IComboBoxInternalProps, IComboBox
</CommandButton>
) : (
<Checkbox
id={id + '-list' + item.index}
id={item.id ?? id + '-list' + item.index}
ariaLabel={item.ariaLabel}
key={item.key}
styles={optionStyles}
Expand Down Expand Up @@ -1848,7 +1848,7 @@ class ComboBoxInternal extends React.Component<IComboBoxInternalProps, IComboBox
this.props.hoisted.setSuggestedDisplayValue(suggestedDisplayValue);
this.setState({
currentPendingValue: normalizeToString(currentPendingValue),
currentPendingValueValidIndex: currentPendingValueValidIndex,
currentPendingValueValidIndex,
currentPendingValueValidIndexOnHover: HoverStatus.default,
});
}
Expand Down Expand Up @@ -1951,9 +1951,7 @@ class ComboBoxInternal extends React.Component<IComboBoxInternalProps, IComboBox
*/
private _setOpenStateAndFocusOnClose(isOpen: boolean, focusInputAfterClose: boolean): void {
this._focusInputAfterClose = focusInputAfterClose;
this.setState({
isOpen: isOpen,
});
this.setState({ isOpen });
}

/**
Expand Down Expand Up @@ -2359,11 +2357,15 @@ class ComboBoxInternal extends React.Component<IComboBoxInternalProps, IComboBox
* null otherwise
*/
private _getAriaActiveDescendantValue(): string | undefined {
const { selectedIndices } = this.props.hoisted;
const { currentOptions, selectedIndices } = this.props.hoisted;
const { isOpen, currentPendingValueValidIndex } = this.state;
let descendantText = isOpen && selectedIndices?.length ? this._id + '-list' + selectedIndices[0] : undefined;
const options = currentOptions.map((item, index) => ({ ...item, index }));
Copy link
Contributor

Choose a reason for hiding this comment

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

The list is probably never long enough for this difference to matter that much and I could see object spread getting transpiled to Object.assign anyway but using Object.assign is faster than spread.

https://jsbench.me/q5lb5lumdv

Suggested change
const options = currentOptions.map((item, index) => ({ ...item, index }));
const options = currentOptions.map((item, index) => (Object.assign({}, item, { index }));

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make a separate PR to address this (it happens in more places within ComboBox).

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I get hit with an eslint rule error if I do that, so maybe we prefer this approach in v8

image

Copy link
Contributor

@spmonahan spmonahan Dec 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The docs for this plugin state that

Introduced in ES2018, object spread is a declarative alternative which may perform better than the more dynamic, imperative Object.assign.

which, along with readability, seem to be the arguments in favor of enforcing spread. In the benchmark I linked spread is ~80% slower in Edge and ~85% slower in Chrome, while Object.assign is ~6% slower in Firefox. May perform better indeed 😉. What I'm getting from this is that the performance is implementation-dependent so it could change tomorrow, but that even in the case where spread is faster it's not that much faster (and it's within the margin of error, +/- ~7%). Compared with the cases where spread is slower, it's much slower and well outside the margin of error. So where we think performance is critical we should prefer Object.assign.

So is performance critical here? That's not so clear to me. I'm of the opinion that we should strive for Fluent to be as fast as (reasonably) possible, leaving as many resources to our users as possible so in that sense all performance is always critical. Yet, it's not clear to me that this measurably affects the perf of ComboBox?

I think this change is simple enough that it's worth benchmarking spread vs object.assign in ComboBox itself to see what shakes out.

What do you think about running a ComboBox bench for this case?

ETA: I should note that in testing Firefox is in the neighborhood of 875 opts/s at the fastest while Chrome and Edge are around 1000 opts/s meaning Firefox is more consistent but slower overall.

let descendantText =
isOpen && selectedIndices?.length
? options[selectedIndices[0]].id ?? this._id + '-list' + selectedIndices[0]
: undefined;
if (isOpen && this._hasFocus() && currentPendingValueValidIndex !== -1) {
descendantText = this._id + '-list' + currentPendingValueValidIndex;
descendantText = options[currentPendingValueValidIndex].id ?? this._id + '-list' + currentPendingValueValidIndex;
}
return descendantText;
}
Expand Down