From f18fd45bb188907a00f93557792f36273a4c9e66 Mon Sep 17 00:00:00 2001 From: Dobrin Dimchev Date: Tue, 30 Mar 2021 15:32:18 +0300 Subject: [PATCH] fix(ui5-select): keyboard/selection handling improvement (#2907) --- packages/main/src/Option.js | 11 ++ packages/main/src/Select.js | 84 +++++++------ packages/main/src/SelectPopover.hbs | 6 +- packages/main/src/themes/Select.css | 5 + packages/main/test/specs/Select.spec.js | 159 ++++++++++++++---------- 5 files changed, 155 insertions(+), 110 deletions(-) diff --git a/packages/main/src/Option.js b/packages/main/src/Option.js index 74f7d4e99175..4423ca74b7bc 100644 --- a/packages/main/src/Option.js +++ b/packages/main/src/Option.js @@ -66,6 +66,17 @@ const metadata = { stableDomRef: { type: String, }, + + /** + * Defines the focused state of the ui5-option. + * @type {boolean} + * @defaultvalue false + * @since 1.0.0-rc.13 + * @private + */ + _focused: { + type: Boolean, + }, }, slots: /** @lends sap.ui.webcomponents.main.Option.prototype */ { /** diff --git a/packages/main/src/Select.js b/packages/main/src/Select.js index 26accdf3113b..867b6871e4a5 100644 --- a/packages/main/src/Select.js +++ b/packages/main/src/Select.js @@ -324,10 +324,6 @@ class Select extends UI5Element { if (!this._listWidth) { this._listWidth = this.responsivePopover.offsetWidth; } - if (this.responsivePopover.querySelector("[ui5-li][focused]:not([selected])")) { - // selection changed programmatically => apply focus to the newly selected item - this._applyFocusAfterOpen(); - } } } @@ -385,9 +381,11 @@ class Select extends UI5Element { } opt.selected = false; + opt._focused = false; return { selected: false, + _focused: false, disabled: opt.disabled, icon: opt.icon, value: opt.value, @@ -399,7 +397,9 @@ class Select extends UI5Element { if (lastSelectedOptionIndex > -1 && !opts[lastSelectedOptionIndex].disabled) { opts[lastSelectedOptionIndex].selected = true; + opts[lastSelectedOptionIndex]._focused = true; this.options[lastSelectedOptionIndex].selected = true; + this.options[lastSelectedOptionIndex]._focused = true; this._text = opts[lastSelectedOptionIndex].textContent; this._selectedIndex = lastSelectedOptionIndex; } else { @@ -407,7 +407,9 @@ class Select extends UI5Element { this._selectedIndex = -1; if (opts[firstEnabledOptionIndex]) { opts[firstEnabledOptionIndex].selected = true; + opts[firstEnabledOptionIndex]._focused = true; this.options[firstEnabledOptionIndex].selected = true; + this.options[firstEnabledOptionIndex]._focused = true; this._selectedIndex = firstEnabledOptionIndex; this._text = this.options[firstEnabledOptionIndex].textContent; } @@ -444,14 +446,24 @@ class Select extends UI5Element { event.preventDefault(); } - if (!this._isPickerOpen) { - this._handleArrowNavigation(event, true); + if (isEscape(event) && this._isPickerOpen) { + this._escapePressed = true; + } + + if (isEnter(event)) { + this._handleSelectionChange(); } + + this._handleArrowNavigation(event, true); } _onkeyup(event) { - if (isSpace(event) && !this._isPickerOpen) { - this._toggleRespPopover(); + if (isSpace(event)) { + if (this._isPickerOpen) { + this._handleSelectionChange(); + } else { + this._toggleRespPopover(); + } } } @@ -472,9 +484,13 @@ class Select extends UI5Element { _handleItemPress(event) { const item = event.detail.item; const selectedItemIndex = this._getSelectedItemIndex(item); - this._select(selectedItemIndex); - this._toggleRespPopover(); + this._handleSelectionChange(selectedItemIndex); + } + + _itemMousedown(event) { + // prevent actual focus of items + event.preventDefault(); } _onclick(event) { @@ -483,36 +499,13 @@ class Select extends UI5Element { } /** - * The user used arrow up/down on the list + * The user selected an item with Enter or Space * @private */ - _handleSelectionChange(event) { - const item = event.detail.selectedItems[0]; - const selectedItemIndex = this._getSelectedItemIndex(item); - this._select(selectedItemIndex); - } + _handleSelectionChange(index = this._selectedIndex) { + this._select(index); - _applyFocusAfterOpen() { - if (!this._currentlySelectedOption) { - return; - } - - const li = this.responsivePopover.querySelector(`#${this._currentlySelectedOption._id}-li`); - if (!li) { - return; - } - - this.responsivePopover.querySelector("[ui5-list]").focusItem(li); - } - - _handlePickerKeydown(event) { - if (isEscape(event) && this._isPickerOpen) { - this._escapePressed = true; - } - - if (isEnter(event) || isSpace(event)) { - this._shouldClosePopover = true; - } + this._toggleRespPopover(); } _handleArrowNavigation(event, shouldFireEvent) { @@ -530,14 +523,22 @@ class Select extends UI5Element { } this.options[this._selectedIndex].selected = false; + this.options[this._selectedIndex]._focused = false; + this.options[nextIndex].selected = true; + this.options[nextIndex]._focused = true; + this._selectedIndex = nextIndex === -1 ? this._selectedIndex : nextIndex; if (currentIndex !== this._selectedIndex) { + // Announce new item even if picker is opened. + // The aria-activedescendents attribute can't be used, + // because listitem elements are in different shadow dom this.itemSelectionAnnounce(); } - if (shouldFireEvent) { + if (shouldFireEvent && !this._isPickerOpen) { + // arrow pressed on closed picker - do selection change this._fireChangeEvent(this.options[nextIndex]); } } @@ -556,7 +557,12 @@ class Select extends UI5Element { this._lastSelectedOption = this.options[this._selectedIndex]; } + _afterOpen() { + this.opened = true; + } + _afterClose() { + this.opened = false; this._iconPressed = false; this._listWidth = 0; @@ -674,7 +680,7 @@ class Select extends UI5Element { itemSelectionAnnounce() { const invisibleText = this.shadowRoot.querySelector(`#${this._id}-selectionText`); - if (this.focused && !this._isPickerOpen && this._currentlySelectedOption) { + if (this.focused && this._currentlySelectedOption) { invisibleText.textContent = this._currentlySelectedOption.textContent; } else { invisibleText.textContent = ""; diff --git a/packages/main/src/SelectPopover.hbs b/packages/main/src/SelectPopover.hbs index 15ea0baf7c08..e508506a6cc1 100644 --- a/packages/main/src/SelectPopover.hbs +++ b/packages/main/src/SelectPopover.hbs @@ -5,7 +5,7 @@ content-only-on-desktop placement-type="Bottom" horizontal-align="Left" - @ui5-after-open="{{_applyFocusAfterOpen}}" + @ui5-after-open="{{_afterOpen}}" @ui5-before-open="{{_beforeOpen}}" @ui5-after-close="{{_afterClose}}" @keydown="{{_onkeydown}}" @@ -41,8 +41,7 @@ {{#each _syncedOptions}} @@ -50,6 +49,7 @@ id="{{this.id}}-li" icon="{{this.icon}}" ?selected="{{this.selected}}" + ?focused="{{this._focused}}" ?disabled="{{this.disabled}}" ?aria-selected="{{this.selected}}" data-ui5-stable="{{this.stableDomRef}}" diff --git a/packages/main/src/themes/Select.css b/packages/main/src/themes/Select.css index c746fef4bf66..44098154ac4b 100644 --- a/packages/main/src/themes/Select.css +++ b/packages/main/src/themes/Select.css @@ -35,3 +35,8 @@ :host(:not([disabled])) { cursor: pointer; } + +:host([focused][opened]), +:host([value-state]:not([value-state="None"])[focused][opened]) { + outline: none; +} diff --git a/packages/main/test/specs/Select.spec.js b/packages/main/test/specs/Select.spec.js index 6d57b79b1da2..f0755d1315d4 100644 --- a/packages/main/test/specs/Select.spec.js +++ b/packages/main/test/specs/Select.spec.js @@ -23,19 +23,23 @@ describe("Select general interaction", () => { }); it("does not fire change, when clicking on selected item", () => { + browser.url("http://localhost:8080/test-resources/pages/Select.html"); + const select = $("#mySelect"); const inputResult = browser.$("#inputResult").shadow$("input"); select.click(); const staticAreaItemClassName = browser.getStaticAreaItemClassName("#mySelect") - const firstItem = browser.$(`.${staticAreaItemClassName}`).shadow$("ui5-li:first-child"); + const firstItem = browser.$(`.${staticAreaItemClassName}`).shadow$("ui5-li:last-child"); firstItem.click(); - assert.strictEqual(inputResult.getProperty("value"), "1", "Event not fired when already selected item is selected"); + assert.strictEqual(inputResult.getProperty("value"), "", "Event not fired when already selected item is selected"); }); it("fires change on selection with keyboard handling", () => { + browser.url("http://localhost:8080/test-resources/pages/Select.html"); + const select = $("#mySelect2").shadow$(".ui5-select-root"); const selectText = browser.$("#mySelect2").shadow$(".ui5-select-label-root"); const inputResult = browser.$("#inputResult"); @@ -46,19 +50,21 @@ describe("Select general interaction", () => { select.keys("ArrowUp"); select.keys("Enter"); - assert.strictEqual(inputResult.getProperty("value"), "2", "Fired change event is called once more."); + assert.strictEqual(inputResult.getProperty("value"), "1", "Fired change event is called once more."); assert.ok(selectText.getHTML(false).indexOf(EXPECTED_SELECTION_TEXT1) !== -1, "Select label is correct."); select.click(); select.keys("ArrowDown"); select.keys("Space"); - assert.strictEqual(inputResult.getProperty("value"), "3", "Fired change event is called once more."); + assert.strictEqual(inputResult.getProperty("value"), "2", "Fired change event is called once more."); assert.ok(selectText.getHTML(false).indexOf(EXPECTED_SELECTION_TEXT2) !== -1, "Select label is correct."); }); it("changes selection while closed with Arrow Up/Down", () => { + browser.url("http://localhost:8080/test-resources/pages/Select.html"); + const inputResult = browser.$("#inputResult").shadow$("input"); const select = $("#mySelect2"); const selectText = browser.$("#mySelect2").shadow$(".ui5-select-label-root"); @@ -75,42 +81,55 @@ describe("Select general interaction", () => { select.keys("ArrowDown"); assert.ok(selectText.getHTML(false).indexOf(EXPECTED_SELECTION_TEXT2) > -1, "Arrow Down should change selected item"); - assert.strictEqual(inputResult.getProperty("value"), "5", "Change event should have fired twice"); + assert.strictEqual(inputResult.getProperty("value"), "2", "Change event should have fired twice"); }); - // TODO: Temporary commented as fails on the central build on regular basis - // it("changes selection sync with selection announcement", () => { - // const btn = $("#myBtn2"); - // const inputResult = browser.$("#inputResult").shadow$("input"); - // const select = $("#mySelect2"); - // const selectId = select.getProperty("_id") - // const selectText = browser.$("#mySelect2").shadow$(".ui5-select-label-root"); - // const selectionText = browser.$("#mySelect2").shadow$(`#${selectId}-selectionText`); - // const EXPECTED_SELECTION_TEXT1 = "Compact"; - // const EXPECTED_SELECTION_TEXT2 = "Condensed"; + it("changes selection sync with selection announcement", () => { + browser.url("http://localhost:8080/test-resources/pages/Select.html"); - // select.click(); - // select.keys("Escape"); + const btn = $("#myBtn2"); + const inputResult = browser.$("#inputResult").shadow$("input"); + const select = $("#mySelect2"); + const selectId = select.getProperty("_id") + const selectText = browser.$("#mySelect2").shadow$(".ui5-select-label-root"); + const selectionText = browser.$("#mySelect2").shadow$(`#${selectId}-selectionText`); + const EXPECTED_SELECTION_TEXT1 = "Compact"; + const EXPECTED_SELECTION_TEXT2 = "Condensed"; - // assert.strictEqual(selectionText.getHTML(false), "", "Selection announcement text should be clear if there is no interaction"); + // open picker without following interaction + select.click(); + select.keys("Escape"); - // select.keys("ArrowUp"); - // assert.ok(selectText.getHTML(false).indexOf(EXPECTED_SELECTION_TEXT1), "Arrow Up should change selected item"); - // assert.strictEqual(selectionText.getHTML(false), EXPECTED_SELECTION_TEXT1, "Selection announcement text should be equalt to the current selected item's text"); + assert.strictEqual(selectionText.getHTML(false), "", "Selection announcement text should be clear if there is no interaction"); - // select.click(); - // select.keys("Escape"); - // assert.strictEqual(selectionText.getHTML(false), "", "Selection announcement text should be cleared if the picker is opened"); + // change selection with picker closed + select.keys("ArrowUp"); + assert.ok(selectText.getHTML(false).indexOf(EXPECTED_SELECTION_TEXT1), "Arrow Up should change selected item"); + assert.strictEqual(selectionText.getHTML(false), EXPECTED_SELECTION_TEXT1, "Selection announcement text should be equalt to the current selected item's text"); - // select.keys("ArrowDown"); - // assert.ok(selectText.getHTML(false).indexOf(EXPECTED_SELECTION_TEXT2), "Arrow Up should change selected item"); - // assert.strictEqual(selectionText.getHTML(false), EXPECTED_SELECTION_TEXT2, "Selection announcement text should be equalt to the current selected item's text"); + // change selection with picker closed + select.keys("ArrowDown"); + assert.ok(selectText.getHTML(false).indexOf(EXPECTED_SELECTION_TEXT2), "Arrow Down should change selected item"); + assert.strictEqual(selectionText.getHTML(false), EXPECTED_SELECTION_TEXT2, "Selection announcement text should be equalt to the current selected item's text"); + + // change previewed item with picker opened + select.click(); + select.keys("ArrowUp"); + assert.strictEqual(selectionText.getHTML(false), EXPECTED_SELECTION_TEXT1, "Selection announcement text should be equalt to the current selected item's text"); + select.keys("Escape"); + + // change selection with picker opened + select.click(); + select.keys("ArrowUp"); + select.keys("Enter"); + assert.ok(selectText.getHTML(false).indexOf(EXPECTED_SELECTION_TEXT1), "Arrow Up and Enter should change selected item"); + assert.strictEqual(selectionText.getHTML(false), EXPECTED_SELECTION_TEXT1, "Selection announcement text should be equalt to the current selected item's text"); - // btn.click(); - // assert.strictEqual(selectionText.getHTML(false), "", "Selection announcement text should be cleared on focusout"); + btn.click(); + assert.strictEqual(selectionText.getHTML(false), "", "Selection announcement text should be cleared on focusout"); - // assert.strictEqual(inputResult.getProperty("value"), "7", "Change event should have fired twice"); - // }); + assert.strictEqual(inputResult.getProperty("value"), "3", "Change event should have fired twice"); + }); /* it("changes selection on Tab", () => { @@ -261,56 +280,60 @@ describe("Select general interaction", () => { restoreItemsBtn.click(); }); - // TODO: Temporary commented as fails on the central build on regular basis - // it("reverts value before open after clicking on escape", () => { - // const select = $("#mySelect"); - // const selectText = browser.$("#mySelect").shadow$(".ui5-select-label-root").getHTML(false); - // const inputResult = browser.$("#inputResult").shadow$("input"); + it("reverts value before open after clicking on escape", () => { + browser.url("http://localhost:8080/test-resources/pages/Select.html"); + + const select = $("#mySelect"); + const selectText = browser.$("#mySelect").shadow$(".ui5-select-label-root").getHTML(false); + const inputResult = browser.$("#inputResult").shadow$("input"); - // select.click(); - // select.keys("ArrowDown"); - // select.keys("Escape"); + select.click(); + select.keys("ArrowDown"); + select.keys("Escape"); - // const selectedOption = browser.$("#mySelect ui5-option[selected]"); - // const selectTextAfterEscape = browser.$("#mySelect").shadow$(".ui5-select-label-root").getHTML(false); + const selectedOption = browser.$("#mySelect ui5-option[selected]"); + const selectTextAfterEscape = browser.$("#mySelect").shadow$(".ui5-select-label-root").getHTML(false); - // assert.ok(selectedOption.getProperty("selected"), "Initially selected item should remain selected"); - // assert.strictEqual(inputResult.getProperty("value"), "7", "Change event should not be fired"); - // assert.strictEqual(selectTextAfterEscape, selectText, "Initially selected item should remain selected"); - // }); + assert.ok(selectedOption.getProperty("selected"), "Initially selected item should remain selected"); + assert.strictEqual(inputResult.getProperty("value"), "", "Change event should not be fired"); + assert.strictEqual(selectTextAfterEscape, selectText, "Initially selected item should remain selected"); + }); - // it("fires change event after selection is change and picker if focussed out", () => { - // const select = $("#mySelect"); - // const inputResult = browser.$("#inputResult").shadow$("input"); - // const btn = $("#myBtn2"); + it("fires change event after selection is change and picker if focussed out", () => { + browser.url("http://localhost:8080/test-resources/pages/Select.html"); - // select.click(); - // select.keys("ArrowDown"); - // select.keys("ArrowDown"); + const select = $("#mySelect"); + const inputResult = browser.$("#inputResult").shadow$("input"); + const btn = $("#myBtn2"); - // // focus out select - // btn.click(); + select.click(); + select.keys("ArrowUp"); - // assert.strictEqual(inputResult.getProperty("value"), "8", "Change event should be fired"); - // }); + // focus out select + btn.click(); - // it("fires change event after selecting a previewed item", () => { - // const select = $("#mySelect"); - // const inputResult = browser.$("#inputResult").shadow$("input"); + assert.strictEqual(inputResult.getProperty("value"), "1", "Change event should be fired"); + }); - // select.click(); - // select.keys("ArrowDown"); + it("fires change event after selecting a previewed item", () => { + browser.url("http://localhost:8080/test-resources/pages/Select.html"); - // select.keys("Escape"); + const select = $("#mySelect"); + const inputResult = browser.$("#inputResult").shadow$("input"); + + select.click(); + select.keys("ArrowDown"); + + select.keys("Escape"); - // select.click(); - // const staticAreaItemClassName = browser.getStaticAreaItemClassName("#mySelect"); - // const firstItem = browser.$(`.${staticAreaItemClassName}`).shadow$("ui5-li:first-child"); + select.click(); + const staticAreaItemClassName = browser.getStaticAreaItemClassName("#mySelect"); + const firstItem = browser.$(`.${staticAreaItemClassName}`).shadow$("ui5-li:first-child"); - // firstItem.click(); + firstItem.click(); - // assert.strictEqual(inputResult.getProperty("value"), "9", "Change event should be fired"); - // }); + assert.strictEqual(inputResult.getProperty("value"), "1", "Change event should be fired"); + }); it("tests ESC on closed picker", () => { const select = $("#mySelectEsc");