From e941c37aa91f308680dbd3cc102712150b1e4758 Mon Sep 17 00:00:00 2001 From: Gabriel Fouasnon Date: Sun, 23 Jul 2023 22:14:53 +0300 Subject: [PATCH 01/13] wip --- packages/default-theme/style/menubar.css | 12 +++- packages/widgets/src/menubar.ts | 70 +++++++++++++--------- packages/widgets/tests/src/menubar.spec.ts | 20 ++++--- 3 files changed, 64 insertions(+), 38 deletions(-) diff --git a/packages/default-theme/style/menubar.css b/packages/default-theme/style/menubar.css index b60623444..8d80d62ba 100644 --- a/packages/default-theme/style/menubar.css +++ b/packages/default-theme/style/menubar.css @@ -33,7 +33,8 @@ border-right: 1px solid transparent; } -.lm-MenuBar-item.lm-mod-active { +.lm-MenuBar-item.lm-mod-active, +.lm-MenuBar-item:hover { background: #e5e5e5; } @@ -44,3 +45,12 @@ border-right: 1px solid #c0c0c0; box-shadow: 0px 0px 6px rgba(0, 0, 0, 0.2); } + +.lm-MenuBar-item:focus { + text-decoration: underline; +} + +.lm-MenuBar-item:focus-visible { + outline: 2px solid #333; + outline-offset: -2px; +} \ No newline at end of file diff --git a/packages/widgets/src/menubar.ts b/packages/widgets/src/menubar.ts index 414b2510c..bce87544d 100644 --- a/packages/widgets/src/menubar.ts +++ b/packages/widgets/src/menubar.ts @@ -153,19 +153,6 @@ export class MenuBar extends Widget { // Update the active index. this._activeIndex = value; - // Update the focus index. - if (value !== -1) { - this._tabFocusIndex = value; - } - - // Update focus to new active index - if ( - this._activeIndex >= 0 && - this.contentNode.childNodes[this._activeIndex] - ) { - (this.contentNode.childNodes[this._activeIndex] as HTMLElement).focus(); - } - // Schedule an update of the items. this.update(); } @@ -370,8 +357,8 @@ export class MenuBar extends Widget { case 'mousemove': this._evtMouseMove(event as MouseEvent); break; - case 'mouseleave': - this._evtMouseLeave(event as MouseEvent); + case 'focusout': + this._evtFocusOut(event as FocusEvent); break; case 'contextmenu': event.preventDefault(); @@ -387,7 +374,7 @@ export class MenuBar extends Widget { this.node.addEventListener('keydown', this); this.node.addEventListener('mousedown', this); this.node.addEventListener('mousemove', this); - this.node.addEventListener('mouseleave', this); + this.node.addEventListener('focusout', this); this.node.addEventListener('contextmenu', this); } @@ -398,7 +385,7 @@ export class MenuBar extends Widget { this.node.removeEventListener('keydown', this); this.node.removeEventListener('mousedown', this); this.node.removeEventListener('mousemove', this); - this.node.removeEventListener('mouseleave', this); + this.node.removeEventListener('focusout', this); this.node.removeEventListener('contextmenu', this); this._closeChildMenu(); } @@ -408,7 +395,7 @@ export class MenuBar extends Widget { */ protected onActivateRequest(msg: Message): void { if (this.isAttached) { - this.activeIndex = 0; + this._focusItemAt(0); } } @@ -446,6 +433,7 @@ export class MenuBar extends Widget { active: i === activeIndex && menus[i].items.length !== 0, tabbable: i === tabFocusIndex, onfocus: () => { + this._tabFocusIndex = i; this.activeIndex = i; } }); @@ -483,6 +471,7 @@ export class MenuBar extends Widget { active: length === activeIndex && menus[length].items.length !== 0, tabbable: length === tabFocusIndex, onfocus: () => { + this._tabFocusIndex = length; this.activeIndex = length; } }); @@ -503,6 +492,7 @@ export class MenuBar extends Widget { active: false, tabbable: length === tabFocusIndex, onfocus: () => { + this._tabFocusIndex = length; this.activeIndex = length; } }); @@ -582,6 +572,9 @@ export class MenuBar extends Widget { // Enter, Space, Up Arrow, Down Arrow if (kc === 13 || kc === 32 || kc === 38 || kc === 40) { + // The active index may have changed (for example, user hovers over an + // item with the mouse), so be sure to use the focus index. + this.activeIndex = this._tabFocusIndex; this.openActiveMenu(); return; } @@ -589,8 +582,7 @@ export class MenuBar extends Widget { // Escape if (kc === 27) { this._closeChildMenu(); - this.activeIndex = -1; - this.node.blur(); + this._focusItemAt(this.activeIndex); return; } @@ -598,7 +590,7 @@ export class MenuBar extends Widget { if (kc === 37) { let i = this._activeIndex; let n = this._menus.length; - this.activeIndex = i === 0 ? n - 1 : i - 1; + this._focusItemAt(i === 0 ? n - 1 : i - 1); return; } @@ -606,7 +598,7 @@ export class MenuBar extends Widget { if (kc === 39) { let i = this._activeIndex; let n = this._menus.length; - this.activeIndex = i === n - 1 ? 0 : i + 1; + this._focusItemAt(i === n - 1 ? 0 : i + 1); return; } @@ -631,8 +623,10 @@ export class MenuBar extends Widget { this.openActiveMenu(); } else if (result.index !== -1) { this.activeIndex = result.index; + this._focusItemAt(this.activeIndex); } else if (result.auto !== -1) { this.activeIndex = result.auto; + this._focusItemAt(this.activeIndex); } } @@ -648,7 +642,6 @@ export class MenuBar extends Widget { // Stop the propagation of the event. Immediate propagation is // also stopped so that an open menu does not handle the event. - event.preventDefault(); event.stopPropagation(); event.stopImmediatePropagation(); @@ -673,6 +666,9 @@ export class MenuBar extends Widget { this._closeChildMenu(); this.activeIndex = index; } else { + // If we don't call preventDefault() here, then the item in the menu + // bar will take focus over the menu that is being opened. + // event.preventDefault(); const position = this._positionForMenu(index); Menu.saveWindowData(); // Begin DOM modifications. @@ -737,15 +733,28 @@ export class MenuBar extends Widget { } /** - * Handle the `'mouseleave'` event for the menu bar. + * Handle the `'focusout'` event for the menu bar. */ - private _evtMouseLeave(event: MouseEvent): void { - // Reset the active index if there is no open menu. - if (!this._childMenu) { + private _evtFocusOut(event: FocusEvent): void { + // Reset the active index if there is no open menu and the menubar is losing focus. + if (!this._childMenu && !this.node.contains(event.relatedTarget as Node)) { this.activeIndex = -1; } } + /** + * Focus an item in the menu bar. + * + * #### Notes + * Does not open the associated menu. + */ + private _focusItemAt(index: number): void { + const itemNode = this.contentNode.childNodes[index] as HTMLElement | void; + if (itemNode) { + itemNode.focus(); + } + } + /** * Open the child menu at the active index immediately. * @@ -776,6 +785,11 @@ export class MenuBar extends Widget { document.addEventListener('mousedown', this, true); } + // Update the tab focus index. Note: it's important to follow this with an + // update message so that the tabIndex value on each menubar item gets + // properly updated. + this._tabFocusIndex = this.activeIndex; + // Ensure the menu bar is updated and look up the item node. MessageLoop.sendMessage(this, Widget.Msg.UpdateRequest); @@ -880,7 +894,7 @@ export class MenuBar extends Widget { this.update(); } - // Track the index of the item that is currently focused. -1 means nothing focused. + // Track the index of the item that is currently focused or hovered. -1 means nothing focused or hovered. private _activeIndex = -1; // Track which item can be focused using the TAB key. Unlike _activeIndex will always point to a menuitem. private _tabFocusIndex = 0; diff --git a/packages/widgets/tests/src/menubar.spec.ts b/packages/widgets/tests/src/menubar.spec.ts index 50e5b7669..789513c7c 100644 --- a/packages/widgets/tests/src/menubar.spec.ts +++ b/packages/widgets/tests/src/menubar.spec.ts @@ -768,16 +768,16 @@ describe('@lumino/widgets', () => { }); }); - context('mouseleave', () => { + context('focusout', () => { it('should reset the active index if there is no open menu', () => { - bar.node.dispatchEvent(new MouseEvent('mouseleave', { bubbles })); + bar.node.dispatchEvent(new FocusEvent('focusout', { bubbles })); expect(bar.activeIndex).to.equal(-1); }); it('should be a no-op if there is an open menu', () => { bar.openActiveMenu(); let menu = bar.activeMenu!; - bar.node.dispatchEvent(new MouseEvent('mouseleave', { bubbles })); + bar.node.dispatchEvent(new FocusEvent('focusous', { bubbles })); expect(bar.activeIndex).to.equal(0); expect(menu.isAttached).to.equal(true); }); @@ -794,7 +794,8 @@ describe('@lumino/widgets', () => { context('focus', () => { it('should lose focus on tab key', () => { let bar = createUnfocusedMenuBar(); - bar.activeIndex = 0; + bar.activate(); + MessageLoop.flush(); expect(bar.contentNode.contains(document.activeElement)).to.equal( true ); @@ -806,7 +807,8 @@ describe('@lumino/widgets', () => { it('should lose focus on shift-tab key', () => { let bar = createUnfocusedMenuBar(); - bar.activeIndex = 0; + bar.activate(); + MessageLoop.flush(); expect(bar.contentNode.contains(document.activeElement)).to.equal( true ); @@ -834,8 +836,8 @@ describe('@lumino/widgets', () => { expect(bar.events.indexOf('mousedown')).to.not.equal(-1); node.dispatchEvent(new MouseEvent('mousemove', { bubbles })); expect(bar.events.indexOf('mousemove')).to.not.equal(-1); - node.dispatchEvent(new MouseEvent('mouseleave', { bubbles })); - expect(bar.events.indexOf('mouseleave')).to.not.equal(-1); + node.dispatchEvent(new FocusEvent('focusout', { bubbles })); + expect(bar.events.indexOf('focusout')).to.not.equal(-1); node.dispatchEvent(new MouseEvent('contextmenu', { bubbles })); expect(bar.events.indexOf('contextmenu')).to.not.equal(-1); bar.dispose(); @@ -855,8 +857,8 @@ describe('@lumino/widgets', () => { expect(bar.events.indexOf('mousedown')).to.equal(-1); node.dispatchEvent(new MouseEvent('mousemove', { bubbles })); expect(bar.events.indexOf('mousemove')).to.equal(-1); - node.dispatchEvent(new MouseEvent('mouseleave', { bubbles })); - expect(bar.events.indexOf('mouseleave')).to.equal(-1); + node.dispatchEvent(new FocusEvent('focusout', { bubbles })); + expect(bar.events.indexOf('focusout')).to.equal(-1); node.dispatchEvent(new MouseEvent('contextmenu', { bubbles })); expect(bar.events.indexOf('contextmenu')).to.equal(-1); bar.dispose(); From 50954e9f5176a721899b44d1f8e5b54a08e2cfdc Mon Sep 17 00:00:00 2001 From: Gabriel Fouasnon Date: Thu, 27 Jul 2023 14:38:56 +0300 Subject: [PATCH 02/13] do not allow empty menu to be active --- packages/default-theme/style/menubar.css | 7 +- packages/widgets/src/menubar.ts | 64 ++++++++---- packages/widgets/tests/src/menubar.spec.ts | 112 +++++++++++++++++++-- 3 files changed, 151 insertions(+), 32 deletions(-) diff --git a/packages/default-theme/style/menubar.css b/packages/default-theme/style/menubar.css index 8d80d62ba..3b49f32f1 100644 --- a/packages/default-theme/style/menubar.css +++ b/packages/default-theme/style/menubar.css @@ -33,8 +33,7 @@ border-right: 1px solid transparent; } -.lm-MenuBar-item.lm-mod-active, -.lm-MenuBar-item:hover { +.lm-MenuBar-item.lm-mod-active { background: #e5e5e5; } @@ -53,4 +52,8 @@ .lm-MenuBar-item:focus-visible { outline: 2px solid #333; outline-offset: -2px; +} + +.lm-MenuBar-item[aria-disabled="true"] { + color: rgba(0, 0, 0, 0.37); } \ No newline at end of file diff --git a/packages/widgets/src/menubar.ts b/packages/widgets/src/menubar.ts index bce87544d..760560fb7 100644 --- a/packages/widgets/src/menubar.ts +++ b/packages/widgets/src/menubar.ts @@ -145,6 +145,11 @@ export class MenuBar extends Widget { value = -1; } + // An empty menu cannot be active + if (value > -1 && this._menus[value].items.length === 0) { + value = -1; + } + // Bail early if the index will not change. if (this._activeIndex === value) { return; @@ -430,8 +435,9 @@ export class MenuBar extends Widget { for (let i = 0; i < length; ++i) { content[i] = renderer.renderItem({ title: menus[i].title, - active: i === activeIndex && menus[i].items.length !== 0, + active: i === activeIndex, tabbable: i === tabFocusIndex, + disabled: menus[i].items.length === 0, onfocus: () => { this._tabFocusIndex = i; this.activeIndex = i; @@ -470,6 +476,7 @@ export class MenuBar extends Widget { title: this._overflowMenu.title, active: length === activeIndex && menus[length].items.length !== 0, tabbable: length === tabFocusIndex, + disabled: menus[length].items.length === 0, onfocus: () => { this._tabFocusIndex = length; this.activeIndex = length; @@ -491,6 +498,7 @@ export class MenuBar extends Widget { title: menu.title, active: false, tabbable: length === tabFocusIndex, + disabled: menus[length].items.length === 0, onfocus: () => { this._tabFocusIndex = length; this.activeIndex = length; @@ -575,6 +583,12 @@ export class MenuBar extends Widget { // The active index may have changed (for example, user hovers over an // item with the mouse), so be sure to use the focus index. this.activeIndex = this._tabFocusIndex; + if (this.activeIndex !== this._tabFocusIndex) { + // Bail if the setter refused to set activeIndex to tabFocusIndex + // because it means that the item at tabFocusIndex cannot be opened (for + // example, it has an empty menu) + return; + } this.openActiveMenu(); return; } @@ -586,19 +600,18 @@ export class MenuBar extends Widget { return; } - // Left Arrow - if (kc === 37) { - let i = this._activeIndex; - let n = this._menus.length; - this._focusItemAt(i === 0 ? n - 1 : i - 1); - return; - } - - // Right Arrow - if (kc === 39) { - let i = this._activeIndex; + // Left or Right Arrow + if (kc === 37 || kc === 39) { + let direction = kc === 37 ? -1 : 1; + let start = this._tabFocusIndex + direction; let n = this._menus.length; - this._focusItemAt(i === n - 1 ? 0 : i + 1); + for (let i = 0; i < n; i++) { + let index = (n + start + direction * i) % n; + if (this._menus[index].items.length) { + this._focusItemAt(index); + return; + } + } return; } @@ -668,7 +681,7 @@ export class MenuBar extends Widget { } else { // If we don't call preventDefault() here, then the item in the menu // bar will take focus over the menu that is being opened. - // event.preventDefault(); + event.preventDefault(); const position = this._positionForMenu(index); Menu.saveWindowData(); // Begin DOM modifications. @@ -786,8 +799,8 @@ export class MenuBar extends Widget { } // Update the tab focus index. Note: it's important to follow this with an - // update message so that the tabIndex value on each menubar item gets - // properly updated. + // "update-request" message so that the `tabindex` attribute on each menubar + // item gets properly updated. this._tabFocusIndex = this.activeIndex; // Ensure the menu bar is updated and look up the item node. @@ -961,6 +974,15 @@ export namespace MenuBar { */ readonly tabbable: boolean; + /** + * Whether the item is disabled. + * + * #### Notes + * A disabled item cannot be active. + * A disabled item cannot be focussed. + */ + readonly disabled: boolean; + readonly onfocus?: (event: FocusEvent) => void; } @@ -1000,7 +1022,7 @@ export namespace MenuBar { { className, dataset, - tabindex: data.tabbable ? '0' : '-1', + ...(data.disabled ? {} : { tabindex: data.tabbable ? '0' : '-1' }), onfocus: data.onfocus, ...aria }, @@ -1047,7 +1069,7 @@ export namespace MenuBar { if (data.title.className) { name += ` ${data.title.className}`; } - if (data.active) { + if (data.active && !data.disabled) { name += ' lm-mod-active'; } return name; @@ -1072,7 +1094,11 @@ export namespace MenuBar { * @returns The aria attributes object for the item. */ createItemARIA(data: IRenderData): ElementARIAAttrs { - return { role: 'menuitem', 'aria-haspopup': 'true' }; + return { + role: 'menuitem', + 'aria-haspopup': 'true', + 'aria-disabled': data.disabled ? 'true' : 'false' + }; } /** diff --git a/packages/widgets/tests/src/menubar.spec.ts b/packages/widgets/tests/src/menubar.spec.ts index 789513c7c..e4a1b0d3e 100644 --- a/packages/widgets/tests/src/menubar.spec.ts +++ b/packages/widgets/tests/src/menubar.spec.ts @@ -173,6 +173,9 @@ describe('@lumino/widgets', () => { it('should get the child menu of the menu bar', () => { let bar = new MenuBar(); let menu = new Menu({ commands }); + // Add item to menu because MenuBar will not allow an empty menu to be + // active or opened. + menu.addItem({ command: DEFAULT_CMD }); bar.addMenu(menu); bar.activeIndex = 0; bar.openActiveMenu(); @@ -203,6 +206,9 @@ describe('@lumino/widgets', () => { it('should get the active menu of the menu bar', () => { let bar = new MenuBar(); let menu = new Menu({ commands }); + // Add item to menu because MenuBar will not allow an empty menu to be + // active or opened. + menu.addItem({ command: DEFAULT_CMD }); bar.addMenu(menu); bar.activeIndex = 0; expect(bar.activeMenu).to.equal(menu); @@ -220,6 +226,9 @@ describe('@lumino/widgets', () => { it('should set the currently active menu', () => { let bar = new MenuBar(); let menu = new Menu({ commands }); + // Add item to menu because MenuBar will not allow an empty menu to be + // active or opened. + menu.addItem({ command: DEFAULT_CMD }); bar.addMenu(menu); bar.activeMenu = menu; expect(bar.activeMenu).to.equal(menu); @@ -239,6 +248,9 @@ describe('@lumino/widgets', () => { it('should get the index of the currently active menu', () => { let bar = new MenuBar(); let menu = new Menu({ commands }); + // Add item to menu because MenuBar will not allow an empty menu to be + // active or opened. + menu.addItem({ command: DEFAULT_CMD }); bar.addMenu(menu); bar.activeMenu = menu; expect(bar.activeIndex).to.equal(0); @@ -256,6 +268,9 @@ describe('@lumino/widgets', () => { it('should set the index of the currently active menu', () => { let bar = new MenuBar(); let menu = new Menu({ commands }); + // Add item to menu because MenuBar will not allow an empty menu to be + // active or opened. + menu.addItem({ command: DEFAULT_CMD }); bar.addMenu(menu); bar.activeIndex = 0; expect(bar.activeIndex).to.equal(0); @@ -280,6 +295,15 @@ describe('@lumino/widgets', () => { expect(bar.activeIndex).to.equal(0); bar.dispose(); }); + + it('should set to `-1` if menu at index is empty', () => { + let bar = createMenuBar(); + let emptyMenu = new Menu({ commands }); + bar.insertMenu(1, emptyMenu); + bar.activeIndex = 1; + expect(bar.activeIndex).to.equal(-1); + bar.dispose(); + }); }); describe('#menus', () => { @@ -569,6 +593,24 @@ describe('@lumino/widgets', () => { expect(bar.activeIndex!).to.equal(1); }); + it('should skip past empty menu on Left Arrow', () => { + let emptyMenu = new Menu({ commands }); + bar.insertMenu(0, emptyMenu); + // Update DOM so we can focus the item node. + MessageLoop.flush(); + // Call .focus() because keyboard shortcuts work from the item that + // has focus which may not match `bar.activeIndex`. Focus the item + // just to the right of the item that matches the empty menu. + (bar.contentNode.children[1] as HTMLElement).focus(); + bar.node.dispatchEvent( + new KeyboardEvent('keydown', { + bubbles, + keyCode: 37 + }) + ); + expect(bar.activeIndex).to.equal(bar.menus.length - 1); + }); + it('should activate the next menu on Right Arrow', () => { bar.node.dispatchEvent( new KeyboardEvent('keydown', { @@ -593,6 +635,25 @@ describe('@lumino/widgets', () => { expect(bar.activeIndex!).to.equal(0); }); + it('should skip past empty menu on Right Arrow', () => { + let emptyMenu = new Menu({ commands }); + bar.addMenu(emptyMenu); + // Update DOM so we can focus the item node. + MessageLoop.flush(); + let emptyIndex = bar.menus.indexOf(emptyMenu); + // Call .focus() because keyboard shortcuts work from the item that + // has focus which may not match `bar.activeIndex`. Focus the item + // just to the left of the item that matches the empty menu. + (bar.contentNode.children[emptyIndex - 1] as HTMLElement).focus(); + bar.node.dispatchEvent( + new KeyboardEvent('keydown', { + bubbles, + keyCode: 39 + }) + ); + expect(bar.activeIndex).to.equal(0); + }); + it('should open the menu matching a mnemonic', () => { bar.node.dispatchEvent( new KeyboardEvent('keydown', { @@ -636,6 +697,9 @@ describe('@lumino/widgets', () => { it('should select the only menu matching the first letter', () => { let menu = new Menu({ commands }); + // Add item to menu because MenuBar will not allow an empty menu to be + // active or opened. + menu.addItem({ command: DEFAULT_CMD }); menu.title.label = 'Test1'; bar.addMenu(menu); bar.addMenu(new Menu({ commands })); @@ -646,8 +710,8 @@ describe('@lumino/widgets', () => { }) ); expect(bar.activeIndex).to.equal(3); - menu = bar.activeMenu!; - expect(menu.isAttached).to.equal(false); + expect(bar.activeMenu).to.equal(menu); + expect(bar.activeMenu!.isAttached).to.equal(false); }); }); @@ -668,6 +732,26 @@ describe('@lumino/widgets', () => { it('should close an active menu', () => { bar.openActiveMenu(); + let menu = bar.activeMenu!; + let firstItemNode = bar.node.getElementsByClassName( + 'lm-MenuBar-item' + )[0] as HTMLElement; + let rect = firstItemNode.getBoundingClientRect(); + let mouseEvent = new MouseEvent('mousedown', { + bubbles, + clientX: rect.left, + clientY: rect.top + }); + expect(bar.activeIndex).to.equal(0); + bar.node.dispatchEvent(mouseEvent); + expect(bar.activeIndex).to.equal(0); + expect(menu.isAttached).to.equal(false); + // Ensure that mousedown default is not prevented, to allow browser + // focus to go to an item that a user clicks in the menu bar. + expect(mouseEvent.defaultPrevented).to.equal(false); + }); + + it('should open an active menu', () => { let menu = bar.activeMenu!; let node = bar.node.getElementsByClassName( 'lm-MenuBar-item' @@ -681,10 +765,11 @@ describe('@lumino/widgets', () => { }) ); expect(bar.activeIndex).to.equal(0); - expect(menu.isAttached).to.equal(false); + expect(menu.isAttached).to.equal(true); }); - it('should open an active menu', () => { + it('should not close an active menu if not a left mouse press', () => { + bar.openActiveMenu(); let menu = bar.activeMenu!; let node = bar.node.getElementsByClassName( 'lm-MenuBar-item' @@ -693,6 +778,7 @@ describe('@lumino/widgets', () => { bar.node.dispatchEvent( new MouseEvent('mousedown', { bubbles, + button: 1, clientX: rect.left, clientY: rect.top }) @@ -701,9 +787,12 @@ describe('@lumino/widgets', () => { expect(menu.isAttached).to.equal(true); }); - it('should not close an active menu if not a left mouse press', () => { - bar.openActiveMenu(); - let menu = bar.activeMenu!; + it('should not work on a menu bar item whose menu is empty', () => { + let emptyMenu = new Menu({ commands }); + // Add title to empty menu, otherwise it will have zero width in the + // menu bar, which makes it impossible to test with mousedown. + emptyMenu.title.label = 'Empty Menu' + bar.insertMenu(0, emptyMenu); let node = bar.node.getElementsByClassName( 'lm-MenuBar-item' )[0] as HTMLElement; @@ -711,13 +800,13 @@ describe('@lumino/widgets', () => { bar.node.dispatchEvent( new MouseEvent('mousedown', { bubbles, - button: 1, + button: 0, clientX: rect.left, clientY: rect.top }) ); - expect(bar.activeIndex).to.equal(0); - expect(menu.isAttached).to.equal(true); + expect(bar.activeIndex).to.equal(-1); + expect(emptyMenu.isAttached).to.equal(false); }); }); @@ -990,7 +1079,8 @@ describe('@lumino/widgets', () => { data = { title: widget.title, active: true, - tabbable: true + tabbable: true, + disabled: false }; }); From c4b4a3881c8199dae8c0c030608ecc783f766f25 Mon Sep 17 00:00:00 2001 From: Gabriel Fouasnon Date: Thu, 27 Jul 2023 14:52:45 +0300 Subject: [PATCH 03/13] remove underline --- packages/default-theme/style/menubar.css | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/default-theme/style/menubar.css b/packages/default-theme/style/menubar.css index 3b49f32f1..2042cbab2 100644 --- a/packages/default-theme/style/menubar.css +++ b/packages/default-theme/style/menubar.css @@ -45,10 +45,6 @@ box-shadow: 0px 0px 6px rgba(0, 0, 0, 0.2); } -.lm-MenuBar-item:focus { - text-decoration: underline; -} - .lm-MenuBar-item:focus-visible { outline: 2px solid #333; outline-offset: -2px; From bb793c64dddeae3dfb32e8b9849cdf2cc2de3568 Mon Sep 17 00:00:00 2001 From: Gabriel Fouasnon Date: Thu, 27 Jul 2023 14:53:20 +0300 Subject: [PATCH 04/13] newline --- packages/default-theme/style/menubar.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/default-theme/style/menubar.css b/packages/default-theme/style/menubar.css index 2042cbab2..46aced891 100644 --- a/packages/default-theme/style/menubar.css +++ b/packages/default-theme/style/menubar.css @@ -52,4 +52,4 @@ .lm-MenuBar-item[aria-disabled="true"] { color: rgba(0, 0, 0, 0.37); -} \ No newline at end of file +} From 60c3f00b3101b1e78861c09a75350e07098af9da Mon Sep 17 00:00:00 2001 From: Gabriel Fouasnon Date: Thu, 27 Jul 2023 15:03:14 +0300 Subject: [PATCH 05/13] return test line --- packages/widgets/tests/src/menubar.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/widgets/tests/src/menubar.spec.ts b/packages/widgets/tests/src/menubar.spec.ts index e4a1b0d3e..c093438d3 100644 --- a/packages/widgets/tests/src/menubar.spec.ts +++ b/packages/widgets/tests/src/menubar.spec.ts @@ -711,7 +711,7 @@ describe('@lumino/widgets', () => { ); expect(bar.activeIndex).to.equal(3); expect(bar.activeMenu).to.equal(menu); - expect(bar.activeMenu!.isAttached).to.equal(false); + expect(menu.isAttached).to.equal(false); }); }); From ce2286ed86147de6eb98480bf0baf0f4c506940e Mon Sep 17 00:00:00 2001 From: Gabriel Fouasnon Date: Thu, 27 Jul 2023 15:34:27 +0300 Subject: [PATCH 06/13] fix prevent default --- packages/widgets/tests/src/menubar.spec.ts | 33 +++++++++++++++------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/packages/widgets/tests/src/menubar.spec.ts b/packages/widgets/tests/src/menubar.spec.ts index c093438d3..ef4bd587c 100644 --- a/packages/widgets/tests/src/menubar.spec.ts +++ b/packages/widgets/tests/src/menubar.spec.ts @@ -513,7 +513,10 @@ describe('@lumino/widgets', () => { context('keydown', () => { it('should bail on Tab', () => { - let event = new KeyboardEvent('keydown', { keyCode: 9 }); + let event = new KeyboardEvent('keydown', { + cancelable: true, + keyCode: 9 + }); bar.node.dispatchEvent(event); expect(event.defaultPrevented).to.equal(false); }); @@ -717,7 +720,11 @@ describe('@lumino/widgets', () => { context('mousedown', () => { it('should bail if the mouse press was not on the menu bar', () => { - let event = new MouseEvent('mousedown', { bubbles, clientX: -10 }); + let event = new MouseEvent('mousedown', { + bubbles, + cancelable: true, + clientX: -10 + }); bar.node.dispatchEvent(event); expect(event.defaultPrevented).to.equal(false); }); @@ -739,6 +746,7 @@ describe('@lumino/widgets', () => { let rect = firstItemNode.getBoundingClientRect(); let mouseEvent = new MouseEvent('mousedown', { bubbles, + cancelable: true, clientX: rect.left, clientY: rect.top }); @@ -757,15 +765,20 @@ describe('@lumino/widgets', () => { 'lm-MenuBar-item' )[0] as HTMLElement; let rect = node.getBoundingClientRect(); - bar.node.dispatchEvent( - new MouseEvent('mousedown', { - bubbles, - clientX: rect.left, - clientY: rect.top - }) - ); + let mouseEvent = new MouseEvent('mousedown', { + bubbles, + cancelable: true, + clientX: rect.left, + clientY: rect.top + }); + bar.node.dispatchEvent(mouseEvent); expect(bar.activeIndex).to.equal(0); expect(menu.isAttached).to.equal(true); + // When opening a menu, be sure to prevent default during the + // mousedown so that the item being clicked in the menu bar does not + // "steal" focus from the menu being opened. + mouseEvent.preventDefault(); + expect(mouseEvent.defaultPrevented).to.equal(true); }); it('should not close an active menu if not a left mouse press', () => { @@ -791,7 +804,7 @@ describe('@lumino/widgets', () => { let emptyMenu = new Menu({ commands }); // Add title to empty menu, otherwise it will have zero width in the // menu bar, which makes it impossible to test with mousedown. - emptyMenu.title.label = 'Empty Menu' + emptyMenu.title.label = 'Empty Menu'; bar.insertMenu(0, emptyMenu); let node = bar.node.getElementsByClassName( 'lm-MenuBar-item' From 06a82c25ad3283cbaa2f057e4df3235763d00fa7 Mon Sep 17 00:00:00 2001 From: Gabriel Fouasnon Date: Thu, 27 Jul 2023 15:44:43 +0300 Subject: [PATCH 07/13] reduce repeated comments --- packages/widgets/tests/src/menubar.spec.ts | 40 ++++++++-------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/packages/widgets/tests/src/menubar.spec.ts b/packages/widgets/tests/src/menubar.spec.ts index ef4bd587c..67ef03eb9 100644 --- a/packages/widgets/tests/src/menubar.spec.ts +++ b/packages/widgets/tests/src/menubar.spec.ts @@ -57,12 +57,17 @@ describe('@lumino/widgets', () => { let commands: CommandRegistry; + function createMenu() { + let menu = new Menu({ commands }); + // Several tests rely on this function returning a non-empty menu + menu.addItem({ command: DEFAULT_CMD }); + return menu; + } + function createMenuBar(options?: MenuBar.IOptions): MenuBar { let bar = new MenuBar(options); for (let i = 0; i < 3; i++) { - let menu = new Menu({ commands }); - let item = menu.addItem({ command: DEFAULT_CMD }); - menu.addItem(item); + let menu = createMenu(); menu.title.label = `Menu${i}`; menu.title.mnemonic = 4; bar.addMenu(menu); @@ -172,10 +177,7 @@ describe('@lumino/widgets', () => { describe('#childMenu', () => { it('should get the child menu of the menu bar', () => { let bar = new MenuBar(); - let menu = new Menu({ commands }); - // Add item to menu because MenuBar will not allow an empty menu to be - // active or opened. - menu.addItem({ command: DEFAULT_CMD }); + let menu = createMenu(); bar.addMenu(menu); bar.activeIndex = 0; bar.openActiveMenu(); @@ -205,9 +207,7 @@ describe('@lumino/widgets', () => { describe('#activeMenu', () => { it('should get the active menu of the menu bar', () => { let bar = new MenuBar(); - let menu = new Menu({ commands }); - // Add item to menu because MenuBar will not allow an empty menu to be - // active or opened. + let menu = createMenu(); menu.addItem({ command: DEFAULT_CMD }); bar.addMenu(menu); bar.activeIndex = 0; @@ -225,10 +225,7 @@ describe('@lumino/widgets', () => { it('should set the currently active menu', () => { let bar = new MenuBar(); - let menu = new Menu({ commands }); - // Add item to menu because MenuBar will not allow an empty menu to be - // active or opened. - menu.addItem({ command: DEFAULT_CMD }); + let menu = createMenu(); bar.addMenu(menu); bar.activeMenu = menu; expect(bar.activeMenu).to.equal(menu); @@ -247,10 +244,7 @@ describe('@lumino/widgets', () => { describe('#activeIndex', () => { it('should get the index of the currently active menu', () => { let bar = new MenuBar(); - let menu = new Menu({ commands }); - // Add item to menu because MenuBar will not allow an empty menu to be - // active or opened. - menu.addItem({ command: DEFAULT_CMD }); + let menu = createMenu(); bar.addMenu(menu); bar.activeMenu = menu; expect(bar.activeIndex).to.equal(0); @@ -267,10 +261,7 @@ describe('@lumino/widgets', () => { it('should set the index of the currently active menu', () => { let bar = new MenuBar(); - let menu = new Menu({ commands }); - // Add item to menu because MenuBar will not allow an empty menu to be - // active or opened. - menu.addItem({ command: DEFAULT_CMD }); + let menu = createMenu(); bar.addMenu(menu); bar.activeIndex = 0; expect(bar.activeIndex).to.equal(0); @@ -699,10 +690,7 @@ describe('@lumino/widgets', () => { }); it('should select the only menu matching the first letter', () => { - let menu = new Menu({ commands }); - // Add item to menu because MenuBar will not allow an empty menu to be - // active or opened. - menu.addItem({ command: DEFAULT_CMD }); + let menu = createMenu(); menu.title.label = 'Test1'; bar.addMenu(menu); bar.addMenu(new Menu({ commands })); From 43540b154da2cc957c0a958be1b23681ad9f4acb Mon Sep 17 00:00:00 2001 From: Gabriel Fouasnon Date: Thu, 27 Jul 2023 15:47:11 +0300 Subject: [PATCH 08/13] missed a spot --- packages/widgets/tests/src/menubar.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/widgets/tests/src/menubar.spec.ts b/packages/widgets/tests/src/menubar.spec.ts index 67ef03eb9..69003ddd6 100644 --- a/packages/widgets/tests/src/menubar.spec.ts +++ b/packages/widgets/tests/src/menubar.spec.ts @@ -208,7 +208,6 @@ describe('@lumino/widgets', () => { it('should get the active menu of the menu bar', () => { let bar = new MenuBar(); let menu = createMenu(); - menu.addItem({ command: DEFAULT_CMD }); bar.addMenu(menu); bar.activeIndex = 0; expect(bar.activeMenu).to.equal(menu); From 4bf5a0b7621e7761506a22f04319efbfd2b4cecf Mon Sep 17 00:00:00 2001 From: Gabriel Fouasnon Date: Thu, 27 Jul 2023 15:51:22 +0300 Subject: [PATCH 09/13] move some comments around --- packages/widgets/src/menubar.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/widgets/src/menubar.ts b/packages/widgets/src/menubar.ts index 760560fb7..ddae8ee69 100644 --- a/packages/widgets/src/menubar.ts +++ b/packages/widgets/src/menubar.ts @@ -798,12 +798,8 @@ export class MenuBar extends Widget { document.addEventListener('mousedown', this, true); } - // Update the tab focus index. Note: it's important to follow this with an - // "update-request" message so that the `tabindex` attribute on each menubar - // item gets properly updated. + // Update the tab focus index and ensure the menu bar is updated. this._tabFocusIndex = this.activeIndex; - - // Ensure the menu bar is updated and look up the item node. MessageLoop.sendMessage(this, Widget.Msg.UpdateRequest); // Get the positioning data for the new menu. @@ -909,7 +905,10 @@ export class MenuBar extends Widget { // Track the index of the item that is currently focused or hovered. -1 means nothing focused or hovered. private _activeIndex = -1; - // Track which item can be focused using the TAB key. Unlike _activeIndex will always point to a menuitem. + // Track which item can be focused using the TAB key. Unlike _activeIndex will + // always point to a menuitem. Whenever you update this value, it's important + // to follow it with an "update-request" message so that the `tabindex` + // attribute on each menubar item gets properly updated. private _tabFocusIndex = 0; private _forceItemsPosition: Menu.IOpenOptions; private _overflowMenuOptions: IOverflowMenuOptions; From eb8750b426ec79a450821af5f4403ebaf8cf8cb1 Mon Sep 17 00:00:00 2001 From: Gabriel Fouasnon Date: Wed, 16 Aug 2023 12:31:52 +0300 Subject: [PATCH 10/13] yarn api && yarn lint --- packages/default-theme/style/menubar.css | 2 +- review/api/widgets.api.md | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/default-theme/style/menubar.css b/packages/default-theme/style/menubar.css index 46aced891..537ade11b 100644 --- a/packages/default-theme/style/menubar.css +++ b/packages/default-theme/style/menubar.css @@ -50,6 +50,6 @@ outline-offset: -2px; } -.lm-MenuBar-item[aria-disabled="true"] { +.lm-MenuBar-item[aria-disabled='true'] { color: rgba(0, 0, 0, 0.37); } diff --git a/review/api/widgets.api.md b/review/api/widgets.api.md index c11eab012..5cef33e2b 100644 --- a/review/api/widgets.api.md +++ b/review/api/widgets.api.md @@ -809,6 +809,7 @@ export namespace MenuBar { } export interface IRenderData { readonly active: boolean; + readonly disabled: boolean; // (undocumented) readonly onfocus?: (event: FocusEvent) => void; readonly tabbable: boolean; From 888038f062caf67263f50380aaa3cf78c9feff89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Collonval?= Date: Thu, 17 Aug 2023 18:01:38 +0200 Subject: [PATCH 11/13] Add menubar example to doc --- docs/source/conf.py | 2 +- docs/source/examples.rst | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/source/conf.py b/docs/source/conf.py index 8b904bd22..8e96444d7 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -34,7 +34,7 @@ HERE = osp.abspath(osp.dirname(__file__)) -EXAMPLES = ["accordionpanel", "datagrid", "dockpanel"] +EXAMPLES = ["accordionpanel", "datagrid", "dockpanel", "menubar"] # -- General configuration ------------------------------------------------ diff --git a/docs/source/examples.rst b/docs/source/examples.rst index 4008747ad..6581eb3b0 100644 --- a/docs/source/examples.rst +++ b/docs/source/examples.rst @@ -13,3 +13,5 @@ Rendered static examples `DataGrid `_ `DockPanel `_ + +`MenuBar `_ From e51598055ce1b6d7354e58daffd355b3a2805091 Mon Sep 17 00:00:00 2001 From: gabalafou Date: Thu, 17 Aug 2023 11:19:36 -0500 Subject: [PATCH 12/13] Update packages/widgets/src/menubar.ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Frédéric Collonval --- packages/widgets/src/menubar.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/widgets/src/menubar.ts b/packages/widgets/src/menubar.ts index ddae8ee69..9c7561aa6 100644 --- a/packages/widgets/src/menubar.ts +++ b/packages/widgets/src/menubar.ts @@ -980,7 +980,7 @@ export namespace MenuBar { * A disabled item cannot be active. * A disabled item cannot be focussed. */ - readonly disabled: boolean; + readonly disabled?: boolean; readonly onfocus?: (event: FocusEvent) => void; } From cc192179bcbace9b3b37a095a1b9d465bbce9684 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Collonval?= Date: Fri, 18 Aug 2023 16:20:14 +0200 Subject: [PATCH 13/13] Fix CI --- .github/workflows/tests.yml | 2 +- docs/source/conf.py | 1 - review/api/widgets.api.md | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 8d5b28e14..1f9957e9c 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -117,7 +117,7 @@ jobs: - uses: jupyterlab/maintainer-tools/.github/actions/base-setup@v1 - uses: jupyterlab/maintainer-tools/.github/actions/check-links@v1 with: - ignore_links: "./api/index.html examples/accordionpanel/index.html examples/datagrid/index.html examples/dockpanel/index.html https://mybinder.org/v2/gh/jupyterlab/lumino/.*" + ignore_links: "./api/index.html examples/accordionpanel/index.html examples/datagrid/index.html examples/dockpanel/index.html examples/menubar/index.html https://mybinder.org/v2/gh/jupyterlab/lumino/.*" check_release: runs-on: ubuntu-latest diff --git a/docs/source/conf.py b/docs/source/conf.py index 8e96444d7..890788ed1 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -50,7 +50,6 @@ 'sphinx.ext.intersphinx', 'sphinx.ext.mathjax', 'sphinx_copybutton', - ] myst_enable_extensions = ["html_image"] diff --git a/review/api/widgets.api.md b/review/api/widgets.api.md index 5cef33e2b..7462b947b 100644 --- a/review/api/widgets.api.md +++ b/review/api/widgets.api.md @@ -809,7 +809,7 @@ export namespace MenuBar { } export interface IRenderData { readonly active: boolean; - readonly disabled: boolean; + readonly disabled?: boolean; // (undocumented) readonly onfocus?: (event: FocusEvent) => void; readonly tabbable: boolean;