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 528be04..cbf94c9 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 4afea9e..454d636 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 f1a9137..fcb4b62 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 c91afc9..7c41c94 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 e6b6890..4b4a9ad 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 239793d..283edbf 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 8a6f8d9..b934f0f 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 93372b9..b71351c 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 7a23aab..d1db84a 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 ca8dbce..6062958 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 e155dbd..b95b040 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 3f79f34..3f42881 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]"