Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(ui5-menu): adjust menu and sub-menu creation #8367

Merged
merged 5 commits into from
Mar 25, 2024
Merged

refactor(ui5-menu): adjust menu and sub-menu creation #8367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1de0f66
refactor(ui5-menu): adjust menu and sub-menu creation
unazko Feb 28, 2024
e1b21ae
refactor(ui5-menu): adjust menu and sub-menu creation
unazko Mar 1, 2024
f34d90d
refactor(ui5-menu): adjust menu and sub-menu creation
unazko Mar 5, 2024
e4c1447
refactor(ui5-menu): adjust menu and sub-menu creation
unazko Mar 5, 2024
631437b
refactor(ui5-menu): adjust menu and sub-menu creation
unazko Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/main/src/Menu.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
icon="decline"
design="Transparent"
aria-label="{{labelClose}}"
@click={{close}}
@click={{_closeAll}}
>
</ui5-button>
</div>
Expand Down Expand Up @@ -67,6 +67,7 @@
accessible-role="menuitem"
.additionalText="{{this.item._additionalText}}"
tooltip="{{this.item.tooltip}}"
selected="{{this.item.subMenuOpened}}"
?disabled={{this.item.disabled}}
?starts-section={{this.item.startsSection}}
?selected={{this.item.subMenuOpened}}
Expand Down
90 changes: 33 additions & 57 deletions packages/main/src/Menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
} from "@ui5/webcomponents-base/dist/Keys.js";
import {
isPhone,
isTablet,
isDesktop,
} from "@ui5/webcomponents-base/dist/Device.js";
import { getI18nBundle } from "@ui5/webcomponents-base/dist/i18nBundle.js";
Expand Down Expand Up @@ -265,13 +264,6 @@ class Menu extends UI5Element {
@property({ type: Object, multiple: true })
_currentItems!: Array<CurrentItem>;

/**
* Stores a list of parent menu items for each sub-menu (on phone).
* @private
*/
@property({ type: Object, multiple: true })
_parentItemsStack!: Array<MenuItem>;

/**
* Stores the ResponsivePopover instance
*/
Expand Down Expand Up @@ -347,15 +339,15 @@ class Menu extends UI5Element {
}

get isSubMenuOpened() {
return !!this._parentMenuItem;
return this._parentMenuItem && this._popover?.isOpen();
}

get menuHeaderTextPhone() {
return this._parentMenuItem ? this._parentMenuItem.text : this.headerText;
}

onBeforeRendering() {
!isPhone() && this._prepareCurrentItems(this.items);
this._prepareCurrentItems(this.items);

const itemsWithChildren = this.itemsWithChildren;
const itemsWithIcon = this.itemsWithIcon;
Expand Down Expand Up @@ -385,7 +377,7 @@ class Menu extends UI5Element {

if (this.open) {
const opener = this.getOpener();
if (opener) {
if (opener && !this.isSubMenuOpened) {
this.showAt(opener);
}
} else {
Expand All @@ -400,17 +392,14 @@ class Menu extends UI5Element {
* @public
*/
async showAt(opener: HTMLElement): Promise<void> {
if (isPhone()) {
this._prepareCurrentItems(this.items);
this._parentItemsStack = [];
}
if (!this._isSubMenu) {
this._parentMenuItem = undefined;
this._opener = undefined;
}
const busyWithoutItems = !this._parentMenuItem?.items.length && this._parentMenuItem?.busy;
const popover = await this._createPopover();
popover.initialFocus = `${this._id}-menu-item-0`;
popover.showAt(opener);
popover.showAt(opener, busyWithoutItems);
}

/**
Expand All @@ -419,17 +408,14 @@ class Menu extends UI5Element {
* @public
*/
close(): void {
if (this._popover) {
if (isPhone()) {
this._parentItemsStack = [];
}
this._popover.close(false, false, true);
}
this._popover?.close(false, false, true);
}

async _createPopover() {
const staticAreaItemDomRef = await this.getStaticAreaItemDomRef();
this._popover = staticAreaItemDomRef!.querySelector<ResponsivePopover>("[ui5-responsive-popover]")!;
if (!this._popover) {
const staticAreaItemDomRef = await this.getStaticAreaItemDomRef();
this._popover = staticAreaItemDomRef!.querySelector<ResponsivePopover>("[ui5-responsive-popover]")!;
}
return this._popover;
}

Expand All @@ -439,14 +425,12 @@ class Menu extends UI5Element {
}

_navigateBack() {
const parentMenuItem = this._parentItemsStack.pop();
this._closeItemSubMenu(this._parentMenuItem as MenuItem, true);
}

this.focus();
if (parentMenuItem) {
const parentMenuItemParent = parentMenuItem.parentElement as MenuItem;
this._prepareCurrentItems(parentMenuItemParent.items);
this._parentMenuItem = this._parentItemsStack.length ? this._parentItemsStack[this._parentItemsStack.length - 1] : undefined;
}
_closeAll() {
const mainMenu = this._findMainMenu(this);
mainMenu?.close();
}

_prepareCurrentItems(items: Array<MenuItem>) {
Expand All @@ -460,6 +444,9 @@ class Menu extends UI5Element {
}

_createSubMenu(item: MenuItem, opener: HTMLElement) {
if (item._subMenu) {
return;
}
const ctor = this.constructor as typeof Menu;
const subMenu = document.createElement(ctor.getMetadata().getTag()) as Menu;

Expand Down Expand Up @@ -488,7 +475,7 @@ class Menu extends UI5Element {

_openItemSubMenu(item: MenuItem, opener: HTMLElement) {
const mainMenu = this._findMainMenu(item);
mainMenu.fireEvent<MenuBeforeOpenEventDetail>("before-open", {
mainMenu?.fireEvent<MenuBeforeOpenEventDetail>("before-open", {
item,
}, false, false);
item._subMenu!.showAt(opener);
Expand All @@ -514,8 +501,6 @@ class Menu extends UI5Element {

if (forceClose || !parentItem._preventSubMenuClose) {
subMenu.close();
subMenu.remove();
parentItem._subMenu = undefined;
if (keyboard) {
subMenu._opener?.focus();
}
Expand All @@ -525,7 +510,7 @@ class Menu extends UI5Element {
}
}

_prepareSubMenuDesktopTablet(item: MenuItem, opener: HTMLElement) {
_prepareSubMenu(item: MenuItem, opener: HTMLElement) {
if (opener.id !== this._subMenuOpenerId || (item && item.hasSubmenu)) {
// close opened sub-menu if there is any opened
this._closeItemSubMenu(this._openedSubMenuItem!, true);
Expand All @@ -540,12 +525,6 @@ class Menu extends UI5Element {
}
}

_prepareSubMenuPhone(item: MenuItem) {
this._prepareCurrentItems(item.items);
this._parentMenuItem = item;
this._parentItemsStack.push(item);
}

_onfocusin(e: FocusEvent): void {
const target = e.target as HTMLElement;
const menuListItem = target.hasAttribute("ui5-menu-li")
Expand All @@ -561,7 +540,7 @@ class Menu extends UI5Element {

// Sets the new timeout
this._timeout = setTimeout(() => {
this._prepareSubMenuDesktopTablet(item, opener);
this._prepareSubMenu(item, opener);
}, MENU_OPEN_DELAY);
}

Expand Down Expand Up @@ -620,7 +599,7 @@ class Menu extends UI5Element {
const opener = e.target as OpenerStandardListItem;
const item = opener.associatedItem;

item.hasSubmenu && this._prepareSubMenuDesktopTablet(item, opener);
item.hasSubmenu && this._prepareSubMenu(item, opener);
} else if (shouldCloseMenu && this._isSubMenu && this._parentMenuItem) {
const parentMenuItemParent = this._parentMenuItem.parentElement as Menu;
parentMenuItemParent._closeItemSubMenu(this._parentMenuItem, true, true);
Expand All @@ -634,9 +613,6 @@ class Menu extends UI5Element {
if (!item.hasSubmenu) {
// click on an item that doesn't have sub-items fires an "item-click" event
if (!this._isSubMenu) {
if (isPhone()) {
this._parentMenuItem = undefined;
}
// fire event if the click is on top-level menu item
const prevented = !this.fireEvent<MenuItemClickEventDetail>("item-click", {
"item": item,
Expand Down Expand Up @@ -666,22 +642,22 @@ class Menu extends UI5Element {
mainMenu._popover!.close();
}
}
} else if (isPhone()) {
// prepares and opens sub-menu on phone
this._prepareSubMenuPhone(item);
} else if (isTablet()) {
// prepares and opens sub-menu on tablet
this._prepareSubMenuDesktopTablet(item, opener);
} else {
this._prepareSubMenu(item, opener);
}
}

_findMainMenu(item: MenuItem) {
let parentMenu = item.parentElement as Menu;
while (parentMenu._parentMenuItem) {
parentMenu = parentMenu._parentMenuItem.parentElement as Menu;
_findMainMenu(element: MenuItem | Menu) {
let menu = this._isMenu(element) ? element as Menu : element.parentElement as Menu;
while (menu && menu._parentMenuItem) {
menu = menu._parentMenuItem.parentElement as Menu;
}

return parentMenu;
return menu;
}

_isMenu(element: HTMLElement) {
return element.hasAttribute("ui5-menu");
}

_beforePopoverOpen(e: CustomEvent) {
Expand Down
2 changes: 1 addition & 1 deletion packages/main/src/MenuItem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ class MenuItem extends UI5Element {
}

get subMenuOpened() {
return !!this._subMenu;
return !!this._subMenu?._popover?.isOpen();
}

get _additionalText() {
Expand Down
15 changes: 6 additions & 9 deletions packages/main/src/NavigationMenu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import slot from "@ui5/webcomponents-base/dist/decorators/slot.js";
import litRender from "@ui5/webcomponents-base/dist/renderer/LitRenderer.js";
import {
isDesktop,
isPhone,
isTablet,
} from "@ui5/webcomponents-base/dist/Device.js";
import type { ListItemClickEventDetail } from "./List.js";
import Menu from "./Menu.js";
Expand Down Expand Up @@ -67,6 +65,10 @@ class NavigationMenu extends Menu {
@slot({ "default": true, type: HTMLElement, invalidateOnChildChange: true })
declare items: Array<NavigationMenuItem>;

_isMenu(element: HTMLElement) {
return element.hasAttribute("ui5-navigation-menu");
}

_itemMouseOver(e: MouseEvent) {
if (isDesktop()) {
// respect mouseover only on desktop
Expand Down Expand Up @@ -124,14 +126,9 @@ class NavigationMenu extends Menu {
mainMenu._popover!.close();
}

if (isPhone()) {
// prepares and opens sub-menu on phone
this._prepareSubMenuPhone(item);
} else if (isTablet()) {
// prepares and opens sub-menu on tablet
this._prepareSubMenuDesktopTablet(item, opener);
}
this._prepareSubMenu(item, opener);
}

get accSideNavigationPopoverHiddenText() {
return NavigationMenu.i18nBundle.getText(NAVIGATION_MENU_POPOVER_HIDDEN_TEXT);
}
Expand Down
4 changes: 2 additions & 2 deletions packages/main/test/pages/Menu.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<body class="bg">
<ui5-button id="btnOpen">Open Menu</ui5-button> <br/>
<ui5-menu id="menu" header-text="My ui5-menu">
<ui5-menu-item text="New File" accessible-name="Opens a file explorer" additional-text="Ctrl+Alt+Shift+N" tooltip="Select a file" icon="add-document"></ui5-menu-item>
<ui5-menu-item text="New File(selection prevented)" accessible-name="Opens a file explorer" additional-text="Ctrl+Alt+Shift+N" tooltip="Select a file - prevent default" icon="add-document"></ui5-menu-item>
<ui5-menu-item text="New Folder with very long title for a menu item" additional-text="Ctrl+F" icon="add-folder" disabled></ui5-menu-item>
<ui5-menu-item text="Open" icon="open-folder" starts-section busy-delay="100" busy>
<ui5-menu-item text="Open Locally" icon="open-folder" additional-text="Ctrl+K">
Expand Down Expand Up @@ -119,7 +119,7 @@
menu.addEventListener("ui5-item-click", function(event) {
const item = event.detail.item;
selectionInput.value = item.text;
if (item && item.text === "New File") {
if (item && item.text === "New File(selection prevented)") {
event.preventDefault();
}
});
Expand Down
28 changes: 13 additions & 15 deletions packages/main/test/specs/Menu.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,24 +59,22 @@ describe("Menu interaction", () => {

listItems[3].click(); // open sub-menu

await submenuList.$("ui5-menu").waitForExist({
await submenuList.$("ui5-menu:nth-of-type(1)").waitForExist({
timeout: 1000,
timeoutMsg: "The second level sub-menu is should be created"
timeoutMsg: "First sub-menu is created"
})

assert.ok(await submenuList.$("ui5-menu"), "The second level sub-menu is being created"); // new ui5-menu element is created for the sub-menu

await browser.keys("ArrowLeft"); // back to main menu
await browser.keys("ArrowDown"); // go to the next menu item (close sub-menu)
listItems[4].click(); // open sub-menu

await submenuList.$("ui5-menu").waitForExist({
reverse: true,
await submenuList.$("ui5-menu:nth-of-type(2)").waitForExist({
timeout: 1000,
timeoutMsg: "The second level sub-menu is should be destroyed"
timeoutMsg: "Second sub-menu is created"
})

assert.strictEqual(await submenuList.$$("ui5-menu").length, 0,
"The second level sub-menu is being destroyed"); // sub-menu ui5-menu element is destroyed
assert.strictEqual(await submenuList.$$("ui5-menu").length, 2,
"Two sub-menus are present");
});

it("Event firing after 'click' on menu item", async () => {
Expand All @@ -92,7 +90,7 @@ describe("Menu interaction", () => {

await listItems[0].click({x: 1, y: 1});

assert.strictEqual(await selectionInput.getAttribute("value"), "New File", "Click on first item fires an event");
assert.strictEqual(await selectionInput.getAttribute("value"), "New File(selection prevented)", "Click on first item fires an event");
});

it("Event firing after [Space] on menu item", async () => {
Expand All @@ -107,7 +105,7 @@ describe("Menu interaction", () => {

await browser.keys("Space");

assert.strictEqual(await selectionInput.getAttribute("value"), "New File", "Pressing [Space] on first item fires an event");
assert.strictEqual(await selectionInput.getAttribute("value"), "New File(selection prevented)", "Pressing [Space] on first item fires an event");
});

it("Event firing after [Enter] on menu item", async () => {
Expand All @@ -122,7 +120,7 @@ describe("Menu interaction", () => {

await browser.keys("Enter");

assert.strictEqual(await selectionInput.getAttribute("value"), "New File", "Pressing [Enter] on first item fires an event");
assert.strictEqual(await selectionInput.getAttribute("value"), "New File(selection prevented)", "Pressing [Enter] on first item fires an event");
});

it("Events firing on open/close of the menu", async () => {
Expand Down Expand Up @@ -180,7 +178,7 @@ describe("Menu interaction", () => {
await browser.pause(100);

const menuPopover = await browser.$("ui5-static-area-item:last-of-type").shadow$("ui5-responsive-popover");
const newFileItem = await menuPopover.$("ui5-menu-li[accessible-name='New File']");
const newFileItem = await menuPopover.$("ui5-menu-li[accessible-name='New File(selection prevented)']");
newFileItem.click();
await browser.pause(100);

Expand Down Expand Up @@ -217,11 +215,11 @@ describe("Menu Accessibility", () => {

assert.strictEqual(await list.getAttribute("accessible-role"), "menu", "There is proper 'menu' role for the menu list");
assert.strictEqual(await listItems[0].getAttribute("accessible-role"), "menuitem", "There is proper 'menuitem' role for the menu list items");
assert.strictEqual(await listItems[0].getAttribute("tooltip"), "Select a file", "There is a tooltip");
assert.strictEqual(await listItems[0].getAttribute("tooltip"), "Select a file - prevent default", "There is a tooltip");
assert.strictEqual(await listItems[2].shadow$(".ui5-li-root").getAttribute("aria-haspopup"), "menu", "There is an aria-haspopup attribute");
assert.strictEqual(
await listItems[0].getAttribute("accessible-name"),
"New File Opens a file explorer",
"New File(selection prevented) Opens a file explorer",
"There is additional description added");
});
});