Skip to content

Commit

Permalink
Split Select dual popover/listbox element in two
Browse files Browse the repository at this point in the history
  • Loading branch information
@acelaya
acelaya committed Nov 19, 2024
1 parent 485c02e commit cdfe295
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 55 deletions.
64 changes: 36 additions & 28 deletions src/components/input/Select.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,18 +280,18 @@ type ListboxCSSProps =
* as a popover, as that makes it render in the top layer, making it impossible
* to position it relative to the toggle button via regular CSS.
*/
function useListboxPositioning(
function usePopoverPositioning(
buttonRef: RefObject<HTMLElement | undefined>,
listboxRef: RefObject<HTMLElement | null>,
popoverRef: RefObject<HTMLElement | null>,
listboxOpen: boolean,
asPopover: boolean,
alignListboxToRight: boolean,
) {
const adjustListboxPositioning = useCallback(() => {
const listboxEl = listboxRef.current;
const popoverEl = popoverRef.current;
const buttonEl = buttonRef.current;

if (!buttonEl || !listboxEl || !listboxOpen) {
if (!buttonEl || !popoverEl || !listboxOpen) {
return () => {};
}

Expand All @@ -303,9 +303,9 @@ function useListboxPositioning(
const setListboxCSSProps = (
props: Partial<Record<ListboxCSSProps, string>>,
) => {
Object.assign(listboxEl.style, props);
Object.assign(popoverEl.style, props);
const keys = Object.keys(props) as ListboxCSSProps[];
return () => keys.map(prop => (listboxEl.style[prop] = ''));
return () => keys.map(prop => (popoverEl.style[prop] = ''));
};

const viewportHeight = window.innerHeight;
Expand All @@ -318,7 +318,7 @@ function useListboxPositioning(
} = buttonEl.getBoundingClientRect();
const buttonDistanceToBottom = viewportHeight - buttonBottom;
const { height: listboxHeight, width: listboxWidth } =
listboxEl.getBoundingClientRect();
popoverEl.getBoundingClientRect();

// The listbox should drop up only if there's not enough space below to
// fit it, and also, there's more absolute space above than below
Expand Down Expand Up @@ -373,7 +373,7 @@ function useListboxPositioning(
: `calc(${absBodyTop + buttonDistanceToTop + buttonHeight}px + ${LISTBOX_TOGGLE_GAP})`,
left: `${Math.max(LISTBOX_VIEWPORT_HORIZONTAL_GAP, left)}px`,
});
}, [asPopover, buttonRef, listboxOpen, listboxRef, alignListboxToRight]);
}, [asPopover, buttonRef, listboxOpen, popoverRef, alignListboxToRight]);

useLayoutEffect(() => {
const cleanup = adjustListboxPositioning();
Expand Down Expand Up @@ -498,13 +498,13 @@ function SelectMain<T>({
listboxAsPopover = HTMLElement.prototype.hasOwnProperty('popover'),
}: SelectMainProps<T>) {
const wrapperRef = useRef<HTMLDivElement | null>(null);
const listboxRef = useRef<HTMLUListElement | null>(null);
const popoverRef = useRef<HTMLDivElement | null>(null);
const [listboxOpen, setListboxOpen] = useState(false);
const toggleListbox = useCallback(
(open: boolean) => {
setListboxOpen(open);
if (listboxAsPopover) {
listboxRef.current?.togglePopover(open);
popoverRef.current?.togglePopover(open);
}
},
[listboxAsPopover],
Expand All @@ -514,9 +514,9 @@ function SelectMain<T>({
const buttonRef = useSyncedRef(elementRef);
const defaultButtonId = useId();

useListboxPositioning(
usePopoverPositioning(
buttonRef,
listboxRef,
popoverRef,
listboxOpen,
listboxAsPopover,
alignListbox === 'right',
Expand All @@ -538,7 +538,7 @@ function SelectMain<T>({
useKeyPress(['Escape'], closeListbox);

// Vertical arrow key for options in the listbox
useArrowKeyNavigation(listboxRef, {
useArrowKeyNavigation(popoverRef, {
horizontal: false,
loop: false,
autofocus: true,
Expand Down Expand Up @@ -604,13 +604,17 @@ function SelectMain<T>({
listboxOverflow,
}}
>
<ul
<div
className={classnames(
'absolute z-5 max-h-80 overflow-y-auto overflow-x-hidden',
// We don't want the listbox to ever render outside the viewport,
// and we give it a 16px gap
'max-w-[calc(100%-16px)]',
'rounded border bg-white shadow hover:shadow-md focus-within:shadow-md',
listboxAsPopover && [
// We don't want the listbox to ever render outside the viewport,
// and we give it a 16px gap
'max-w-[calc(100%-16px)]',
// Overwrite [popover] default styles
'p-0 m-0',
],
!listboxAsPopover && {
// Hiding instead of unmounting to
// * Ensure screen readers detect button as a listbox handler
Expand All @@ -621,22 +625,26 @@ function SelectMain<T>({
},
listboxClasses,
)}
role="listbox"
ref={listboxRef}
id={listboxId}
aria-multiselectable={multiple}
aria-labelledby={buttonId ?? defaultButtonId}
aria-orientation="vertical"
data-testid="select-listbox"
data-listbox-open={listboxOpen}
ref={popoverRef}
// nb. Use `undefined` rather than `false` because Preact doesn't
// handle boolean values correctly for this attribute (it will set
// `popover="false"` instead of removing the attribute).
popover={listboxAsPopover ? 'auto' : undefined}
onScroll={onListboxScroll}
data-testid="select-popover"
>
{listboxOpen && children}
</ul>
<ul
role="listbox"
id={listboxId}
aria-multiselectable={multiple}
aria-labelledby={buttonId ?? defaultButtonId}
aria-orientation="vertical"
data-testid="select-listbox"
data-listbox-open={listboxOpen}
onScroll={onListboxScroll}
>
{listboxOpen && children}
</ul>
</div>
</SelectContext.Provider>
</div>
);
Expand Down
56 changes: 29 additions & 27 deletions src/components/input/test/Select-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ describe('Select', () => {

const getListbox = wrapper => wrapper.find('[data-testid="select-listbox"]');

const getPopover = wrapper => wrapper.find('[data-testid="select-popover"]');

const isListboxClosed = wrapper =>
getListbox(wrapper).prop('data-listbox-open') === false;

Expand All @@ -108,14 +110,14 @@ describe('Select', () => {
};

const listboxDidDropUp = wrapper => {
const { top: listboxTop } = getListbox(wrapper)
const { top: popoverTop } = getPopover(wrapper)
.getDOMNode()
.getBoundingClientRect();
const { top: buttonTop } = getToggleButton(wrapper)
.getDOMNode()
.getBoundingClientRect();

return listboxTop < buttonTop;
return popoverTop < buttonTop;
};

const getOption = (wrapper, id) =>
Expand Down Expand Up @@ -411,7 +413,7 @@ describe('Select', () => {
let resolve;
const promise = new Promise(res => (resolve = res));

getListbox(wrapper).getDOMNode().addEventListener('toggle', resolve);
getPopover(wrapper).getDOMNode().addEventListener('toggle', resolve);
toggleListbox(wrapper);

// This test will time out if the toggle event is not dispatched
Expand All @@ -425,28 +427,28 @@ describe('Select', () => {
// Inferring listboxAsPopover based on browser support
{
listboxAsPopover: undefined,
getListboxLeft: wrapper => {
const leftStyle = getListbox(wrapper).getDOMNode().style.left;
getPopoverLeft: wrapper => {
const leftStyle = getPopover(wrapper).getDOMNode().style.left;
// Remove `px` unit indicator
return Number(leftStyle.replace('px', ''));
},
},
// Explicitly enabling listboxAsPopover
{
listboxAsPopover: true,
getListboxLeft: wrapper => {
const leftStyle = getListbox(wrapper).getDOMNode().style.left;
getPopoverLeft: wrapper => {
const leftStyle = getPopover(wrapper).getDOMNode().style.left;
// Remove `px` unit indicator
return Number(leftStyle.replace('px', ''));
},
},
// Explicitly disabling listboxAsPopover
{
listboxAsPopover: false,
getListboxLeft: wrapper =>
getListbox(wrapper).getDOMNode().getBoundingClientRect().left,
getPopoverLeft: wrapper =>
getPopover(wrapper).getDOMNode().getBoundingClientRect().left,
},
].forEach(({ listboxAsPopover, getListboxLeft }) => {
].forEach(({ listboxAsPopover, getPopoverLeft }) => {
it('aligns listbox to the right if `alignListbox="right"` is provided', async () => {
const wrapper = createComponent({
listboxAsPopover,
Expand All @@ -461,19 +463,17 @@ describe('Select', () => {
const { left: buttonLeft } = getToggleButton(wrapper)
.getDOMNode()
.getBoundingClientRect();
const listboxLeft = getListboxLeft(wrapper);
const popoverLeft = getPopoverLeft(wrapper);

assert.isTrue(listboxLeft < buttonLeft);
assert.isAtMost(popoverLeft, buttonLeft);
});
});
});

context('when listbox does not fit in available space', () => {
async function getOpenListbox(wrapper) {
async function openListbox(wrapper) {
toggleListbox(wrapper);
await waitFor(() => !isListboxClosed(wrapper));

return getListbox(wrapper);
}

it('never renders a listbox bigger than the viewport', async () => {
Expand All @@ -485,12 +485,13 @@ describe('Select', () => {
},
);

const listbox = await getOpenListbox(wrapper);
const { width: listboxWidth } = listbox
await openListbox(wrapper);

const { width: popoverWidth } = getPopover(wrapper)
.getDOMNode()
.getBoundingClientRect();

assert.isTrue(listboxWidth < window.innerWidth);
assert.isBelow(popoverWidth, window.innerWidth);
});

[
Expand Down Expand Up @@ -579,24 +580,25 @@ describe('Select', () => {
},
);

const listbox = await getOpenListbox(wrapper);
const listboxDOMNode = listbox.getDOMNode();
const listboxStyleLeft = listboxDOMNode.style.left;
const listboxLeft = Number(listboxStyleLeft.replace('px', ''));
const listboxRight =
listboxDOMNode.getBoundingClientRect().width + listboxLeft;
await openListbox(wrapper);

const popoverDOMNode = getPopover(wrapper).getDOMNode();
const popoverStyleLeft = popoverDOMNode.style.left;
const popoverLeft = Number(popoverStyleLeft.replace('px', ''));
const popoverRight =
popoverDOMNode.getBoundingClientRect().width + popoverLeft;

const expectedCoordinates = getExpectedCoordinates(
wrapper,
listboxDOMNode,
popoverDOMNode,
);

assert.equal(
listboxLeft.toFixed(0),
popoverLeft.toFixed(0),
expectedCoordinates.left.toFixed(0),
);
assert.equal(
listboxRight.toFixed(0),
popoverRight.toFixed(0),
expectedCoordinates.right.toFixed(0),
);
});
Expand Down

0 comments on commit cdfe295

Please sign in to comment.