diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a599d22..37f98c70 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/addon/components/select-box/index.gjs b/addon/components/select-box/index.gjs index ef9a4330..113f0845 100644 --- a/addon/components/select-box/index.gjs +++ b/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 ) ) }} diff --git a/addon/components/select-box/option.gjs b/addon/components/select-box/option.gjs index b0d88795..dc79f2dd 100644 --- a/addon/components/select-box/option.gjs +++ b/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 diff --git a/addon/components/select-box/options.gjs b/addon/components/select-box/options.gjs index da623771..46431abc 100644 --- a/addon/components/select-box/options.gjs +++ b/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 > diff --git a/tests/integration/components/option/render-test.gjs b/tests/integration/components/option/render-test.gjs index 808250b7..e3eb784c 100644 --- a/tests/integration/components/option/render-test.gjs +++ b/tests/integration/components/option/render-test.gjs @@ -103,7 +103,7 @@ module('select-box/option', function (hooks) { ); - assert.dom('.select-box__option').hasAttribute('tabindex', '3'); + assert.dom('.select-box__option').doesNotHaveAttribute('tabindex'); }); test('disabled', async function (assert) { diff --git a/tests/integration/components/options/render-test.gjs b/tests/integration/components/options/render-test.gjs index 3e4417ea..366f7a2a 100644 --- a/tests/integration/components/options/render-test.gjs +++ b/tests/integration/components/options/render-test.gjs @@ -51,7 +51,13 @@ module('select-box/options', function (hooks) { ); - 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,7 +70,15 @@ module('select-box/options', function (hooks) { ); - 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) { @@ -72,12 +86,18 @@ module('select-box/options', function (hooks) { await render(); - 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) { diff --git a/tests/integration/components/select-box/click-trigger-test.gjs b/tests/integration/components/select-box/click-trigger-test.gjs index 8c35e1c9..63586579 100644 --- a/tests/integration/components/select-box/click-trigger-test.gjs +++ b/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); diff --git a/tests/integration/components/select-box/closing-test.gjs b/tests/integration/components/select-box/closing-test.gjs index e74f90ec..c9f9d668 100644 --- a/tests/integration/components/select-box/closing-test.gjs +++ b/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(); - 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(); + + 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) { +
); diff --git a/tests/integration/components/select-box/focus-test.gjs b/tests/integration/components/select-box/focus-test.gjs index ac1d9b2a..cc263f20 100644 --- a/tests/integration/components/select-box/focus-test.gjs +++ b/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(); @@ -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(); - - 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.` - ); - }); }); diff --git a/tests/integration/components/select-box/key-enter-test.gjs b/tests/integration/components/select-box/key-enter-test.gjs index 99d4e2f3..781fc78b 100644 --- a/tests/integration/components/select-box/key-enter-test.gjs +++ b/tests/integration/components/select-box/key-enter-test.gjs @@ -256,7 +256,7 @@ module('select-box (enter)', function (hooks) { }); test('pressing enter on a focusable option (listbox)', async function (assert) { - assert.expect(2); + assert.expect(1); await render(