Skip to content

Commit a8c5445

Browse files
didip1000didip1000
authored
refactor(ui5-menu): remove references to parent (#11217)
Refactored `ui5-menu` closing and navigation handling, removing references to the parent element: - `MenuItems` now close their own submenus when necessary. - Exiting end content navigation with the up/down arrow keys with throw an exit-end-content event, which is handled by the parent `Menu` or `MenuItem`. - Fixed an issue with menu item navigation when holding down space on a `MenuItem` with a submenu.
1 parent 0ffbede commit a8c5445

File tree

5 files changed

+169
-70
lines changed

5 files changed

+169
-70
lines changed

‎packages/main/src/Menu.ts‎

Lines changed: 60 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
isLeft,
88
isRight,
99
isEnter,
10+
isSpace,
1011
isTabNext,
1112
isTabPrevious,
1213
isDown,
@@ -24,9 +25,10 @@ import type { Timeout } from "@ui5/webcomponents-base/dist/types.js";
2425
import { renderFinished } from "@ui5/webcomponents-base/dist/Render.js";
2526
import DOMReferenceConverter from "@ui5/webcomponents-base/dist/converters/DOMReference.js";
2627
import type ResponsivePopover from "./ResponsivePopover.js";
27-
import type PopoverHorizontalAlign from "./types/PopoverHorizontalAlign.js";
2828
import type MenuItem from "./MenuItem.js";
29-
import "./MenuItem.js";
29+
// The import below should be kept, as MenuItem is part of the Menu component.
30+
import { isInstanceOfMenuItem } from "./MenuItem.js";
31+
import type PopoverHorizontalAlign from "./types/PopoverHorizontalAlign.js";
3032
import "./MenuSeparator.js";
3133
import type {
3234
ListItemClickEventDetail,
@@ -294,30 +296,20 @@ class Menu extends UI5Element {
294296
item.selected = true;
295297
}
296298

297-
_closeItemSubMenu(item: MenuItem) {
298-
if (item && item._popover) {
299-
const openedSibling = item._menuItems.find(menuItem => menuItem._popover && menuItem._popover.open);
300-
if (openedSibling) {
301-
this._closeItemSubMenu(openedSibling);
302-
}
303-
304-
item._popover.open = false;
305-
item.selected = false;
299+
_itemMouseOver(e: MouseEvent) {
300+
if (!isDesktop()) {
301+
return;
306302
}
307-
}
308303

309-
_itemMouseOver(e: MouseEvent) {
310-
if (isDesktop()) {
311-
// respect mouseover only on desktop
312-
const item = e.target as MenuItem;
304+
const item = e.target as MenuItem;
305+
if (!isInstanceOfMenuItem(item)) {
306+
return;
307+
}
313308

314-
if (this._isInstanceOfMenuItem(item)) {
315-
item.focus();
309+
item.focus();
316310

317-
// Opens submenu with 300ms delay
318-
this._startOpenTimeout(item);
319-
}
320-
}
311+
// Opens submenu with 300ms delay
312+
this._startOpenTimeout(item);
321313
}
322314

323315
async focus(focusOptions?: FocusOptions): Promise<void> {
@@ -331,15 +323,24 @@ class Menu extends UI5Element {
331323
return super.focus(focusOptions);
332324
}
333325

326+
_closeOtherSubMenus(item: MenuItem) {
327+
const menuItems = this._menuItems;
328+
if (!menuItems.includes(item)) {
329+
return;
330+
}
331+
332+
menuItems.forEach(menuItem => {
333+
if (menuItem !== item) {
334+
menuItem._close();
335+
}
336+
});
337+
}
338+
334339
_startOpenTimeout(item: MenuItem) {
335340
clearTimeout(this._timeout);
336341

337342
this._timeout = setTimeout(() => {
338-
const opener = item.parentElement as MenuItem | Menu;
339-
const openedSibling = opener && opener._menuItems.find(menuItem => menuItem._popover && menuItem._popover.open);
340-
if (openedSibling) {
341-
this._closeItemSubMenu(openedSibling);
342-
}
343+
this._closeOtherSubMenus(item);
343344

344345
this._openItemSubMenu(item);
345346
}, MENU_OPEN_DELAY);
@@ -365,41 +366,46 @@ class Menu extends UI5Element {
365366
_itemKeyDown(e: KeyboardEvent) {
366367
const isTabNextPrevious = isTabNext(e) || isTabPrevious(e);
367368
const item = e.target as MenuItem;
368-
const parentElement = item.parentElement as MenuItem;
369-
const shouldItemNavigation = isUp(e) || isDown(e);
369+
370+
if (!isInstanceOfMenuItem(item)) {
371+
return;
372+
}
373+
374+
const menuItemInMenu = this._menuItems.includes(item);
375+
const isItemNavigation = isUp(e) || isDown(e);
376+
const isItemSelection = isEnter(e) || isSpace(e);
377+
const isEndContentNavigation = isRight(e) || isLeft(e);
370378
const shouldOpenMenu = this.isRtl ? isLeft(e) : isRight(e);
371-
const shouldCloseMenu = !shouldItemNavigation && !shouldOpenMenu && this._isInstanceOfMenuItem(parentElement);
379+
const shouldCloseMenu = menuItemInMenu && !(isItemNavigation || isItemSelection || isEndContentNavigation);
372380

373-
if (this._isInstanceOfMenuItem(item)) {
374-
if (isEnter(e) || isTabNextPrevious) {
375-
e.preventDefault();
376-
}
381+
if (isEnter(e) || isTabNextPrevious) {
382+
e.preventDefault();
383+
}
377384

378-
if (isRight(e) || isLeft(e)) {
379-
item._navigateToEndContent(isLeft(e));
380-
}
385+
if (isEndContentNavigation) {
386+
item._navigateToEndContent(isLeft(e));
387+
}
381388

382-
if (shouldOpenMenu) {
383-
this._openItemSubMenu(item);
384-
} else if ((shouldCloseMenu || isTabNextPrevious) && parentElement._popover) {
385-
parentElement._popover.open = false;
386-
parentElement.selected = false;
387-
parentElement._popover.focusOpener();
388-
}
389-
} else if (isUp(e)) {
390-
this._navigateOutOfEndContent(parentElement);
391-
} else if (isDown(e)) {
392-
this._navigateOutOfEndContent(parentElement, true);
389+
if (shouldOpenMenu) {
390+
this._openItemSubMenu(item);
391+
} else if ((shouldCloseMenu || isTabNextPrevious)) {
392+
this._close();
393393
}
394394
}
395395

396-
_navigateOutOfEndContent(menuItem: MenuItem, isDownwards?: boolean) {
397-
const opener = menuItem?.parentElement as MenuItem | Menu;
398-
const currentIndex = opener._menuItems.indexOf(menuItem);
399-
const nextItem = isDownwards ? opener._menuItems[currentIndex + 1] : opener._menuItems[currentIndex - 1];
400-
const itemToFocus = nextItem || opener._menuItems[currentIndex];
396+
_navigateOutOfEndContent(e: CustomEvent) {
397+
const item = e.target as MenuItem;
398+
const shouldNavigateToNextItem = e.detail.shouldNavigateToNextItem;
399+
const menuItems = this._menuItems;
400+
const itemIndex = menuItems.indexOf(item);
401401

402-
itemToFocus.focus();
402+
if (itemIndex > -1) {
403+
const nextItem = shouldNavigateToNextItem ? menuItems[itemIndex + 1] : menuItems[itemIndex - 1];
404+
const itemToFocus = nextItem || menuItems[itemIndex];
405+
itemToFocus?.focus();
406+
407+
e.stopPropagation();
408+
}
403409
}
404410

405411
_beforePopoverOpen(e: CustomEvent) {
@@ -429,10 +435,6 @@ class Menu extends UI5Element {
429435
this.open = false;
430436
this.fireDecoratorEvent("close");
431437
}
432-
433-
_isInstanceOfMenuItem(object: any): object is MenuItem {
434-
return "isMenuItem" in object;
435-
}
436438
}
437439

438440
Menu.define();

‎packages/main/src/MenuItem.ts‎

Lines changed: 103 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,17 @@ import type { AccessibilityAttributes, AriaHasPopup, AriaRole } from "@ui5/webco
55
import event from "@ui5/webcomponents-base/dist/decorators/event-strict.js";
66
import slot from "@ui5/webcomponents-base/dist/decorators/slot.js";
77
import i18n from "@ui5/webcomponents-base/dist/decorators/i18n.js";
8-
import { isPhone } from "@ui5/webcomponents-base/dist/Device.js";
8+
import {
9+
isLeft,
10+
isRight,
11+
isEnter,
12+
isSpace,
13+
isTabNext,
14+
isTabPrevious,
15+
isDown,
16+
isUp,
17+
} from "@ui5/webcomponents-base/dist/Keys.js";
18+
import { isDesktop, isPhone } from "@ui5/webcomponents-base/dist/Device.js";
919
import { renderFinished } from "@ui5/webcomponents-base/dist/Render.js";
1020
import "@ui5/webcomponents-icons/dist/nav-back.js";
1121
import type I18nBundle from "@ui5/webcomponents-base/dist/i18nBundle.js";
@@ -30,6 +40,8 @@ import menuItemCss from "./generated/themes/MenuItem.css.js";
3040
type MenuBeforeOpenEventDetail = { item?: MenuItem };
3141
type MenuBeforeCloseEventDetail = { escPressed: boolean };
3242

43+
type MenuNavigateOutOfEndContentEventDetail = { shouldNavigateToNextItem: boolean };
44+
3345
type MenuItemAccessibilityAttributes = Pick<AccessibilityAttributes, "ariaKeyShortcuts" | "role"> & ListItemAccessibilityAttributes;
3446

3547
/**
@@ -88,6 +100,14 @@ type MenuItemAccessibilityAttributes = Pick<AccessibilityAttributes, "ariaKeySho
88100
bubbles: true,
89101
})
90102

103+
/**
104+
* Fired when navigating out of end-content.
105+
* @private
106+
*/
107+
@event("exit-end-content", {
108+
bubbles: true,
109+
})
110+
91111
/**
92112
* Fired before the menu is closed. This event can be cancelled, which will prevent the menu from closing.
93113
* @public
@@ -110,7 +130,8 @@ class MenuItem extends ListItem implements IMenuItem {
110130
"open": void
111131
"before-close": MenuBeforeCloseEventDetail
112132
"close": void
113-
"close-menu": void
133+
"close-menu": void,
134+
"exit-end-content": MenuNavigateOutOfEndContentEventDetail,
114135
}
115136
/**
116137
* Defines the text of the tree item.
@@ -272,10 +293,11 @@ class MenuItem extends ListItem implements IMenuItem {
272293
});
273294
}
274295

275-
_navigateToEndContent(isLast?: boolean) {
276-
const item = isLast
277-
? this._navigableItems[this._navigableItems.length - 1]
278-
: this._navigableItems[0];
296+
_navigateToEndContent(shouldNavigateToPreviousItem: boolean) {
297+
const navigatableItems = this._navigableItems;
298+
const item = shouldNavigateToPreviousItem
299+
? navigatableItems[navigatableItems.length - 1]
300+
: navigatableItems[0];
279301

280302
if (item) {
281303
this._itemNavigation.setCurrentItem(item);
@@ -378,6 +400,72 @@ class MenuItem extends ListItem implements IMenuItem {
378400
return this.items.filter((item): item is MenuItem => !item.isSeparator);
379401
}
380402

403+
_closeOtherSubMenus(item: MenuItem) {
404+
const menuItems = this._menuItems;
405+
if (!menuItems.includes(item)) {
406+
return;
407+
}
408+
409+
menuItems.forEach(menuItem => {
410+
if (menuItem !== item) {
411+
menuItem._close();
412+
}
413+
});
414+
}
415+
416+
_itemMouseOver(e: MouseEvent) {
417+
if (!isDesktop()) {
418+
return;
419+
}
420+
const item = e.target as MenuItem;
421+
422+
if (!isInstanceOfMenuItem(item)) {
423+
return;
424+
}
425+
item.focus();
426+
427+
this._closeOtherSubMenus(item);
428+
}
429+
430+
_itemKeyDown(e: KeyboardEvent) {
431+
const item = e.target as MenuItem;
432+
const itemInMenuItems = this._menuItems.includes(item);
433+
const isTabNextPrevious = isTabNext(e) || isTabPrevious(e);
434+
const isItemNavigation = isUp(e) || isDown(e);
435+
const isItemSelection = isSpace(e) || isEnter(e);
436+
const shouldOpenMenu = this.isRtl ? isLeft(e) : isRight(e);
437+
const shouldCloseMenu = !(isItemNavigation || isItemSelection || shouldOpenMenu) || isTabNextPrevious;
438+
439+
if (itemInMenuItems && shouldCloseMenu) {
440+
this._close();
441+
this.focus();
442+
e.stopPropagation();
443+
}
444+
}
445+
446+
_endContentKeyDown(e: KeyboardEvent) {
447+
const shouldNavigateOutOfEndContent = isUp(e) || isDown(e);
448+
449+
if (shouldNavigateOutOfEndContent) {
450+
this.fireDecoratorEvent("exit-end-content", { shouldNavigateToNextItem: isDown(e) });
451+
}
452+
}
453+
454+
_navigateOutOfEndContent(e: CustomEvent) {
455+
const item = e.target as MenuItem;
456+
const shouldNavigateToNextItem = e.detail.shouldNavigateToNextItem;
457+
const menuItems = this._menuItems;
458+
const itemIndex = menuItems.indexOf(item);
459+
460+
if (itemIndex > -1) {
461+
const nextItem = shouldNavigateToNextItem ? menuItems[itemIndex + 1] : menuItems[itemIndex - 1];
462+
const itemToFocus = nextItem || menuItems[itemIndex];
463+
itemToFocus?.focus();
464+
465+
e.stopPropagation();
466+
}
467+
}
468+
381469
_closeAll() {
382470
if (this._popover) {
383471
this._popover.open = false;
@@ -389,6 +477,7 @@ class MenuItem extends ListItem implements IMenuItem {
389477
_close() {
390478
if (this._popover) {
391479
this._popover.open = false;
480+
this._menuItems.forEach(item => item._close());
392481
}
393482
this.selected = false;
394483
}
@@ -434,10 +523,18 @@ class MenuItem extends ListItem implements IMenuItem {
434523

435524
MenuItem.define();
436525

526+
const isInstanceOfMenuItem = (object: any): object is MenuItem => {
527+
return "isMenuItem" in object;
528+
};
529+
437530
export default MenuItem;
438531

439532
export type {
440533
MenuBeforeCloseEventDetail,
441534
MenuBeforeOpenEventDetail,
442535
MenuItemAccessibilityAttributes,
443536
};
537+
538+
export {
539+
isInstanceOfMenuItem,
540+
};

‎packages/main/src/MenuItemTemplate.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ function rightContent(this: MenuItem) {
4646
</div>
4747
);
4848
case this.hasEndContent:
49-
return <slot name="endContent"></slot>;
49+
return <slot name="endContent" onKeyDown={this._endContentKeyDown}></slot>;
5050
case !!this.additionalText:
5151
return (
5252
<span
@@ -123,8 +123,11 @@ function listItemPostContent(this: MenuItem) {
123123
accessibleRole="Menu"
124124
loading={this.loading}
125125
loadingDelay={this.loadingDelay}
126+
onMouseOver={this._itemMouseOver}
127+
onKeyDown={this._itemKeyDown}
126128
// handles event from slotted children
127129
onui5-close-menu={this._close}
130+
onui5-exit-end-content={this._navigateOutOfEndContent}
128131
>
129132
<slot></slot>
130133
</List>

‎packages/main/src/MenuTemplate.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ export default function MenuTemplate(this: Menu) {
5555
onKeyDown={this._itemKeyDown}
5656
// handles event from slotted children
5757
onui5-close-menu={this._close}
58+
onui5-exit-end-content={this._navigateOutOfEndContent}
5859
>
5960
<slot></slot>
6061
</List>)

‎packages/main/test/pages/Menu.html‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@
9494
<ui5-menu-separator></ui5-menu-separator>
9595
<ui5-menu-item text="Open" icon="open-folder" accessible-name="Choose platform">
9696
<ui5-menu-item text="Open Locally" icon="open-folder" additional-text="Ctrl+K">
97-
<ui5-menu-item text="Open from C"></ui5-menu-item>
97+
<ui5-menu-item text="Open from C" id="menuItemFromC"></ui5-menu-item>
9898
<ui5-menu-item text="Open from D"></ui5-menu-item>
9999
<ui5-menu-separator></ui5-menu-separator>
100100
<ui5-menu-item text="Open from E" disabled></ui5-menu-item>
@@ -298,10 +298,6 @@
298298
role: "menuitemcheckbox",
299299
};
300300

301-
menuItemFromD.accessibilityAttributes = {
302-
role: "menuitemcheckbox",
303-
};
304-
305301
</script>
306302
</body>
307303
</html>

0 commit comments

Comments
 (0)