From f37a0ad9f722adc6035d73d27198a4e26cfe4ba0 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Thu, 2 Jan 2025 00:29:26 -0600 Subject: [PATCH] Decouple opening from focusing --- README.md | 8 ++--- .../controllers/hw_combobox_controller.js | 4 +-- .../javascripts/hotwire_combobox.esm.js | 35 +++++++------------ .../models/combobox/multiselect.js | 6 ++-- .../hw_combobox/models/combobox/navigation.js | 4 +-- .../hw_combobox/models/combobox/selection.js | 2 +- .../hw_combobox/models/combobox/toggle.js | 19 +++------- .../component/markup/input.rb | 2 +- .../component/markup/wrapper.rb | 2 +- test/system/keyboard_navigation_test.rb | 6 +++- test/system/multiselect_test.rb | 4 +-- test/system/selection_test.rb | 1 - 12 files changed, 37 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index 528be04c..cbf94c98 100644 --- a/README.md +++ b/README.md @@ -124,11 +124,9 @@ This gem follows the [APG combobox pattern guidelines](https://www.w3.org/WAI/AR These are the exceptions: -1. Users cannot manipulate the combobox while it's closed. As long as the combobox is focused, the listbox is shown. -2. The escape key closes the listbox and blurs the combobox. It does not clear the combobox. -3. The listbox has wrap-around selection. That is, pressing `Up Arrow` when the user is on the first option will select the last option. And pressing `Down Arrow` when on the last option will select the first option. In paginated comboboxes, the first and last options refer to the currently available options. More options may be loaded after navigating to the last currently available option. -4. It is possible to have an unlabeled combobox, as that responsibility is delegated to the implementing user. -5. There are currently [no APG guidelines](https://github.com/w3c/aria-practices/issues/1512) for a multiselect combobox. We've introduced some mechanisms to make the experience accessible, like announcing multi-selections via a live region. But we'd welcome feedback on how to make it better until official guidelines are available. +1. The listbox has wrap-around selection. That is, pressing `Up Arrow` when the user is on the first option will select the last option. And pressing `Down Arrow` when on the last option will select the first option. In paginated comboboxes, the first and last options refer to the currently available options. More options may be loaded after navigating to the last currently available option. +2. It is possible to have an unlabeled combobox, as that responsibility is delegated to the implementing user. +3. There are currently [no APG guidelines](https://github.com/w3c/aria-practices/issues/1512) for a multiselect combobox. We've introduced some mechanisms to make the experience accessible, like announcing multi-selections via a live region. But we'd welcome feedback on how to make it better until official guidelines are available. It should be noted none of the maintainers use assistive technologies in their daily lives. If you do, and you feel these exceptions are detrimental to your ability to use the component, or if you find an undocumented exception, please [open a GitHub issue](https://github.com/josefarias/hotwire_combobox/issues). We'll get it sorted. diff --git a/app/assets/javascripts/controllers/hw_combobox_controller.js b/app/assets/javascripts/controllers/hw_combobox_controller.js index 4afea9e6..454d6363 100644 --- a/app/assets/javascripts/controllers/hw_combobox_controller.js +++ b/app/assets/javascripts/controllers/hw_combobox_controller.js @@ -108,7 +108,7 @@ export default class HwComboboxController extends Concerns(...concerns) { this._resetMultiselectionMarks() if (inputType === "hw:multiselectSync") { - this.openByFocusing() + this.open() } else if (inputType !== "hw:lockInSelection") { this._selectOnQuery(inputType) } @@ -119,7 +119,7 @@ export default class HwComboboxController extends Concerns(...concerns) { } closerTargetConnected() { - this._closeAndBlur("hw:asyncCloser") + this.close("hw:asyncCloser") } // Use +_printStack+ for debugging purposes diff --git a/app/assets/javascripts/hotwire_combobox.esm.js b/app/assets/javascripts/hotwire_combobox.esm.js index f1a9137d..fcb4b620 100644 --- a/app/assets/javascripts/hotwire_combobox.esm.js +++ b/app/assets/javascripts/hotwire_combobox.esm.js @@ -802,7 +802,7 @@ Combobox.Multiselect = Base => class extends Base { currentTarget.closest("[data-hw-combobox-chip]").remove(); if (!this._isSmallViewport) { - this.openByFocusing(); + this.open(); } this._announceToScreenReader(display, "removed"); @@ -827,7 +827,7 @@ Combobox.Multiselect = Base => class extends Base { cancel(event); }, Escape: (event) => { - this.openByFocusing(); + this.open(); cancel(event); } } @@ -851,7 +851,7 @@ Combobox.Multiselect = Base => class extends Base { if (shouldReopen) { await nextRepaint(); - this.openByFocusing(); + this.open(); } this._announceToScreenReader(display, "multi-selected. Press Shift + Tab, then Enter to remove."); @@ -969,11 +969,11 @@ Combobox.Navigation = Base => class extends Base { cancel(event); }, Enter: (event) => { - this._closeAndBlur("hw:keyHandler:enter"); + this.close("hw:keyHandler:enter"); cancel(event); }, Escape: (event) => { - this._closeAndBlur("hw:keyHandler:escape"); + this.close("hw:keyHandler:escape"); cancel(event); }, Backspace: (event) => { @@ -1082,7 +1082,7 @@ Combobox.Options = Base => class extends Base { Combobox.Selection = Base => class extends Base { selectOnClick({ currentTarget, inputType }) { this._forceSelectionAndFilter(currentTarget, inputType); - this._closeAndBlur("hw:optionRoleClick"); + this.close("hw:optionRoleClick"); } _connectSelection() { @@ -1512,10 +1512,6 @@ Combobox.Toggle = Base => class extends Base { this.expandedValue = true; } - openByFocusing() { - this._actingCombobox.focus(); - } - close(inputType) { if (this._isOpen) { const shouldReopen = this._isMultiselect && @@ -1543,9 +1539,9 @@ Combobox.Toggle = Base => class extends Base { toggle() { if (this.expandedValue) { - this._closeAndBlur("hw:toggle"); + this.close("hw:toggle"); } else { - this.openByFocusing(); + this.open(); } } @@ -1556,30 +1552,25 @@ Combobox.Toggle = Base => class extends Base { if (this.mainWrapperTarget.contains(target) && !this._isDialogDismisser(target)) return if (this._withinElementBounds(event)) return - this._closeAndBlur("hw:clickOutside"); + this.close("hw:clickOutside"); } closeOnFocusOutside({ target }) { if (!this._isOpen) return if (this.element.contains(target)) return - this._closeAndBlur("hw:focusOutside"); + this.close("hw:focusOutside"); } clearOrToggleOnHandleClick() { if (this._isQueried) { this._clearQuery(); - this._actingCombobox.focus(); + this.open(); } else { this.toggle(); } } - _closeAndBlur(inputType) { - this.close(inputType); - this._actingCombobox.blur(); - } - // Some browser extensions like 1Password overlay elements on top of the combobox. // Hovering over these elements emits a click event for some reason. // These events don't contain any telling information, so we use `_withinElementBounds` @@ -1808,7 +1799,7 @@ class HwComboboxController extends Concerns(...concerns) { this._resetMultiselectionMarks(); if (inputType === "hw:multiselectSync") { - this.openByFocusing(); + this.open(); } else if (inputType !== "hw:lockInSelection") { this._selectOnQuery(inputType); } @@ -1819,7 +1810,7 @@ class HwComboboxController extends Concerns(...concerns) { } closerTargetConnected() { - this._closeAndBlur("hw:asyncCloser"); + this.close("hw:asyncCloser"); } // Use +_printStack+ for debugging purposes diff --git a/app/assets/javascripts/hw_combobox/models/combobox/multiselect.js b/app/assets/javascripts/hw_combobox/models/combobox/multiselect.js index c91afc96..7c41c948 100644 --- a/app/assets/javascripts/hw_combobox/models/combobox/multiselect.js +++ b/app/assets/javascripts/hw_combobox/models/combobox/multiselect.js @@ -25,7 +25,7 @@ Combobox.Multiselect = Base => class extends Base { currentTarget.closest("[data-hw-combobox-chip]").remove() if (!this._isSmallViewport) { - this.openByFocusing() + this.open() } this._announceToScreenReader(display, "removed") @@ -50,7 +50,7 @@ Combobox.Multiselect = Base => class extends Base { cancel(event) }, Escape: (event) => { - this.openByFocusing() + this.open() cancel(event) } } @@ -74,7 +74,7 @@ Combobox.Multiselect = Base => class extends Base { if (shouldReopen) { await nextRepaint() - this.openByFocusing() + this.open() } this._announceToScreenReader(display, "multi-selected. Press Shift + Tab, then Enter to remove.") diff --git a/app/assets/javascripts/hw_combobox/models/combobox/navigation.js b/app/assets/javascripts/hw_combobox/models/combobox/navigation.js index e6b6890d..4b4a9ad6 100644 --- a/app/assets/javascripts/hw_combobox/models/combobox/navigation.js +++ b/app/assets/javascripts/hw_combobox/models/combobox/navigation.js @@ -31,11 +31,11 @@ Combobox.Navigation = Base => class extends Base { cancel(event) }, Enter: (event) => { - this._closeAndBlur("hw:keyHandler:enter") + this.close("hw:keyHandler:enter") cancel(event) }, Escape: (event) => { - this._closeAndBlur("hw:keyHandler:escape") + this.close("hw:keyHandler:escape") cancel(event) }, Backspace: (event) => { diff --git a/app/assets/javascripts/hw_combobox/models/combobox/selection.js b/app/assets/javascripts/hw_combobox/models/combobox/selection.js index 239793df..283edbf9 100644 --- a/app/assets/javascripts/hw_combobox/models/combobox/selection.js +++ b/app/assets/javascripts/hw_combobox/models/combobox/selection.js @@ -4,7 +4,7 @@ import { wrapAroundAccess, isDeleteEvent } from "hw_combobox/helpers" Combobox.Selection = Base => class extends Base { selectOnClick({ currentTarget, inputType }) { this._forceSelectionAndFilter(currentTarget, inputType) - this._closeAndBlur("hw:optionRoleClick") + this.close("hw:optionRoleClick") } _connectSelection() { diff --git a/app/assets/javascripts/hw_combobox/models/combobox/toggle.js b/app/assets/javascripts/hw_combobox/models/combobox/toggle.js index 8a6f8d90..b934f0f1 100644 --- a/app/assets/javascripts/hw_combobox/models/combobox/toggle.js +++ b/app/assets/javascripts/hw_combobox/models/combobox/toggle.js @@ -6,10 +6,6 @@ Combobox.Toggle = Base => class extends Base { this.expandedValue = true } - openByFocusing() { - this._actingCombobox.focus() - } - close(inputType) { if (this._isOpen) { const shouldReopen = this._isMultiselect && @@ -37,9 +33,9 @@ Combobox.Toggle = Base => class extends Base { toggle() { if (this.expandedValue) { - this._closeAndBlur("hw:toggle") + this.close("hw:toggle") } else { - this.openByFocusing() + this.open() } } @@ -50,30 +46,25 @@ Combobox.Toggle = Base => class extends Base { if (this.mainWrapperTarget.contains(target) && !this._isDialogDismisser(target)) return if (this._withinElementBounds(event)) return - this._closeAndBlur("hw:clickOutside") + this.close("hw:clickOutside") } closeOnFocusOutside({ target }) { if (!this._isOpen) return if (this.element.contains(target)) return - this._closeAndBlur("hw:focusOutside") + this.close("hw:focusOutside") } clearOrToggleOnHandleClick() { if (this._isQueried) { this._clearQuery() - this._actingCombobox.focus() + this.open() } else { this.toggle() } } - _closeAndBlur(inputType) { - this.close(inputType) - this._actingCombobox.blur() - } - // Some browser extensions like 1Password overlay elements on top of the combobox. // Hovering over these elements emits a click event for some reason. // These events don't contain any telling information, so we use `_withinElementBounds` diff --git a/app/presenters/hotwire_combobox/component/markup/input.rb b/app/presenters/hotwire_combobox/component/markup/input.rb index 93372b93..b71351c6 100644 --- a/app/presenters/hotwire_combobox/component/markup/input.rb +++ b/app/presenters/hotwire_combobox/component/markup/input.rb @@ -19,7 +19,7 @@ def input_type def input_data combobox_attrs.fetch(:data, {}).merge \ action: " - focus->hw-combobox#open + click->hw-combobox#toggle input->hw-combobox#filterAndSelect keydown->hw-combobox#navigate click@window->hw-combobox#closeOnClickOutside diff --git a/app/presenters/hotwire_combobox/component/markup/wrapper.rb b/app/presenters/hotwire_combobox/component/markup/wrapper.rb index 7a23aaba..d1db84a6 100644 --- a/app/presenters/hotwire_combobox/component/markup/wrapper.rb +++ b/app/presenters/hotwire_combobox/component/markup/wrapper.rb @@ -2,6 +2,6 @@ module HotwireCombobox::Component::Markup::Wrapper def main_wrapper_attrs customize :main_wrapper, base: { class: "hw-combobox__main__wrapper", - data: { action: ("click->hw-combobox#openByFocusing:self" if multiselect?), hw_combobox_target: "mainWrapper" } } + data: { action: ("click->hw-combobox#open:self" if multiselect?), hw_combobox_target: "mainWrapper" } } end end diff --git a/test/system/keyboard_navigation_test.rb b/test/system/keyboard_navigation_test.rb index ca8dbce3..60629582 100644 --- a/test/system/keyboard_navigation_test.rb +++ b/test/system/keyboard_navigation_test.rb @@ -37,9 +37,13 @@ class KeyboardNavigationTest < ApplicationSystemTestCase assert_no_visible_selected_option # because the list is closed type_in_combobox "#state-field", :backspace - assert_open_combobox + assert_closed_combobox assert_combobox_display_and_value "#state-field", "Illinoi", nil + + open_combobox "#state-field" + assert_open_combobox assert_no_visible_selected_option + assert_combobox_display_and_value "#state-field", "Illinoi", nil end test "navigating with the arrow keys" do diff --git a/test/system/multiselect_test.rb b/test/system/multiselect_test.rb index e155dbdc..b95b0409 100644 --- a/test/system/multiselect_test.rb +++ b/test/system/multiselect_test.rb @@ -21,14 +21,12 @@ class MultiselectTest < ApplicationSystemTestCase states(:alabama, :california, :arizona).pluck(:id) remove_chip "California" + assert_option_with text: "California" assert_combobox_display_and_value \ "#states-field", %w[ Alabama Arizona ], states(:alabama, :arizona).pluck(:id) - - open_combobox "#states-field" - assert_option_with text: "California" end test "multiselect idiosyncrasies" do diff --git a/test/system/selection_test.rb b/test/system/selection_test.rb index 3f79f347..3f42881a 100644 --- a/test/system/selection_test.rb +++ b/test/system/selection_test.rb @@ -147,7 +147,6 @@ class SelectionTest < ApplicationSystemTestCase assert_no_selector ".hw-combobox__input[data-queried]" open_combobox "#movie-field" click_on_option "Aladdin" - sleep 1 assert_closed_combobox assert_combobox_display_and_value "#movie-field", "Aladdin", movies(:aladdin).id assert_selector ".hw-combobox__input[data-queried]"