fix(ui5-combobox): Improve arrow navigation functionality (#3928)

* Enable arrow up on the first item to move the focus to the input

* Include valuestate header in the arrow chain

* On ENTER move the focus to the input, bug fixes

* Fix the big with the undesired closing of the popover when navigating fast through the items + refactoring of the arrow navigation logic

* Restore hash.txt

* Restore hash file

* Add tests

* Add more test, linting

* Do not focus selected suggestion after opening

* Restore visible focus on the input when typing and suggestions are opened

* Merge with master, converts the new tests to be async

* Fix arrow nav when the suggestions popover is closed

* Fix lint errors

* Refactoring

* Add more tests

* Retrigger build

* Fix item announcement

* Fix focus on closing with alt+arr

* Fix double focus on value state header when ESC is pressed and on user input

Author: ndeshev

packages/main/src/ComboBox.js

@@ -189,6 +189,14 @@ const metadata = {
 			type: Boolean,
 		},
 
+		/**
+		 * Indicates whether the visual focus is on the value state header
+		 * @private
+		 */
+		_isValueStateFocused: {
+			type: Boolean,
+		},
+
 		/**
 		 * Sets the accessible aria name of the component.
 		 *
@@ -390,13 +398,6 @@ class ComboBox extends UI5Element {
 		}
 
 		this._selectMatchingItem();
-
-		if (this._isKeyNavigation && this.responsivePopover && this.responsivePopover.opened) {
-			this.focused = false;
-		} else if (this.shadowRoot.activeElement) {
-			this.focused = this.shadowRoot.activeElement.id === "ui5-combobox-input";
-		}
-
 		this._initialRendering = false;
 		this._isKeyNavigation = false;
 	}
@@ -410,15 +411,16 @@ class ComboBox extends UI5Element {
 
 		if (this.shouldClosePopover() && !isPhone()) {
 			this.responsivePopover.close(false, false, true);
+			this._clearFocus();
+			this._itemFocused = false;
 		}
 
-		this._itemFocused = false;
 		this.toggleValueStatePopover(this.shouldOpenValueStateMessagePopover);
 		this.storeResponsivePopoverWidth();
 	}
 
 	shouldClosePopover() {
-		return this.responsivePopover.opened && !this.focused && !this._itemFocused;
+		return this.responsivePopover.opened && !this.focused && !this._itemFocused && !this._isValueStateFocused;
 	}
 
 	_focusin(event) {
@@ -519,6 +521,8 @@ class ComboBox extends UI5Element {
 		if (event.target === this.inner) {
 			// stop the native event, as the semantic "input" would be fired.
 			event.stopImmediatePropagation();
+			this.focused = true;
+			this._isValueStateFocused = false;
 		}
 
 		this._filteredItems = this._filterItems(value);
@@ -538,8 +542,6 @@ class ComboBox extends UI5Element {
 				});
 
 				this._selectionChanged = false;
-
-				item.focused = true;
 			}
 		}
 
@@ -573,40 +575,62 @@ class ComboBox extends UI5Element {
 			return;
 		}
 
+		const isOpen = this.open;
 		const isArrowDown = isDown(event);
 		const isArrowUp = isUp(event);
 		const currentItem = this._filteredItems.find(item => {
-			return this.responsivePopover.opened ? item.focused : item.selected;
+			return isOpen ? item.focused : item.selected;
 		});
-		let indexOfItem = this._filteredItems.indexOf(currentItem);
+		const indexOfItem = this._filteredItems.indexOf(currentItem);
 
 		event.preventDefault();
 
-		if ((indexOfItem === 0 && isArrowUp) || (this._filteredItems.length - 1 === indexOfItem && isArrowDown)) {
+		if ((this.focused === true && isArrowUp && isOpen) || (this._filteredItems.length - 1 === indexOfItem && isArrowDown)) {
 			return;
 		}
 
-		this._clearFocus();
+		this._isKeyNavigation = true;
 
-		indexOfItem += isArrowDown ? 1 : -1;
-		indexOfItem = indexOfItem < 0 ? 0 : indexOfItem;
-		this._filteredItems[indexOfItem].focused = true;
+		if (isArrowDown) {
+			this._handleArrowDown(event, indexOfItem);
+		}
 
-		if (this.responsivePopover.opened) {
-			this.announceSelectedItem(indexOfItem);
+		if (isArrowUp) {
+			this._handleArrowUp(event, indexOfItem);
 		}
+	}
 
-		this.value = this._filteredItems[indexOfItem].isGroupItem ? this.filterValue : this._filteredItems[indexOfItem].text;
+	_handleItemNavigation(event, indexOfItem, isForward) {
+		const isOpen = this.open;
+		const currentItem = this._filteredItems[indexOfItem];
+		const nextItem = isForward ? this._filteredItems[indexOfItem + 1] : this._filteredItems[indexOfItem - 1];
+		const isGroupItem = currentItem && currentItem.isGroupItem;
 
-		this._isKeyNavigation = true;
-		this._itemFocused = true;
+		if ((!isOpen) && ((isGroupItem && !nextItem) || (!isGroupItem && !currentItem))) {
+			return;
+		}
+
+		this._clearFocus();
+
+		if (isOpen) {
+			this._itemFocused = true;
+			this.value = isGroupItem ? this.filterValue : currentItem.text;
+			this.focused = false;
+			currentItem.focused = true;
+		} else {
+			this.focused = true;
+			this.value = isGroupItem ? nextItem.text : currentItem.text;
+			currentItem.focused = false;
+		}
+
+		this._isValueStateFocused = false;
 		this._selectionChanged = true;
 
-		if (this._filteredItems[indexOfItem].isGroupItem) {
+		if (isGroupItem && isOpen) {
 			return;
 		}
 
-		this._filteredItems[indexOfItem].selected = true;
+		this._announceSelectedItem(indexOfItem);
 
 		// autocomplete
 		const item = this._autoCompleteValue(this.value);
@@ -621,6 +645,48 @@ class ComboBox extends UI5Element {
 		this._fireChangeEvent();
 	}
 
+	_handleArrowDown(event, indexOfItem) {
+		const isOpen = this.open;
+
+		if (this.focused && indexOfItem === -1 && this.hasValueStateText && isOpen) {
+			this._isValueStateFocused = true;
+			this.focused = false;
+			return;
+		}
+
+		indexOfItem = !isOpen && this.hasValueState && indexOfItem === -1 ? 0 : indexOfItem;
+
+		this._handleItemNavigation(event, ++indexOfItem, true /* isForward */);
+	}
+
+	_handleArrowUp(event, indexOfItem) {
+		const isOpen = this.open;
+
+		if (indexOfItem === 0 && !this.hasValueStateText) {
+			this._clearFocus();
+			this.focused = true;
+			this._itemFocused = false;
+			return;
+		}
+
+		if (indexOfItem === 0 && this.hasValueStateText && isOpen) {
+			this._clearFocus();
+			this._itemFocused = false;
+			this._isValueStateFocused = true;
+			this._filteredItems[0].selected = false;
+			return;
+		}
+
+		if (this._isValueStateFocused) {
+			this.focused = true;
+			this._isValueStateFocused = false;
+			return;
+		}
+
+		indexOfItem = !isOpen && this.hasValueState && indexOfItem === -1 ? 0 : indexOfItem;
+		this._handleItemNavigation(event, --indexOfItem, false /* isForward */);
+	}
+
 	_keydown(event) {
 		const isArrowKey = isDown(event) || isUp(event);
 		this._autocomplete = !(isBackSpace(event) || isDelete(event));
@@ -632,16 +698,32 @@ class ComboBox extends UI5Element {
 		if (isEnter(event)) {
 			this._fireChangeEvent();
 			this._closeRespPopover();
+			this.focused = true;
 		}
 
-		if (isEscape(event) && !this.open) {
-			this.value = this._lastValue;
+		if (isEscape(event)) {
+			this.focused = true;
+			this.value = !this.open ? this._lastValue : this.value;
+			this._isValueStateFocused = false;
 		}
 
 		if (isShow(event) && !this.readonly && !this.disabled) {
 			event.preventDefault();
+
 			this._resetFilter();
 			this._toggleRespPopover();
+
+			const selectedItem = this._filteredItems.find(item => {
+				return item.selected;
+			});
+
+			if (selectedItem && this.open) {
+				this._itemFocused = true;
+				selectedItem.focused = true;
+				this.focused = false;
+			} else {
+				this.focused = true;
+			}
 		}
 	}
 
@@ -657,6 +739,9 @@ class ComboBox extends UI5Element {
 			this.filterValue = this._selectedItemText;
 		}
 
+		this._isValueStateFocused = false;
+		this._clearFocus();
+
 		this.responsivePopover.close();
 	}
 
@@ -786,7 +871,7 @@ class ComboBox extends UI5Element {
 		this._itemFocused = true;
 	}
 
-	announceSelectedItem(indexOfItem) {
+	_announceSelectedItem(indexOfItem) {
 		const itemPositionText = this.i18nBundle.getText(LIST_ITEM_POSITION, [indexOfItem + 1], [this._filteredItems.length]);
 		const itemSelectionText = this.i18nBundle.getText(LIST_ITEM_SELECTED);
 

packages/main/src/ComboBoxPopover.hbs

@@ -54,7 +54,7 @@
 
 	{{#unless _isPhone}}
 		{{#if hasValueStateText}}
-			<div class="ui5-responsive-popover-header {{classes.popoverValueState}}" style="{{styles.suggestionPopoverHeader}}">
+			<div class="ui5-responsive-popover-header {{classes.popoverValueState}}" ?focused={{_isValueStateFocused}} tabindex="0" style="{{styles.suggestionPopoverHeader}}">
 				{{> valueStateMessage}}
 			</div>
 		{{/if}}

packages/main/src/themes/ValueStateMessage.css

@@ -36,3 +36,10 @@ ui5-responsive-popover .ui5-valuestatemessage-header {
 .ui5-valuestatemessage--information {
 	background: var(--sapInformationBackground);
 }
+
+.ui5-responsive-popover-header[focused] {
+	outline-width: 0.0625rem;
+	outline-style: dotted;
+	outline-offset: -0.125rem;
+	outline-color: initial;
+}

packages/main/test/pages/ComboBox.html

@@ -99,6 +99,25 @@
 		<ui5-combobox id="" style="width: 360px;" value-state="Success"></ui5-combobox>
 	</div>
 
+	<div class="demo-section">
+		<ui5-label id="combo-vs-grouping-label">Items with grouping and value state:</ui5-label>
+
+		<ui5-combobox id="value-state-grouping" value-state="Warning">
+			<ui5-cb-group-item text="A"></ui5-cb-group-item>
+			<ui5-cb-item text="Argentina"></ui5-cb-item>
+			<ui5-cb-item text="Australia"></ui5-cb-item>
+			<ui5-cb-item text="Austria"></ui5-cb-item>	
+			<ui5-cb-group-item text="B"></ui5-cb-group-item>
+			<ui5-cb-item text="Bahrain"></ui5-cb-item>
+			<ui5-cb-item text="Belgium"></ui5-cb-item>
+			<ui5-cb-item text="Brazil"></ui5-cb-item>
+			<ui5-cb-group-item text="C"></ui5-cb-group-item>
+			<ui5-cb-item text="Canada"></ui5-cb-item>
+			<ui5-cb-item text="Chile"></ui5-cb-item>
+		</ui5-combobox>
+	</div>
+	
+
 	<div class="demo-section">
 		<ui5-label id="combo-grouping-label">Items with grouping:</ui5-label>
 		<ui5-combobox filter="StartsWith" id="combo-grouping" style="width: 360px;" aria-label="Select destination:">