wip: enable keyboard scrolling in S2 Popover when content overflows#10091
wip: enable keyboard scrolling in S2 Popover when content overflows#10091yihuiliao wants to merge 7 commits into
Conversation
| default: 'elevated', | ||
| isArrowShown: 'none' | ||
| }, | ||
| outlineStyle: 'solid', |
There was a problem hiding this comment.
I'm not sure if there was reason we were using outline over borders. However, since overflow: auto was moved to the popover and not inner div, the outline was appearing on top of the scrollbar. Using border ensures that it appears underneath.
*ignore the squished image, i didn't want it take up too much space but you can see where the outline is
There was a problem hiding this comment.
for context, i previously changed the popover to use border instead of outline bc the outline was now rendering on top of the scrollbar since i move overflow: auto to the outer div. but this caused the popover to shift bc borders participate in the box models whereas outline doesn't. we could move to box shadows but we already have
boxShadow: {
default: 'elevated',
isArrowShown: 'none'
},
and i'd rather not mess with it just for this.
are we okay with the outline appearing on top of the scrollbar? or can anyone think of a better solution?
i can't clip the scrollbar to the border radius bc that would require overflow: hidden' but we need overflow: auto` so that the outer element gets focused.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| isExiting: 'none' | ||
| } | ||
| }, | ||
| overflow: 'auto' |
There was a problem hiding this comment.
right soooo the only issue with overflow: auto is now it clips the arrow...
it would be nice to go this route though bc browsers will focus the scrollable container and then users can keyboard up/down so not much code (but then no arrow)
we might have to shift focus to the inner div instead to preserve the popover's arrow but not sure how that would play around with submenu's for example
There was a problem hiding this comment.
allow me to cook for a bit...
|
Build successful! 🎉 |
Closes #5303
The original issue was for V3 but the same issue occurred in S2. We focus the Popover but it wasn't a scrollable region (only the inner div was), so if you hit any of the up/down arrow keys, the content wouldn't scroll. The only way to scroll content via keyboard was if there was a focusable item inside. We move
overflow: 'autoto the Popover so it is both focused + a scrollable region.My only concern is that this could potentially affect a lot of other components which rely on Popover. I've tested other components but might have missed something
Btw, I still need to run chromatic so this PR is not quite done yet...
✅ Pull Request Checklist:
📝 Test Instructions:
Test ComboBox, Menu, Picker, Popover in storybook when the content scrolls. Make sure styles haven't changed, no double scrollbars, scroll into view works
Compare S2 Storybook > Popover > Help Center. Previously, if the content scrolled and you pressed the down arrow key, nothing would happen. You would first need to tab into the region. Now, you should be able to press the down arrow key and the content should scroll.
🧢 Your Project: