fix(ui5-select): keyboard/selection handling improvement (#2907)

packages/main/src/Option.js

@@ -66,6 +66,17 @@ const metadata = {
 		stableDomRef: {
 			type: String,
 		},
+
+		/**
+		 * Defines the focused state of the <code>ui5-option</code>.
+		 * @type {boolean}
+		 * @defaultvalue false
+		 * @since 1.0.0-rc.13
+		 * @private
+		 */
+		_focused: {
+			type: Boolean,
+		},
 	},
 	slots: /** @lends sap.ui.webcomponents.main.Option.prototype */ {
 		/**

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,15 +397,19 @@ 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 {
 			this._text = "";
 			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 = "";

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,15 +41,15 @@
 		<ui5-list
 			mode="SingleSelectAuto"
 			separators="None"
-			@keydown="{{_handlePickerKeydown}}"
-			@ui5-selection-change="{{_handleSelectionChange}}"
+			@mousedown="{{_itemMousedown}}"
 			@ui5-item-press="{{_handleItemPress}}"
 		>
 			{{#each _syncedOptions}}
 				<ui5-li
 					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}}"

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;
+}