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: Respecting user-provided ids in ComboBox options #25867

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

khmakoto
Copy link
Member

@khmakoto khmakoto commented Dec 1, 2022

Previous Behavior

Passing id as part of a ComboBoxOption would not be respected, and it would instead be overridden by the automatically generated one.

New Behavior

Passing id as part of a ComboBoxOption is now respected over the automatically generated one. We made sure to propagate the correct value to aria-activedescendant when this happens. A test has been added to ensure this behavior is not broken in the future.

Related Issue(s)

@khmakoto khmakoto added Component: Combobox Fluent UI react (v8) Issues about @fluentui/react (v8) labels Dec 1, 2022
@khmakoto khmakoto requested review from a team as code owners December 1, 2022 21:28
@khmakoto khmakoto self-assigned this Dec 1, 2022
@github-actions github-actions bot added this to the October Project Cycle Q4 2022 milestone Dec 1, 2022
@khmakoto khmakoto enabled auto-merge (squash) December 1, 2022 21:42
@size-auditor
Copy link

size-auditor bot commented Dec 1, 2022

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-ComboBox 237.03 kB 237.248 kB ExceedsBaseline     218 bytes
office-ui-fabric-react fluentui-react-TimePicker 225.971 kB 226.189 kB ExceedsBaseline     218 bytes

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

Baseline commit: 6213380549afac6ea1e224f04fec4a875a7f6aeb (build)

@fabricteam
Copy link
Collaborator

📊 Bundle size report

🤖 This report was generated against 6213380549afac6ea1e224f04fec4a875a7f6aeb

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 1, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 865c7ea:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

🕵 fluentuiv8 Open the Visual Regressions report to inspect the 3 screenshots

✅ There was 0 screenshots added, 0 screenshots removed, 1036 screenshots unchanged, 0 screenshots with different dimensions and 3 screenshots with visible difference.

unknown 3 screenshots
Image Name Diff(in Pixels) Image Type
TagPicker.Root.chromium.png 103 Changed
TagPicker.Selected - RTL.chromium.png 103 Changed
TagPicker.Selected.chromium.png 103 Changed

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 1194 1197 5000
Breadcrumb mount 2767 2774 1000
Checkbox mount 2660 2650 5000
CheckboxBase mount 2365 2374 5000
ChoiceGroup mount 4264 4261 5000
ComboBox mount 1180 1178 1000
CommandBar mount 9232 9237 1000
ContextualMenu mount 10090 10080 1000
DefaultButton mount 1344 1354 5000
DetailsRow mount 3388 3335 5000
DetailsRowFast mount 3399 3366 5000
DetailsRowNoStyles mount 3258 3243 5000
Dialog mount 2960 2952 1000
DocumentCardTitle mount 566 568 1000
Dropdown mount 3152 3145 5000
FocusTrapZone mount 1977 1947 5000
FocusZone mount 1959 1886 5000
GroupedList mount 1832 2053 2
GroupedList virtual-rerender 1111 1090 2
GroupedList virtual-rerender-with-unmount 1575 1573 2
GroupedListV2 mount 572 574 2
GroupedListV2 virtual-rerender 541 553 2
GroupedListV2 virtual-rerender-with-unmount 564 553 2
IconButton mount 1808 1797 5000
Label mount 739 749 5000
Layer mount 4180 4186 5000
Link mount 851 864 5000
MenuButton mount 1600 1606 5000
MessageBar mount 2317 2304 5000
Nav mount 3047 3094 1000
OverflowSet mount 1415 1413 5000
Panel mount 2505 2465 1000
Persona mount 1230 1296 1000
Pivot mount 1515 1519 1000
PrimaryButton mount 1494 1486 5000
Rating mount 6922 6904 5000
SearchBox mount 1497 1499 5000
Shimmer mount 2943 2856 5000
Slider mount 2102 2087 5000
SpinButton mount 4296 4250 5000
Spinner mount 832 821 5000
SplitButton mount 2832 2828 5000
Stack mount 860 866 5000
StackWithIntrinsicChildren mount 2331 2340 5000
StackWithTextChildren mount 4992 5033 5000
SwatchColorPicker mount 9517 9422 5000
TagPicker mount 2334 2326 5000
TeachingBubble mount 75004 74485 5000
Text mount 822 816 5000
TextField mount 1550 1572 5000
ThemeProvider mount 1444 1456 5000
ThemeProvider virtual-rerender 1141 1137 5000
ThemeProvider virtual-rerender-with-unmount 2012 2010 5000
Toggle mount 1120 1151 5000
buttonNative mount 523 546 5000

@khmakoto khmakoto merged commit 3c75461 into microsoft:master Dec 1, 2022
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

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.

@khmakoto khmakoto deleted the comboboxId branch December 1, 2022 22:33
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Dec 2, 2022
* master:
  fix: Pressed and Hover states for toolbar buttons (microsoft#25835)
  feat: add large size for toolbar (microsoft#25830)
  applying package updates
  adding perf test for Persona (microsoft#25863)
  fix: Fixing Slider's programmatic focus (microsoft#25869)
  chore(v0 docs): Add storybook stories that reference docsite examples for 1JS VR tool migration (microsoft#25663)
  fix: Respecting user-provided ids in ComboBox options (microsoft#25867)
  refactor(scripts): more domain boundaries encapsulation (microsoft#25851)
  docs: add documentation about how to migrate V0 createSvgIcon (microsoft#25828)
  support cross story linking and create example in Menu story (microsoft#25850)
Hotell pushed a commit to Hotell/fluentui that referenced this pull request Feb 9, 2023
* fix: Respecting user-provided ids in ComboBox options.

* Adding change file.

Co-authored-by: Humberto Makoto Morimoto Burgos <makotom@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Combobox Fluent UI react (v8) Issues about @fluentui/react (v8)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: ComboBox component option id cannot be customized
4 participants