Do not render Select listbox children while it's closed

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

Hide file tree

Showing 2 changed files with 16 additions and 25 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
@@ -161,19 +161,24 @@ describe('Select', () => {
     const onChange = sinon.stub();
     const wrapper = createComponent({ onChange });
 
+    toggleListbox(wrapper);
     clickOption(wrapper, 3);
     assert.calledWith(onChange.lastCall, defaultItems[2]);
 
+    toggleListbox(wrapper);
     clickOption(wrapper, 5);
     assert.calledWith(onChange.lastCall, defaultItems[4]);
 
+    toggleListbox(wrapper);
     clickOption(wrapper, 1);
     assert.calledWith(onChange.lastCall, defaultItems[0]);
   });
 
   it('does not change selected value when a disabled option is clicked', () => {
     const onChange = sinon.stub();
     const wrapper = createComponent({ onChange });
+    toggleListbox(wrapper);
+
     const clickDisabledOption = () =>
       wrapper.find(`[data-testid="option-4"]`).simulate('click');
 
@@ -185,6 +190,7 @@ describe('Select', () => {
     it(`changes selected value when ${key} is pressed in option`, () => {
       const onChange = sinon.stub();
       const wrapper = createComponent({ onChange });
+      toggleListbox(wrapper);
 
       pressKeyInOption(wrapper, 3, key);
       assert.calledWith(onChange.lastCall, defaultItems[2]);
@@ -199,6 +205,7 @@ describe('Select', () => {
     it(`does not change selected value when ${key} is pressed in a disabled option`, () => {
       const onChange = sinon.stub();
       const wrapper = createComponent({ onChange });
+      toggleListbox(wrapper);
 
       pressKeyInOption(wrapper, 4, key); // Option 4 is disabled
       assert.notCalled(onChange);
@@ -207,6 +214,7 @@ describe('Select', () => {
 
   it('marks the right item as selected', () => {
     const wrapper = createComponent({ value: defaultItems[2] });
+    toggleListbox(wrapper);
 
     assert.isFalse(isOptionSelected(wrapper, 1));
     assert.isFalse(isOptionSelected(wrapper, 2));
@@ -217,6 +225,8 @@ describe('Select', () => {
 
   it('marks the right item as disabled', () => {
     const wrapper = createComponent();
+    toggleListbox(wrapper);
+
     const isOptionDisabled = id =>
       wrapper
         .find(`[data-testid="option-${id}"]`)
@@ -291,22 +301,6 @@ describe('Select', () => {
     }
   });
 
-  it('restores focus to toggle button after closing listbox', () => {
-    const wrapper = createComponent();
-    toggleListbox(wrapper);
-
-    // Focus listbox option before closing listbox
-    wrapper
-      .find('[data-testid="option-3"]')
-      .getDOMNode()
-      .closest('[role="option"]')
-      .focus();
-    toggleListbox(wrapper);
-    wrapper.update();
-
-    assert.equal(document.activeElement, getToggleButton(wrapper).getDOMNode());
-  });
-
   it('displays listbox when ArrowDown is pressed on toggle', () => {
     const wrapper = createComponent();
 
@@ -640,6 +634,8 @@ describe('Select', () => {
     ].forEach(({ value, isResetSelected }) => {
       it('marks reset option as selected when value is empty', () => {
         const wrapper = createComponent({ value }, { Component: MultiSelect });
+        toggleListbox(wrapper);
+
         assert.equal(isOptionSelected(wrapper, 'reset'), isResetSelected);
       });
     });
@@ -651,6 +647,7 @@ describe('Select', () => {
           { onChange, value: [] },
           { Component: MultiSelect },
         );
+        toggleListbox(wrapper);
 
         pressKeyInOptionCheckbox(wrapper, 3, key);
         assert.notCalled(onChange);
@@ -669,6 +666,7 @@ describe('Select', () => {
         { value: [] },
         { Component: MultiSelect },
       );
+      toggleListbox(wrapper);
 
       // Spy on checkbox focus
       const checkbox = wrapper
@@ -688,6 +686,7 @@ describe('Select', () => {
         { value: [] },
         { Component: MultiSelect },
       );
+      toggleListbox(wrapper);
 
       const option = wrapper
         .find(`[data-testid="option-${optionId}"]`)