prevent keyboard-focusable-scrollers from stealing focus

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## 16.4.1
+
+- Don't allow focus to move to listbox, when in a combbox
+  (avoids keyboard-focusable-scroller behaviour)
+- Use touch events instead of focus out on touch devices
+  to determine when to close.
+- Clarify virtual focus tests
+
 ## 16.4.0
 
 - Remove cycling options, to get closer to native behaviour

addon/components/select-box/index.gjs

@@ -134,7 +134,7 @@ export default class SelectBox extends Component {
   }
 
   get optionsTabIndex() {
-    return this.isListBox ? '0' : null;
+    return this.isListBox ? '0' : '-1';
   }
 
   get triggerTabIndex() {
@@ -228,6 +228,9 @@ export default class SelectBox extends Component {
   @action
   handleDestroyElement() {
     this.element = null;
+
+    document.removeEventListener('mouseup', this.handleMouseUp);
+    document.removeEventListener('touchstart', this.handleTouchStart);
   }
 
   @action
@@ -285,7 +288,13 @@ export default class SelectBox extends Component {
   handleMouseDown(event) {
     this.lastMouseDownElement = event.target;
 
-    document.addEventListener('mouseup', this.handleMouseUp, { once: true });
+    document.addEventListener('mouseup', this.handleMouseUp, {
+      once: true
+    });
+
+    document.addEventListener('touchstart', this.handleTouchStart, {
+      once: true
+    });
   }
 
   @action
@@ -312,12 +321,18 @@ export default class SelectBox extends Component {
     this._forgetActiveOption();
   }
 
+  @action
+  handleTouchStart(event) {
+    if (this.element.contains(event.target)) {
+      return;
+    }
+
+    this._close();
+  }
+
   @action
   handleFocusOut(event) {
-    if (
-      document.activeElement === this.interactiveElement ||
-      this.element.contains(event.relatedTarget)
-    ) {
+    if (this.element.contains(event.relatedTarget)) {
       return;
     }
 
@@ -353,6 +368,13 @@ export default class SelectBox extends Component {
     this._handleInputChar(event);
   }
 
+  @action
+  handleMouseDownOptions(event) {
+    if (this.isComboBox) {
+      event.preventDefault();
+    }
+  }
+
   @action
   handleMouseDownTrigger(event) {
     if (this.isDisabled || event.button !== 0) {
@@ -781,6 +803,7 @@ export default class SelectBox extends Component {
           onInsert=this.handleInsertOptions
           onDestroy=this.handleDestroyOptions
           onKeyDown=this.handleKeyDownOptions
+          onMouseDown=this.handleMouseDownOptions
         )
         Option=(component
           SelectBoxOption
@@ -791,7 +814,6 @@ export default class SelectBox extends Component {
           onMouseUp=this.handleMouseUpOption
           onMouseDown=this.handleMouseDownOption
           onKeyDown=this.handleKeyDownOption
-          onFocusIn=this.handleFocusInOption
         )
       )
     }}

addon/components/select-box/option.gjs

@@ -81,12 +81,10 @@ export default class SelectBoxOption extends Component {
       aria-selected="{{this.isSelected}}"
       class={{concat "select-box__option" (if @class (concat " " @class))}}
       role="option"
-      tabindex={{@tabindex}}
       {{on "mouseenter" (fn @onMouseEnter this)}}
       {{on "mousedown" @onMouseDown}}
       {{on "keydown" @onKeyDown}}
       {{on "mouseup" (fn @onMouseUp this)}}
-      {{on "focusin" (fn @onFocusIn this)}}
       {{lifecycle
         onInsert=this.handleInsertElement
         onDestroy=this.handleDestroyElement

addon/components/select-box/options.gjs

@@ -12,6 +12,7 @@ import lifecycle from '@zestia/ember-select-box/modifiers/lifecycle';
       role="listbox"
       tabindex={{@tabindex}}
       {{on "keydown" @onKeyDown}}
+      {{on "mousedown" @onMouseDown}}
       {{lifecycle onInsert=@onInsert onDestroy=@onDestroy}}
       ...attributes
     >

tests/integration/components/option/render-test.gjs

@@ -103,7 +103,7 @@ module('select-box/option', function (hooks) {
       </SelectBox>
     </template>);
 
-    assert.dom('.select-box__option').hasAttribute('tabindex', '3');
+    assert.dom('.select-box__option').doesNotHaveAttribute('tabindex');
   });
 
   test('disabled', async function (assert) {

tests/integration/components/options/render-test.gjs

@@ -51,7 +51,13 @@ module('select-box/options', function (hooks) {
       </SelectBox>
     </template>);
 
-    assert.dom('.select-box__options').hasAttribute('tabindex', '0');
+    assert
+      .dom('.select-box__options')
+      .hasAttribute(
+        'tabindex',
+        '0',
+        'the main interactive element is the listbox'
+      );
   });
 
   test('tabindex (combobox with input)', async function (assert) {
@@ -64,20 +70,34 @@ module('select-box/options', function (hooks) {
       </SelectBox>
     </template>);
 
-    assert.dom('.select-box__options').doesNotHaveAttribute('tabindex');
+    assert.dom('.select-box__options').hasAttribute(
+      'tabindex',
+      '-1',
+      `the main interactive element is the input (combobox)
+       focus should not move to the listbox, which is
+       aria controlled by the input.
+       this prevents keyboard-focusable-scrollers from stealing focus,
+       since options often overflows`
+    );
   });
 
   test('tabindex (combobox with trigger)', async function (assert) {
     assert.expect(1);
 
     await render(<template>
       <SelectBox as |sb|>
-        <sb.Input />
+        <sb.Trigger />
         <sb.Options />
       </SelectBox>
     </template>);
 
-    assert.dom('.select-box__options').doesNotHaveAttribute('tabindex');
+    assert.dom('.select-box__options').hasAttribute(
+      'tabindex',
+      '-1',
+      `the main interactive element is the trigger (combobox)
+       focus should not move to the listbox, which is
+       aria controlled by the trigger`
+    );
   });
 
   test('id', async function (assert) {

tests/integration/components/select-box/click-trigger-test.gjs

@@ -10,6 +10,11 @@ module('select-box (clicking trigger)', function (hooks) {
   test('clicking trigger toggles select box on mousedown', async function (assert) {
     assert.expect(7);
 
+    // Mouse down makes the select box feel more responsive
+    // than on click, which would open on mouseup.
+    // It's also what would happen if you clicked on a
+    // native select box.
+
     let event;
 
     const handleMouseDownTrigger = (_event) => (event = _event);

tests/integration/components/select-box/closing-test.gjs

@@ -178,21 +178,40 @@ module('select-box (closing)', function (hooks) {
   });
 
   test('mousing up outside must have moused down first', async function (assert) {
-    assert.expect(1);
+    assert.expect(2);
 
     await render(<template>
-      <button type="button"></button>
-
-      <SelectBox @open={{true}} @onlClose={{handleClose}} as |sb|>
+      <SelectBox @open={{true}} @onClose={{handleClose}} as |sb|>
         <sb.Trigger />
       </SelectBox>
+
+      <div class="outside"></div>
     </template>);
 
-    await click('button');
+    await click('.outside');
 
+    assert.dom('.select-box').hasAttribute('data-open', 'true');
     assert.verifySteps([]);
   });
 
+  test('closing due to touching outside', async function (assert) {
+    assert.expect(3);
+
+    await render(<template>
+      <SelectBox @open={{true}} @onClose={{handleClose}} as |sb|>
+        <sb.Trigger />
+      </SelectBox>
+
+      <div class="outside"></div>
+    </template>);
+
+    await triggerEvent('.select-box', 'mousedown');
+    await triggerEvent('.outside', 'touchstart');
+
+    assert.dom('.select-box').hasAttribute('data-open', 'false');
+    assert.verifySteps(['close']);
+  });
+
   test('closing due to pressing escape', async function (assert) {
     assert.expect(3);
 
@@ -219,6 +238,7 @@ module('select-box (closing)', function (hooks) {
       <SelectBox @onClose={{handleClose}} as |sb|>
         <sb.Trigger />
       </SelectBox>
+
       <div class="outside"></div>
     </template>);
 

tests/integration/components/select-box/focus-test.gjs

@@ -52,7 +52,7 @@ module('select-box (focus)', function (hooks) {
 
     assert
       .dom('.select-box__input')
-      .isFocused('the input is assumed to be the main interactive element');
+      .isFocused('the input is the main interactive element');
   });
 
   test('focus after clicking an option (trigger combobox)', async function (assert) {
@@ -73,7 +73,7 @@ module('select-box (focus)', function (hooks) {
 
     assert
       .dom('.select-box__trigger')
-      .isFocused('the trigger is assumed to be the main interactive element');
+      .isFocused('the trigger is the main interactive element');
   });
 
   test('focus after clicking an option (input & trigger combobox)', async function (assert) {
@@ -93,7 +93,7 @@ module('select-box (focus)', function (hooks) {
 
     assert
       .dom('.select-box__input')
-      .isFocused('the input is assumed to be the main interactive element');
+      .isFocused('the input is the main interactive element');
   });
 
   test('combobox with input', async function (assert) {
@@ -118,7 +118,8 @@ module('select-box (focus)', function (hooks) {
 
     assert.dom('.select-box__input').isFocused(`
       clicking an option does not steal focus from the input,
-      otherwise the select box would close
+      otherwise the select box would close (due to focus out
+      of the main interactive element)
     `);
 
     assert.true(event.defaultPrevented);
@@ -419,11 +420,21 @@ module('select-box (focus)', function (hooks) {
       );
 
     assert.dom('.select-box__trigger').isFocused(`
-      focus must be maintained on the interactive element,
-      otherwise the select box will not be receptive to user actions
+      focus must be maintained on the interactive element, regardless
+      of where is clicked inside the select box, otherwise the
+      select box will not be receptive to user actions, which is the
+      same as how any other native component would behave/
     `);
 
-    assert.false(event.defaultPrevented);
+    // Note
+    // we use tabindex="-1" on the listbox to prevent
+    // keyboard-focusable-scrollers from stealing focus, but
+    // this has a down side of actually allowing the listbox
+    // to be focusable on click, so we must prevent that.
+    // https://issues.chromium.org/issues/376718258
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1930662
+
+    assert.true(event.defaultPrevented);
   });
 
   test('focus moving to somewhere else inside the combo box', async function (assert) {
@@ -449,7 +460,9 @@ module('select-box (focus)', function (hooks) {
       'true',
       `focus is free to move about inside an open select box
        it will not be receptive to user actions, but the interactive
-       elements inside it will, and that's ok.`
+       elements inside it will, and that's ok. unusual, but at least
+       allows flexibility for the developer if designing a highly
+       custom select box`
     );
 
     assert.dom('.inside').isFocused();
@@ -484,7 +497,7 @@ module('select-box (focus)', function (hooks) {
     await render(<template>
       <SelectBox as |sb|>
         <sb.Options>
-          <sb.Option tabindex="0" />
+          <sb.Option @value="A" tabindex="0" />
         </sb.Options>
       </SelectBox>
     </template>);
@@ -493,8 +506,9 @@ module('select-box (focus)', function (hooks) {
 
     assert.dom('.select-box__option').hasAttribute(
       'aria-current',
-      'true',
-      `focusing an option activates it. usually this is not required
+      'false',
+      `focusing an option does not activate it.
+       options are not directly focusable
        because focus is managed virtually using aria-activedescendant`
     );
   });
@@ -552,32 +566,4 @@ module('select-box (focus)', function (hooks) {
        to the element using the keyboard`
     );
   });
-
-  test('changing tabs', async function (assert) {
-    assert.expect(4);
-
-    await render(<template>
-      <SelectBox as |sb|>
-        <sb.Trigger />
-        <sb.Options />
-      </SelectBox>
-    </template>);
-
-    await click('.select-box__trigger');
-
-    assert.dom('.select-box__trigger').isFocused();
-    assert.dom('.select-box').hasAttribute('data-open', 'true');
-
-    await triggerEvent('.select-box__trigger', 'focusout');
-
-    assert.dom('.select-box__trigger').isFocused();
-    assert.dom('.select-box').hasAttribute(
-      'data-open',
-      'true',
-      `focusout fires when changing tabs (before document.hidden becomes true)
-       which causes the select box to close. but when returning to that tab,
-       focus will still be on the interactive element, therefore really it
-       should not close when changing tabs.`
-    );
-  });
 });