Fix toolbar overflow focus not always going to first element in overflow (#10246)

* fix overflow focus not always going to first element in overflow

* adding tests

src/sql/base/browser/ui/taskbar/overflowActionbar.ts

@@ -86,16 +86,7 @@ export class OverflowActionBar extends ActionBar {
 			}
 		} else if (this._overflow?.hasChildNodes() && width > this._previousWidth) { // uncollapse actions if there is space for it
 			while (width === fullWidth && this._overflow.hasChildNodes()) {
-				// move placeholder in this._items
-				let placeHolderItem = this._items.splice(this._actionsList.childNodes.length - 1, 1);
-				this._items.splice(this._actionsList.childNodes.length, 0, placeHolderItem[0]);
-
-				let item = this._overflow.removeChild(this._overflow.firstChild);
-				// change role back to button when it's in the toolbar
-				if ((<HTMLElement>item).className !== 'taskbarSeparator') {
-					(<HTMLElement>item.firstChild).setAttribute('role', 'button');
-				}
-				this._actionsList.insertBefore(item, this._actionsList.lastChild);
+				this.restoreItem();
 
 				// if the action was too wide, collapse it again
 				if (this._actionsList.scrollWidth > this._actionsList.offsetWidth) {
@@ -111,23 +102,42 @@ export class OverflowActionBar extends ActionBar {
 		this._previousWidth = width;
 	}
 
-	private collapseItem(): void {
-		// move placeholder in this._items
-		let placeHolderItem = this._items.splice(this._actionsList.childNodes.length - 1, 1);
-		this._items.splice(this._actionsList.childNodes.length - 2, 0, placeHolderItem[0]);
-
+	public collapseItem(): void {
 		let index = this._actionsList.childNodes.length - 2; // remove the last toolbar action before the more actions '...'
 		let item = this._actionsList.removeChild(this._actionsList.childNodes[index]);
 		this._overflow.insertBefore(item, this._overflow.firstChild);
 		this._register(DOM.addDisposableListener(item, DOM.EventType.CLICK, (e => { this.hideOverflowDisplay(); })));
 
+		// move placeholder in this._items if item isn't a separator
+		if (!(<HTMLElement>item).classList.contains('taskbarSeparator')) {
+			let placeHolderIndex = this._items.findIndex(i => i === undefined);
+			let placeHolderItem = this._items.splice(placeHolderIndex, 1);
+			this._items.splice(placeHolderIndex - 1, 0, placeHolderItem[0]);
+		}
+
 		// change role to menuItem when it's in the overflow
 		if ((<HTMLElement>this._overflow.firstChild).className !== 'taskbarSeparator') {
 			(<HTMLElement>this._overflow.firstChild.firstChild).setAttribute('role', 'menuItem');
 		}
 	}
 
-	private createMoreItemElement(): void {
+	public restoreItem(): void {
+		let item = this._overflow.removeChild(this._overflow.firstChild);
+		// change role back to button when it's in the toolbar
+		if ((<HTMLElement>item).className !== 'taskbarSeparator') {
+			(<HTMLElement>item.firstChild).setAttribute('role', 'button');
+		}
+		this._actionsList.insertBefore(item, this._actionsList.lastChild);
+
+		// move placeholder in this._items if item isn't a separator
+		if (!(<HTMLElement>item).classList.contains('taskbarSeparator')) {
+			let placeHolderIndex = this._items.findIndex(i => i === undefined);
+			let placeHolderItem = this._items.splice(placeHolderIndex, 1);
+			this._items.splice(placeHolderIndex + 1, 0, placeHolderItem[0]);
+		}
+	}
+
+	public createMoreItemElement(): void {
 		this._moreItemElement = document.createElement('li');
 		this._moreItemElement.className = 'action-item more';
 		this._moreItemElement.setAttribute('role', 'presentation');
@@ -185,15 +195,21 @@ export class OverflowActionBar extends ActionBar {
 		this._items.push(undefined); // add place holder for more item element
 	}
 
-	private moreElementOnClick(event: MouseEvent | StandardKeyboardEvent): void {
+	public moreElementOnClick(event: MouseEvent | StandardKeyboardEvent): void {
 		this._overflow.style.display = this._overflow.style.display === 'block' ? 'none' : 'block';
 		if (this._overflow.style.display === 'block') {
-			this._focusedItem = this._actionsList.childElementCount;
+			// set focus to the first element in the overflow
+			// separators aren't focusable so we don't want to include them when calculating the focused item index
+			this._focusedItem = this._actionsList.childElementCount - this.getActionListSeparatorCount();
 			this.updateFocus();
 		}
 		DOM.EventHelper.stop(event, true);
 	}
 
+	private getActionListSeparatorCount(): number {
+		return Array.from(this._actionsList.children).filter(c => c.className.includes('taskbarSeparator')).length;
+	}
+
 	private hideOverflowDisplay(): void {
 		this._overflow.style.display = 'none';
 		this._focusedItem = this._actionsList.childElementCount - 1;
@@ -333,4 +349,20 @@ export class OverflowActionBar extends ActionBar {
 		this.hideOverflowDisplay();
 		return this._actionRunner.run(action, context);
 	}
+
+	public get actionsList(): HTMLElement {
+		return this._actionsList;
+	}
+
+	public get items(): IActionViewItem[] {
+		return this._items;
+	}
+
+	public get overflow(): HTMLElement {
+		return this._overflow;
+	}
+
+	public get focusedItem(): number {
+		return this._focusedItem;
+	}
 }

src/sql/base/test/browser/ui/taskbar/overflowActionbar.test.ts

@@ -0,0 +1,158 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the Source EULA. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import * as assert from 'assert';
+import { OverflowActionBar } from 'sql/base/browser/ui/taskbar/overflowActionbar';
+import { Action, IActionViewItem } from 'vs/base/common/actions';
+import { Taskbar } from 'sql/base/browser/ui/taskbar/taskbar';
+
+
+suite('Overflow Actionbar tests', () => {
+	let overflowActionbar: OverflowActionBar;
+
+	setup(() => {
+		overflowActionbar = new OverflowActionBar(document.createElement('div'));
+	});
+
+	test('Verify moving actions from toolbar to overflow', () => {
+		assert(overflowActionbar.actionsList.children.length === 0);
+		assert(overflowActionbar.items.length === 0);
+		assert(overflowActionbar.overflow.childElementCount === 0);
+
+		let a1 = new Action('a1');
+		let a2 = new Action('a1');
+		let a3 = new Action('a3');
+		overflowActionbar.pushAction([a1, a2, a3]);
+
+		assert(overflowActionbar.actionsList.children.length === 3);
+		assert(overflowActionbar.items.length === 3);
+		assert(overflowActionbar.overflow.childElementCount === 0);
+
+		// more item element '...' is added when actions are moved to the overflow
+		// and a placeholder undefined element is added to the items array for calculating focus
+		overflowActionbar.createMoreItemElement();
+		assert(overflowActionbar.actionsList.children.length === 4);
+		assert(overflowActionbar.items.length === 4);
+		assert(overflowActionbar.overflow.childElementCount === 0);
+
+		// move a3 to overflow
+		overflowActionbar.collapseItem();
+		assert(overflowActionbar.actionsList.children.length === 3);
+		assert(overflowActionbar.items.length === 4);
+		assert(getMoreItemPlaceholderIndex(overflowActionbar.items) === 2);
+		assert(overflowActionbar.overflow.childElementCount === 1);
+		verifyOverflowFocusedIndex(overflowActionbar, 3);
+
+		// move a2 to overflow
+		overflowActionbar.collapseItem();
+		assert(overflowActionbar.actionsList.children.length === 2);
+		assert(overflowActionbar.items.length === 4);
+		assert(getMoreItemPlaceholderIndex(overflowActionbar.items) === 1);
+		assert(overflowActionbar.overflow.childElementCount === 2);
+		verifyOverflowFocusedIndex(overflowActionbar, 2);
+
+		// move a2 to back to toolbar
+		overflowActionbar.restoreItem();
+		assert(overflowActionbar.actionsList.children.length === 3);
+		assert(overflowActionbar.items.length === 4);
+		assert(getMoreItemPlaceholderIndex(overflowActionbar.items) === 2);
+		assert(overflowActionbar.overflow.childElementCount === 1);
+		verifyOverflowFocusedIndex(overflowActionbar, 3);
+
+		// move a3 to back to toolbar
+		overflowActionbar.restoreItem();
+		assert(overflowActionbar.actionsList.children.length === 4);
+		assert(overflowActionbar.items.length === 4);
+		assert(getMoreItemPlaceholderIndex(overflowActionbar.items) === 3);
+		assert(overflowActionbar.overflow.childElementCount === 0);
+	});
+
+	test('Verify moving actions and separators from toolbar to overflow', () => {
+		assert(overflowActionbar.actionsList.children.length === 0);
+		assert(overflowActionbar.items.length === 0);
+		assert(overflowActionbar.overflow.childElementCount === 0);
+
+		let a1 = new Action('a1');
+		let a2 = new Action('a1');
+		let separator: HTMLElement = Taskbar.createTaskbarSeparator();
+		let a3 = new Action('a3');
+		overflowActionbar.pushAction([a1, a2]);
+		overflowActionbar.pushElement(separator);
+		overflowActionbar.pushAction([a3]);
+
+		assert(overflowActionbar.actionsList.children.length === 4);
+		assert(overflowActionbar.items.length === 3); // items only has focusable elements
+		assert(overflowActionbar.overflow.childElementCount === 0);
+
+		// more item element '...' is added when actions are moved to the overflow
+		// and a placeholder undefined element is added to the items array for calculating focus
+		overflowActionbar.createMoreItemElement();
+		assert(overflowActionbar.actionsList.children.length === 5);
+		assert(overflowActionbar.items.length === 4);
+		assert(overflowActionbar.overflow.childElementCount === 0);
+
+		// move a3 to overflow
+		overflowActionbar.collapseItem();
+		assert(overflowActionbar.actionsList.children.length === 4);
+		assert(overflowActionbar.items.length === 4);
+		assert(getMoreItemPlaceholderIndex(overflowActionbar.items) === 2);
+		assert(overflowActionbar.overflow.childElementCount === 1);
+		verifyOverflowFocusedIndex(overflowActionbar, 3);
+
+		// move separator to overflow
+		overflowActionbar.collapseItem();
+		assert(overflowActionbar.actionsList.children.length === 3);
+		assert(overflowActionbar.items.length === 4);
+		assert(getMoreItemPlaceholderIndex(overflowActionbar.items) === 2);
+		assert(overflowActionbar.overflow.childElementCount === 2);
+		verifyOverflowFocusedIndex(overflowActionbar, 3);
+
+		// move a2 to overflow
+		overflowActionbar.collapseItem();
+		assert(overflowActionbar.actionsList.children.length === 2);
+		assert(overflowActionbar.items.length === 4);
+		assert(getMoreItemPlaceholderIndex(overflowActionbar.items) === 1);
+		assert(overflowActionbar.overflow.childElementCount === 3);
+		verifyOverflowFocusedIndex(overflowActionbar, 2);
+
+		// move a2 to back to toolbar
+		overflowActionbar.restoreItem();
+		assert(overflowActionbar.actionsList.children.length === 3);
+		assert(overflowActionbar.items.length === 4);
+		assert(getMoreItemPlaceholderIndex(overflowActionbar.items) === 2);
+		assert(overflowActionbar.overflow.childElementCount === 2);
+		verifyOverflowFocusedIndex(overflowActionbar, 3);
+
+		// move separator to back to toolbar
+		overflowActionbar.restoreItem();
+		assert(overflowActionbar.actionsList.children.length === 4);
+		assert(overflowActionbar.items.length === 4);
+		assert(getMoreItemPlaceholderIndex(overflowActionbar.items) === 2);
+		assert(overflowActionbar.overflow.childElementCount === 1);
+		verifyOverflowFocusedIndex(overflowActionbar, 3);
+
+		// move a3 to back to toolbar
+		overflowActionbar.restoreItem();
+		assert(overflowActionbar.actionsList.children.length === 5);
+		assert(overflowActionbar.items.length === 4);
+		assert(getMoreItemPlaceholderIndex(overflowActionbar.items) === 3);
+		assert(overflowActionbar.overflow.childElementCount === 0);
+	});
+});
+
+function verifyOverflowFocusedIndex(overflowToolbar: OverflowActionBar, expectedIndex: number) {
+	// click more item element to show overflow and set focus to first element
+	overflowToolbar.moreElementOnClick(new MouseEvent('click'));
+	assert(overflowToolbar.focusedItem === expectedIndex);
+	// click to hide overflow
+	overflowToolbar.moreElementOnClick(new MouseEvent('click'));
+}
+
+/**
+ * Gets index of placeholder for more item element '...'. This undefined element is needed for calculating which item should be focused when the overflow menu is opened
+ */
+function getMoreItemPlaceholderIndex(items: IActionViewItem[]): number {
+	return items.findIndex(i => i === undefined);
+}