fix(react-avatar): Fix trapFocus in AvatarGroup's PopoverSurface and PopoverSurface's role override#23823
Conversation
… fix trapFocus when Popover is triggered.
|
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 c57422c:
|
📊 Bundle size report
Unchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: ea986100088f2572e5311a6b572d0dbcf6327d98 (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1245 | 1306 | 5000 | |
| Button | mount | 841 | 885 | 5000 | |
| FluentProvider | mount | 1694 | 1673 | 5000 | |
| FluentProviderWithTheme | mount | 533 | 570 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 517 | 507 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 626 | 559 | 10 | |
| MakeStyles | mount | 1847 | 1838 | 50000 | |
| SpinButton | mount | 2516 | 2409 | 5000 |
| 'aria-label': 'Overflow', | ||
| children: overflowChildren, | ||
| role: 'list', | ||
| tabIndex: 0, |
There was a problem hiding this comment.
my one comment would be I don't think we need the list to be focusable anymore, since it's now wrapped within the popup surface.
Since it looks like the popover surface is the one that now has overflow: scroll, I think that's the only node that would need to be focusable for keyboard scroll
There was a problem hiding this comment.
That would bring back the same problem of trapFocus not working correctly since the children of PopoverSurface is what will be focused, not the PopoverSurface. I could add the overflow: scroll to the PopoverContent so that it can be focused and allow the user to scroll. What do you think about that change?
…PopoverSurface's role override (microsoft#23823) * fix: Remove override of role=list in AvatarGroup's PopoverSurface and fix trapFocus when Popover is triggered. * change files * updating scroll to be on overflowContent
Current Behavior
listis not compatible witharia-modal.New Behavior
overflowSurfaceslot is added which will only contain the PopoverSurface. The purpose of this slot is to allow for further customization rather than having an internal wrapper. This slot will haveoverflowContentwhich now works correctly withtrapFocus.PopoverSurface's role anymore and follows correct aria rules.Related Issue(s)
Fixes #23822