Skip to content

Conversation

@makopch-ms
Copy link
Contributor

Description of changes

When you add non-multiselect items to a combobox, aria-selected will ALWAYS be false since isOptionChecked has a 'this.props.multiselect' in it.

CommandButtons should use isSelected like they do in v7, since they cannot be "Checked" like a checkbox can.

This is consistent with how we set the 'checked' prop as well.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 4, 2021

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 e10b18a:

Sandbox Source
Fluent UI React Starter Configuration

@size-auditor
Copy link

size-auditor bot commented Aug 4, 2021

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: c55f2388771a2e169c5f8cf0a1f1441e6ef28d15 (build)

@fabricteam
Copy link
Collaborator

📊 Bundle size report

🤖 This report was generated against c55f2388771a2e169c5f8cf0a1f1441e6ef28d15

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 847 817 5000
BaseButton mount 981 964 5000
Breadcrumb mount 2692 2599 1000
ButtonNext mount 470 468 5000
Checkbox mount 1599 1613 5000
CheckboxBase mount 1373 1440 5000
ChoiceGroup mount 5002 5262 5000
ComboBox mount 982 1076 1000
CommandBar mount 10173 10275 1000
ContextualMenu mount 6238 6295 1000
DefaultButton mount 1190 1233 5000
DetailsRow mount 3808 3855 5000
DetailsRowFast mount 4027 3935 5000
DetailsRowNoStyles mount 3705 3707 5000
Dialog mount 2188 2195 1000
DocumentCardTitle mount 152 152 1000
Dropdown mount 3443 3439 5000
FluentProviderNext mount 7005 7215 5000
FocusTrapZone mount 1806 1815 5000
FocusZone mount 1864 1837 5000
IconButton mount 1917 1884 5000
Label mount 366 367 5000
Layer mount 1901 1961 5000
Link mount 504 470 5000
MakeStyles mount 1833 1841 50000
MenuButton mount 1624 1552 5000
MessageBar mount 2032 1975 5000
Nav mount 3399 3454 1000
OverflowSet mount 1114 1077 5000
Panel mount 2135 2199 1000
Persona mount 869 872 1000
Pivot mount 1451 1429 1000
PrimaryButton mount 1359 1335 5000
Rating mount 8316 8165 5000
SearchBox mount 1426 1420 5000
Shimmer mount 2686 2816 5000
Slider mount 2061 1964 5000
SpinButton mount 5245 5143 5000
Spinner mount 422 435 5000
SplitButton mount 3280 3394 5000
Stack mount 525 524 5000
StackWithIntrinsicChildren mount 1671 1690 5000
StackWithTextChildren mount 4874 4976 5000
SwatchColorPicker mount 10764 10811 5000
Tabs mount 1464 1465 1000
TagPicker mount 2745 2727 5000
TeachingBubble mount 12082 11954 5000
Text mount 455 454 5000
TextField mount 1452 1445 5000
ThemeProvider mount 1220 1210 5000
ThemeProvider virtual-rerender 591 620 5000
Toggle mount 847 847 5000
buttonNative mount 114 110 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AvatarMinimalPerf.default 222 199 1.12:1
FlexMinimalPerf.default 321 295 1.09:1
AccordionMinimalPerf.default 170 157 1.08:1
AttachmentMinimalPerf.default 171 158 1.08:1
ListCommonPerf.default 710 659 1.08:1
GridMinimalPerf.default 382 362 1.06:1
ProviderMergeThemesPerf.default 1750 1656 1.06:1
ReactionMinimalPerf.default 442 416 1.06:1
RefMinimalPerf.default 243 230 1.06:1
InputMinimalPerf.default 1355 1294 1.05:1
ListNestedPerf.default 609 578 1.05:1
RadioGroupMinimalPerf.default 512 486 1.05:1
TreeMinimalPerf.default 883 844 1.05:1
AttachmentSlotsPerf.default 1149 1107 1.04:1
ListMinimalPerf.default 550 531 1.04:1
AlertMinimalPerf.default 290 281 1.03:1
ChatWithPopoverPerf.default 391 381 1.03:1
FormMinimalPerf.default 470 455 1.03:1
LoaderMinimalPerf.default 726 706 1.03:1
SegmentMinimalPerf.default 393 382 1.03:1
SliderMinimalPerf.default 1671 1625 1.03:1
TableMinimalPerf.default 448 435 1.03:1
TreeWith60ListItems.default 191 186 1.03:1
AnimationMinimalPerf.default 433 423 1.02:1
CardMinimalPerf.default 590 576 1.02:1
CheckboxMinimalPerf.default 2831 2789 1.02:1
DropdownManyItemsPerf.default 747 733 1.02:1
SkeletonMinimalPerf.default 397 388 1.02:1
SplitButtonMinimalPerf.default 4085 4015 1.02:1
IconMinimalPerf.default 651 636 1.02:1
TextMinimalPerf.default 376 369 1.02:1
ButtonMinimalPerf.default 193 191 1.01:1
ChatMinimalPerf.default 722 712 1.01:1
DatepickerMinimalPerf.default 5636 5578 1.01:1
LabelMinimalPerf.default 413 409 1.01:1
ListWith60ListItems.default 676 671 1.01:1
MenuMinimalPerf.default 882 871 1.01:1
PopupMinimalPerf.default 606 598 1.01:1
PortalMinimalPerf.default 185 183 1.01:1
StatusMinimalPerf.default 736 730 1.01:1
CustomToolbarPrototype.default 3986 3946 1.01:1
ToolbarMinimalPerf.default 1028 1019 1.01:1
TooltipMinimalPerf.default 1106 1090 1.01:1
VideoMinimalPerf.default 681 677 1.01:1
CarouselMinimalPerf.default 496 497 1:1
DialogMinimalPerf.default 773 772 1:1
EmbedMinimalPerf.default 4317 4321 1:1
HeaderSlotsPerf.default 837 835 1:1
ItemLayoutMinimalPerf.default 1288 1294 1:1
MenuButtonMinimalPerf.default 1735 1727 1:1
TableManyItemsPerf.default 2023 2031 1:1
TextAreaMinimalPerf.default 553 552 1:1
DropdownMinimalPerf.default 3134 3157 0.99:1
LayoutMinimalPerf.default 385 388 0.99:1
ButtonSlotsPerf.default 568 578 0.98:1
HeaderMinimalPerf.default 390 396 0.98:1
BoxMinimalPerf.default 363 374 0.97:1
ButtonOverridesMissPerf.default 1738 1787 0.97:1
ProviderMinimalPerf.default 1062 1090 0.97:1
ImageMinimalPerf.default 405 424 0.96:1
RosterPerf.default 1248 1301 0.96:1
ChatDuplicateMessagesPerf.default 298 313 0.95:1
DividerMinimalPerf.default 389 413 0.94:1

Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

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

Can you make a codepen showing the issue you're trying to fix? The change to use isChecked was actually intentional (PR #16331) to fix another accessibility bug, and is supposed to be ported to v7 too (#16329) but I never got around to finishing it.

@makopch-ms
Copy link
Contributor Author

Can you make a codepen showing the issue you're trying to fix? The change to use isChecked was actually intentional (PR #16331) to fix another accessibility bug, and is supposed to be ported to v7 too (#16329) but I never got around to finishing it.

@ecraig12345 I see. Since this may not be the ideal solution given your changes, I opened issue: #19264 where the discussion can continue further.

There are codepens available there.

@msft-fluent-ui-bot
Copy link
Collaborator

Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code.

@msft-fluent-ui-bot msft-fluent-ui-bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Jan 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Combobox Resolution: Soft Close Soft closing inactive issues over a certain period

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants