fix(pickers) docs & styles for non-layered dropdowns for mobile a11y #18495
Conversation
Asset size changes
Baseline commit: 3a08bcacbd059fbc1d528607639576c4512433c7 (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 847 | 859 | 5000 | |
| BaseButton | mount | 985 | 987 | 5000 | |
| Breadcrumb | mount | 2743 | 2713 | 1000 | |
| ButtonNext | mount | 549 | 583 | 5000 | |
| Checkbox | mount | 1675 | 1692 | 5000 | |
| CheckboxBase | mount | 1459 | 1487 | 5000 | |
| ChoiceGroup | mount | 5284 | 5266 | 5000 | |
| ComboBox | mount | 1056 | 1047 | 1000 | |
| CommandBar | mount | 10302 | 10461 | 1000 | |
| ContextualMenu | mount | 6319 | 6377 | 1000 | |
| DefaultButton | mount | 1203 | 1226 | 5000 | |
| DetailsRow | mount | 3882 | 3952 | 5000 | |
| DetailsRowFast | mount | 4110 | 4021 | 5000 | |
| DetailsRowNoStyles | mount | 3658 | 3746 | 5000 | |
| Dialog | mount | 2237 | 2196 | 1000 | |
| DocumentCardTitle | mount | 152 | 148 | 1000 | |
| Dropdown | mount | 3485 | 3470 | 5000 | |
| FocusTrapZone | mount | 1843 | 1917 | 5000 | |
| FocusZone | mount | 1780 | 1916 | 5000 | |
| IconButton | mount | 1885 | 1870 | 5000 | |
| Label | mount | 353 | 361 | 5000 | |
| Layer | mount | 1870 | 1900 | 5000 | |
| Link | mount | 495 | 494 | 5000 | |
| MakeStyles | mount | 1819 | 1764 | 50000 | |
| MenuButton | mount | 1571 | 1547 | 5000 | |
| MessageBar | mount | 2060 | 2077 | 5000 | |
| Nav | mount | 3457 | 3453 | 1000 | |
| OverflowSet | mount | 1071 | 1064 | 5000 | |
| Panel | mount | 1423 | 2103 | 1000 | |
| Persona | mount | 868 | 864 | 1000 | |
| Pivot | mount | 1500 | 1498 | 1000 | |
| PrimaryButton | mount | 1338 | 1358 | 5000 | |
| Rating | mount | 8498 | 8234 | 5000 | |
| SearchBox | mount | 1506 | 1470 | 5000 | |
| Shimmer | mount | 2830 | 2846 | 5000 | |
| Slider | mount | 2114 | 2149 | 5000 | |
| SpinButton | mount | 5430 | 5441 | 5000 | |
| Spinner | mount | 419 | 431 | 5000 | |
| SplitButton | mount | 3325 | 3410 | 5000 | |
| Stack | mount | 534 | 561 | 5000 | |
| StackWithIntrinsicChildren | mount | 1645 | 1726 | 5000 | |
| StackWithTextChildren | mount | 5129 | 5023 | 5000 | |
| SwatchColorPicker | mount | 10857 | 11005 | 5000 | |
| Tabs | mount | 1639 | 1478 | 1000 | |
| TagPicker | mount | 2640 | 2591 | 5000 | |
| TeachingBubble | mount | 12127 | 12320 | 5000 | |
| Text | mount | 482 | 465 | 5000 | |
| TextField | mount | 1509 | 1496 | 5000 | |
| ThemeProvider | mount | 1246 | 1237 | 5000 | |
| ThemeProvider | virtual-rerender | 615 | 625 | 5000 | |
| ThemeProviderNext | mount | 6960 | 6854 | 5000 | |
| Toggle | mount | 849 | 844 | 5000 | |
| buttonNative | mount | 112 | 115 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| DropdownManyItemsPerf.default | 817 | 735 | 1.11:1 |
| ListNestedPerf.default | 651 | 589 | 1.11:1 |
| ChatWithPopoverPerf.default | 416 | 379 | 1.1:1 |
| RefMinimalPerf.default | 261 | 240 | 1.09:1 |
| SkeletonMinimalPerf.default | 415 | 381 | 1.09:1 |
| FlexMinimalPerf.default | 329 | 305 | 1.08:1 |
| PortalMinimalPerf.default | 195 | 180 | 1.08:1 |
| TextMinimalPerf.default | 388 | 358 | 1.08:1 |
| GridMinimalPerf.default | 382 | 357 | 1.07:1 |
| ListCommonPerf.default | 716 | 667 | 1.07:1 |
| FormMinimalPerf.default | 487 | 458 | 1.06:1 |
| RadioGroupMinimalPerf.default | 534 | 505 | 1.06:1 |
| SegmentMinimalPerf.default | 395 | 372 | 1.06:1 |
| ButtonSlotsPerf.default | 614 | 585 | 1.05:1 |
| LayoutMinimalPerf.default | 418 | 398 | 1.05:1 |
| ListWith60ListItems.default | 696 | 665 | 1.05:1 |
| AnimationMinimalPerf.default | 451 | 434 | 1.04:1 |
| CheckboxMinimalPerf.default | 3001 | 2890 | 1.04:1 |
| InputMinimalPerf.default | 1333 | 1287 | 1.04:1 |
| TableMinimalPerf.default | 455 | 439 | 1.04:1 |
| AlertMinimalPerf.default | 297 | 288 | 1.03:1 |
| DividerMinimalPerf.default | 405 | 392 | 1.03:1 |
| HeaderMinimalPerf.default | 406 | 396 | 1.03:1 |
| ProviderMergeThemesPerf.default | 1722 | 1672 | 1.03:1 |
| ReactionMinimalPerf.default | 433 | 420 | 1.03:1 |
| SplitButtonMinimalPerf.default | 4096 | 3976 | 1.03:1 |
| CardMinimalPerf.default | 638 | 626 | 1.02:1 |
| CarouselMinimalPerf.default | 500 | 491 | 1.02:1 |
| DatepickerMinimalPerf.default | 5749 | 5612 | 1.02:1 |
| DropdownMinimalPerf.default | 3221 | 3143 | 1.02:1 |
| EmbedMinimalPerf.default | 4414 | 4345 | 1.02:1 |
| HeaderSlotsPerf.default | 856 | 836 | 1.02:1 |
| ListMinimalPerf.default | 547 | 534 | 1.02:1 |
| StatusMinimalPerf.default | 744 | 732 | 1.02:1 |
| TableManyItemsPerf.default | 2146 | 2108 | 1.02:1 |
| ToolbarMinimalPerf.default | 1042 | 1020 | 1.02:1 |
| ButtonMinimalPerf.default | 186 | 185 | 1.01:1 |
| ChatDuplicateMessagesPerf.default | 313 | 309 | 1.01:1 |
| ChatMinimalPerf.default | 668 | 663 | 1.01:1 |
| ImageMinimalPerf.default | 402 | 398 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1706 | 1691 | 1.01:1 |
| TextAreaMinimalPerf.default | 567 | 559 | 1.01:1 |
| TooltipMinimalPerf.default | 1089 | 1075 | 1.01:1 |
| TreeMinimalPerf.default | 881 | 869 | 1.01:1 |
| VideoMinimalPerf.default | 686 | 676 | 1.01:1 |
| DialogMinimalPerf.default | 803 | 803 | 1:1 |
| LabelMinimalPerf.default | 419 | 419 | 1:1 |
| CustomToolbarPrototype.default | 3967 | 3971 | 1:1 |
| ButtonOverridesMissPerf.default | 1794 | 1821 | 0.99:1 |
| MenuMinimalPerf.default | 913 | 925 | 0.99:1 |
| RosterPerf.default | 1293 | 1309 | 0.99:1 |
| PopupMinimalPerf.default | 606 | 612 | 0.99:1 |
| TreeWith60ListItems.default | 188 | 189 | 0.99:1 |
| AccordionMinimalPerf.default | 156 | 159 | 0.98:1 |
| LoaderMinimalPerf.default | 738 | 751 | 0.98:1 |
| IconMinimalPerf.default | 664 | 678 | 0.98:1 |
| AttachmentSlotsPerf.default | 1199 | 1242 | 0.97:1 |
| ItemLayoutMinimalPerf.default | 1341 | 1381 | 0.97:1 |
| ProviderMinimalPerf.default | 1078 | 1107 | 0.97:1 |
| SliderMinimalPerf.default | 1648 | 1712 | 0.96:1 |
| BoxMinimalPerf.default | 366 | 395 | 0.93:1 |
| AvatarMinimalPerf.default | 207 | 224 | 0.92:1 |
| AttachmentMinimalPerf.default | 166 | 183 | 0.91:1 |
There was a problem hiding this comment.
Some minor comments but this is probably a good change overall. My only larger concern is that the change to z-index for existing Callouts with doNotLayer may catch consumers off guard--I'm a bit rusty on issues related to z-index and layering, so not sure offhand what the implications or practical risk would be.
packages/react-examples/src/react/PeoplePicker/docs/PeoplePickerBestPractices.md
Outdated
Show resolved
Hide resolved
| PeoplePicker dropdowns render in their own layer by default to ensure they are not clipped by containers with `overflow: hidden` or `overflow: scroll`. This causes extra difficulty for people who use touch-based screen readers, so we recommend rendering the PeoplePicker inline unless it is in an overflow container. To do so, set the following property on the PeoplePicker: | ||
|
|
||
| ```js | ||
| pickerCalloutProps={{ doNotLayer: true }} | ||
| ``` |
There was a problem hiding this comment.
Super nitpicky/opinion-based but I think it looks a bit funny to have the prop on its own line (up to you though).
| PeoplePicker dropdowns render in their own layer by default to ensure they are not clipped by containers with `overflow: hidden` or `overflow: scroll`. This causes extra difficulty for people who use touch-based screen readers, so we recommend rendering the PeoplePicker inline unless it is in an overflow container. To do so, set the following property on the PeoplePicker: | |
| ```js | |
| pickerCalloutProps={{ doNotLayer: true }} | |
| ``` | |
| PeoplePicker dropdowns render in their own layer by default to ensure they are not clipped by containers with `overflow: hidden` or `overflow: scroll`. This causes extra difficulty for people who use touch-based screen readers, so we recommend rendering the PeoplePicker inline unless it is in an overflow container. To render inline, set `pickerCalloutProps={{ doNotLayer: true }}` on the PeoplePicker. |
There was a problem hiding this comment.
This is the only comment I didn't change -- I'm leaning towards keeping it on a new line, just because I'm really hoping people will actually use it, so I'd like to make it visually called out & easy to copy/paste. I'm not super tied to it though -- in vNext, I think I'd actually like it to be inline by default, with a prop to render it in a new layer 😄
packages/react-examples/src/react/Pickers/TagPicker.Inline.Example.tsx
Outdated
Show resolved
Hide resolved
| getTextFromItem={getTextFromItem} | ||
| pickerSuggestionsProps={pickerSuggestionsProps} | ||
| itemLimit={4} | ||
| pickerCalloutProps={{ doNotLayer: true }} |
There was a problem hiding this comment.
To make it more explicit, can you add a comment calling out that this is the important part?
There was a problem hiding this comment.
Done! Let me know if you like the wording.
packages/react-examples/src/react/Pickers/docs/PickersBestPractices.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Elizabeth Craig <ecraig12345@gmail.com>
… into picker-inline-dropdown
| @@ -110,6 +110,32 @@ describe('BasePicker', () => { | |||
| disabledTests: ['component-has-root-ref', 'component-handles-ref', 'has-top-level-file'], | |||
| }); | |||
|
|
|||
| it('renders inline callout', () => { | |||
| jest.useFakeTimers(); | |||
There was a problem hiding this comment.
Can you add cleanup for this in afterEach to avoid potential interference with other tests?
if ((setTimeout as any).mock) {
jest.useRealTimers();
}
| @@ -0,0 +1,7 @@ | |||
| { | |||
| "type": "patch", | |||
There was a problem hiding this comment.
Oh also this should be minor since you're adding a prop
| "type": "patch", | |
| "type": "minor", |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
Pull request checklist
$ yarn changeDescription of changes
This PR should make it possible to render the picker dropdowns as an inline callout instead of as a separate Layer, since inline has huge benefits for mobile screen reader a11y. It adds these changes: