From 5fe72d318b0cbb945edfd91cdcb17d81b25f8eb3 Mon Sep 17 00:00:00 2001 From: Kim Santiago <31145923+kisantia@users.noreply.github.com> Date: Tue, 5 May 2020 13:02:06 -0700 Subject: [PATCH] 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 --- .../browser/ui/taskbar/overflowActionbar.ts | 68 ++++++-- .../ui/taskbar/overflowActionbar.test.ts | 158 ++++++++++++++++++ 2 files changed, 208 insertions(+), 18 deletions(-) create mode 100644 src/sql/base/test/browser/ui/taskbar/overflowActionbar.test.ts diff --git a/src/sql/base/browser/ui/taskbar/overflowActionbar.ts b/src/sql/base/browser/ui/taskbar/overflowActionbar.ts index 770fac818b..e761d7c66f 100644 --- a/src/sql/base/browser/ui/taskbar/overflowActionbar.ts +++ b/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 ((item).className !== 'taskbarSeparator') { - (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 (!(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 ((this._overflow.firstChild).className !== 'taskbarSeparator') { (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 ((item).className !== 'taskbarSeparator') { + (item.firstChild).setAttribute('role', 'button'); + } + this._actionsList.insertBefore(item, this._actionsList.lastChild); + + // move placeholder in this._items if item isn't a separator + if (!(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; + } } diff --git a/src/sql/base/test/browser/ui/taskbar/overflowActionbar.test.ts b/src/sql/base/test/browser/ui/taskbar/overflowActionbar.test.ts new file mode 100644 index 0000000000..a7bbb14a6b --- /dev/null +++ b/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); +}