Do not render Select listbox children while it's closed

hypothesis · Nov 4, 2024 · 4c4c10c · 4c4c10c
1 parent 51505f0
commit 4c4c10c
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 16 deletions.
diff --git a/src/components/input/Select.tsx b/src/components/input/Select.tsx
@@ -545,14 +545,6 @@ function SelectMain<T>({
     selector: '[role="option"]:not([aria-disabled="true"])',
   });
 
-  useLayoutEffect(() => {
-    // Focus toggle button after closing listbox, only if previously focused
-    // element was inside the listbox itself
-    if (!listboxOpen && listboxRef.current!.contains(document.activeElement)) {
-      buttonRef.current!.focus();
-    }
-  }, [buttonRef, listboxOpen]);
-
   return (
     <div
       className={classnames(
@@ -640,7 +632,7 @@ function SelectMain<T>({
           popover={listboxAsPopover ? 'auto' : undefined}
           onScroll={onListboxScroll}
         >
-          {children}
+          {listboxOpen && children}
         </ul>
       </SelectContext.Provider>
     </div>

diff --git a/src/components/input/test/Select-test.js b/src/components/input/test/Select-test.js
@@ -101,6 +101,12 @@ describe('Select', () => {
   const isListboxClosed = wrapper =>
     getListbox(wrapper).prop('data-listbox-open') === false;
 
+  const openListbox = wrapper => {
+    if (isListboxClosed(wrapper)) {
+      toggleListbox(wrapper);
+    }
+  };
+
   const listboxDidDropUp = wrapper => {
     const { top: listboxTop } = getListbox(wrapper)
       .getDOMNode()
@@ -112,10 +118,14 @@ describe('Select', () => {
     return listboxTop < buttonTop;
   };
 
-  const clickOption = (wrapper, id) =>
+  const clickOption = (wrapper, id) => {
+    openListbox(wrapper);
     wrapper.find(`[data-testid="option-${id}"]`).simulate('click');
+  };
 
   function clickOptionCheckbox(wrapper, id) {
+    openListbox(wrapper);
+
     const checkbox = wrapper
       .find(`[data-testid="option-${id}"]`)
       .closest('[role="option"]')
@@ -130,14 +140,19 @@ describe('Select', () => {
     checkbox.simulate('change');
   }
 
-  const pressKeyInOption = (wrapper, id, key) =>
+  const pressKeyInOption = (wrapper, id, key) => {
+    openListbox(wrapper);
+
     wrapper
       .find(`[data-testid="option-${id}"]`)
       .closest('[role="option"]')
       .getDOMNode()
       .dispatchEvent(new KeyboardEvent('keydown', { key }));
+  };
 
   function pressKeyInOptionCheckbox(wrapper, id, key) {
+    openListbox(wrapper);
+
     const checkbox = wrapper
       .find(`[data-testid="option-${id}"]`)
       .closest('[role="option"]')
@@ -152,10 +167,12 @@ describe('Select', () => {
     checkbox.getDOMNode().dispatchEvent(new KeyboardEvent('keydown', { key }));
   }
 
-  const isOptionSelected = (wrapper, id) =>
-    wrapper
+  const isOptionSelected = (wrapper, id) => {
+    openListbox(wrapper);
+    return wrapper
       .find(`[data-testid="option-${id}"]`)
       .exists('[data-testid="selected-option"]');
+  };
 
   it('changes selected value when an option is clicked', () => {
     const onChange = sinon.stub();
@@ -177,6 +194,7 @@ describe('Select', () => {
     const clickDisabledOption = () =>
       wrapper.find(`[data-testid="option-4"]`).simulate('click');
 
+    toggleListbox(wrapper);
     clickDisabledOption();
     assert.notCalled(onChange);
   });
@@ -222,6 +240,8 @@ describe('Select', () => {
         .find(`[data-testid="option-${id}"]`)
         .exists('[data-testid="disabled-option"]');
 
+    openListbox(wrapper);
+
     assert.isFalse(isOptionDisabled(1));
     assert.isFalse(isOptionDisabled(2));
     assert.isFalse(isOptionDisabled(3));
@@ -293,6 +313,10 @@ describe('Select', () => {
 
   it('restores focus to toggle button after closing listbox', () => {
     const wrapper = createComponent();
+    const toggleButtonDOMNode = getToggleButton(wrapper).getDOMNode();
+
+    // Focus toggle button before opening listbox
+    toggleButtonDOMNode.focus();
     toggleListbox(wrapper);
 
     // Focus listbox option before closing listbox
@@ -301,10 +325,14 @@ describe('Select', () => {
       .getDOMNode()
       .closest('[role="option"]')
       .focus();
+
     toggleListbox(wrapper);
     wrapper.update();
 
-    assert.equal(document.activeElement, getToggleButton(wrapper).getDOMNode());
+    // After closing listbox, the focus should have returned to the toggle
+    // button, which was the last focused element before all children were
+    // removed
+    assert.equal(document.activeElement, toggleButtonDOMNode);
   });
 
   it('displays listbox when ArrowDown is pressed on toggle', () => {
@@ -582,7 +610,6 @@ describe('Select', () => {
         { Component: MultiSelect },
       );
 
-      toggleListbox(wrapper);
       clickOptionCheckbox(wrapper, 2);
 
       // When a not-yet-selected item is clicked, it will be selected
@@ -606,7 +633,6 @@ describe('Select', () => {
         { Component: MultiSelect },
       );
 
-      toggleListbox(wrapper);
       clickOptionCheckbox(wrapper, 3);
 
       // When an already selected item is clicked, it will be de-selected
@@ -669,6 +695,7 @@ describe('Select', () => {
         { value: [] },
         { Component: MultiSelect },
       );
+      openListbox(wrapper);
 
       // Spy on checkbox focus
       const checkbox = wrapper
@@ -688,6 +715,7 @@ describe('Select', () => {
         { value: [] },
         { Component: MultiSelect },
       );
+      openListbox(wrapper);
 
       const option = wrapper
         .find(`[data-testid="option-${optionId}"]`)