Skip to content

Commit

Permalink
do not allow empty menu to be active
Browse files Browse the repository at this point in the history
  • Loading branch information
@gabalafou
gabalafou committed Jul 27, 2023
1 parent 8b797a2 commit ff4a3c7
Show file tree
Hide file tree
Showing 3 changed files with 151 additions and 32 deletions.
7 changes: 5 additions & 2 deletions packages/default-theme/style/menubar.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@
border-right: 1px solid transparent;
}

.lm-MenuBar-item.lm-mod-active,
.lm-MenuBar-item:hover {
.lm-MenuBar-item.lm-mod-active {
background: #e5e5e5;
}

Expand All @@ -50,4 +49,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);
}
64 changes: 45 additions & 19 deletions packages/widgets/src/menubar.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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
},
Expand Down Expand Up @@ -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;
Expand All @@ -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'
};
}

/**
Expand Down
112 changes: 101 additions & 11 deletions packages/widgets/tests/src/menubar.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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', {
Expand All @@ -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', {
Expand Down Expand Up @@ -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 }));
Expand All @@ -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);
});
});

Expand All @@ -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'
Expand All @@ -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'
Expand All @@ -693,6 +778,7 @@ describe('@lumino/widgets', () => {
bar.node.dispatchEvent(
new MouseEvent('mousedown', {
bubbles,
button: 1,
clientX: rect.left,
clientY: rect.top
})
Expand All @@ -701,23 +787,26 @@ 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;
let rect = node.getBoundingClientRect();
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);
});
});

Expand Down Expand Up @@ -990,7 +1079,8 @@ describe('@lumino/widgets', () => {
data = {
title: widget.title,
active: true,
tabbable: true
tabbable: true,
disabled: false
};
});

Expand Down

0 comments on commit ff4a3c7

Please sign in to comment.