Merge pull request #60358 from nextcloud/fix/59888/waffle-menu-p2-polish

feat(core): app menu polish for NC34

Author: pringelmann

core/src/components/AppMenu.vue

@@ -33,7 +33,7 @@
 				class="app-menu__popover"
 				role="menu"
 				:aria-label="t('core', 'Apps')">
-				<div class="app-menu__grid" @keydown="onGridKeydown">
+				<div ref="grid" class="app-menu__grid" @keydown="onGridKeydown">
 					<AppItem
 						v-for="(item, i) in gridItems"
 						:key="item.id"
@@ -49,13 +49,14 @@
 			v-if="currentApp"
 			class="app-menu__current-app"
 			variant="tertiary-no-background"
-			:aria-label="t('core', 'Open apps menu')"
+			:aria-label="currentAppLabel"
 			aria-haspopup="menu"
 			:aria-expanded="opened ? 'true' : 'false'"
 			@click="onTriggerClick('currentApp')">
 			<template #icon>
 				<img
 					class="app-menu__current-app-icon"
+					:class="{ 'app-menu__current-app-icon--settings': currentApp.type === 'settings' }"
 					:src="currentApp.icon"
 					alt=""
 					aria-hidden="true">
@@ -82,6 +83,9 @@ import IconDotsGrid from 'vue-material-design-icons/DotsGrid.vue'
 import AppItem from './AppItem.vue'
 import logger from '../logger.js'
 
+// Settings IDs that represent actions, not navigable pages.
+const SETTINGS_ACTION_IDS = new Set(['logout'])
+
 export default defineComponent({
 	name: 'AppMenu',
 
@@ -103,8 +107,12 @@ export default defineComponent({
 
 	data() {
 		const appList = loadState<INavigationEntry[]>('core', 'apps', [])
+		// Record<id, entry>, not an array: PHP ships getAll('settings') without
+		// array_values(). Matches AccountMenu.vue's usage.
+		const settingsList = loadState<Record<string, INavigationEntry>>('core', 'settingsNavEntries', {})
 		return {
 			appList,
+			settingsList,
 			isAdmin: getCurrentUser()?.isAdmin ?? false,
 			// Roving tabindex: only this tile has tabindex=0; arrow keys move it.
 			focusedIndex: 0,
@@ -146,7 +154,18 @@ export default defineComponent({
 
 	computed: {
 		currentApp(): INavigationEntry | undefined {
+			// Fall back to the active settings entry on admin pages where no
+			// app is active.
 			return this.appList.find((app) => app.active)
+				?? Object.values(this.settingsList).find((entry) => entry.active && !SETTINGS_ACTION_IDS.has(entry.id))
+		},
+
+		// aria-label overrides the inner span text, so the section name
+		// has to be duplicated here for screen readers.
+		currentAppLabel(): string {
+			return this.currentApp
+				? t('core', 'Open apps menu, currently in {app}', { app: this.currentApp.name })
+				: t('core', 'Open apps menu')
 		},
 
 		// Stable-ordered list that focusedIndex indexes into. The trailing
@@ -159,10 +178,13 @@ export default defineComponent({
 	},
 
 	watch: {
-		// On open, land the roving stop on the active app rather than index 0.
+		// On open, land the roving stop on the active app rather than index 0
+		// and measure the grid as soon as it mounts (before the open
+		// transition finishes, so the cap is set without a flash).
 		opened(isOpen: boolean) {
 			if (isOpen) {
 				this.focusedIndex = this.activeGridIndex()
+				this.tryRecomputeGridMaxHeight(5)
 			}
 		},
 	},
@@ -218,6 +240,43 @@ export default defineComponent({
 			}
 		},
 
+		// Poll briefly for the grid ref (NcPopover renders the slot async)
+		// then measure once. Bounded so a missing ref can never leak frames.
+		tryRecomputeGridMaxHeight(retries: number) {
+			if (!this.opened || retries <= 0) {
+				return
+			}
+			if (!this.$refs.grid) {
+				requestAnimationFrame(() => this.tryRecomputeGridMaxHeight(retries - 1))
+				return
+			}
+			this.recomputeGridMaxHeight()
+		},
+
+		// Cap = sum of first 6 row heights + baseline × 6, so the peek of
+		// row 7 stays constant when wraps grow rows.
+		recomputeGridMaxHeight() {
+			const grid = this.$refs.grid as HTMLElement | undefined
+			if (!grid) {
+				return
+			}
+			const VISIBLE_CELLS = 24 // 4 cols × 6 visible rows
+			const cells = grid.children
+			if (cells.length <= VISIBLE_CELLS) {
+				grid.style.maxHeight = ''
+				return
+			}
+			const firstHidden = cells[VISIBLE_CELLS] as HTMLElement | undefined
+			const firstCell = cells[0] as HTMLElement | undefined
+			if (!firstHidden || !firstCell) {
+				return
+			}
+			const sumOfFirstRows = firstHidden.getBoundingClientRect().top
+				- firstCell.getBoundingClientRect().top
+			const baseline = parseFloat(getComputedStyle(grid).getPropertyValue('--default-grid-baseline')) || 4
+			grid.style.maxHeight = `${sumOfFirstRows + baseline * 6}px`
+		},
+
 		// Index of the active app within `gridItems`, or 0 if none is active.
 		activeGridIndex(): number {
 			const idx = this.gridItems.findIndex((app) => app.active)
@@ -366,6 +425,16 @@ export default defineComponent({
 			outline: none !important;
 			box-shadow: inset 0 0 0 2px var(--color-background-plain-text) !important;
 		}
+
+		// Inner text slot needs min-width: 0 so the label can ellipsize.
+		:deep(.button-vue__text) {
+			min-width: 0;
+		}
+
+		// Hide on small screens (matches $breakpoint-small-mobile in @nextcloud/vue).
+		@media only screen and (max-width: 512px) {
+			display: none !important;
+		}
 	}
 
 	&__current-app-icon {
@@ -374,13 +443,27 @@ export default defineComponent({
 		// Theme-aware inversion + vertical alpha fade via --header-menu-icon-mask.
 		filter: var(--background-image-invert-if-bright);
 		mask: var(--header-menu-icon-mask);
+
+		// Settings icons ship dark (designed for the white settings sidebar);
+		// force-white so they read against the themed header.
+		&--settings {
+			filter: brightness(0) invert(1);
+		}
 	}
 
 	&__current-app-name {
+		// inline-block: inline elements ignore max-width + overflow.
+		display: inline-block;
+		vertical-align: middle;
 		font-size: var(--default-font-size);
 		font-weight: 500;
 		white-space: nowrap;
 		letter-spacing: -0.5px;
+		overflow: hidden;
+		text-overflow: ellipsis;
+		// Cap width so long titles ellipsize instead of pushing the header
+		// icons off-screen (.header-start doesn't shrink).
+		max-width: clamp(80px, 22vw, 320px);
 	}
 
 	&__popover {
@@ -391,14 +474,23 @@ export default defineComponent({
 	&__grid {
 		--app-item-col-width: 69px;
 		--app-item-row-height: 64px;
-		--app-menu-rows-visible: 6;
-		padding: calc(var(--default-grid-baseline) * 3) calc(var(--default-grid-baseline) * 2);
+		// border-box: the JS-set max-height (see recomputeGridMaxHeight)
+		// needs to include padding for the peek math to hold.
+		box-sizing: border-box;
+		padding: calc(var(--default-grid-baseline) * 2);
 		display: grid;
 		grid-template-columns: repeat(4, var(--app-item-col-width));
 		grid-auto-rows: minmax(var(--app-item-row-height), max-content);
-		max-height: calc(var(--app-item-row-height) * var(--app-menu-rows-visible) + var(--default-grid-baseline) * 5);
+		// max-height set inline by recomputeGridMaxHeight(); CSS just owns the scroll.
 		overflow-y: auto;
 
+		// Extra top padding on first-row tiles so the hover bg reads
+		// concentric with the popover's rounded top corner. !important
+		// because AppItem's scoped rule has the same specificity.
+		> :nth-child(-n+4) {
+			padding-block-start: calc(var(--default-grid-baseline) * 2) !important;
+		}
+
 		// WebKit equivalents are in the unscoped block below: scoped CSS
 		// data-attrs don't reach ::-webkit-scrollbar pseudo-elements in Chrome.
 		scrollbar-width: thin;

core/src/tests/components/AppMenu.spec.ts

@@ -165,4 +165,64 @@ describe('core: AppMenu', () => {
 		const currentApp = wrapper.get('.app-menu__current-app').element
 		expect(wrapper.vm.returnFocusTarget()).toBe(currentApp)
 	})
+
+	it('falls back to the active settings entry when no app is active', () => {
+		// Mimics being on /settings/admin/* where the active entry is registered
+		// as type=settings (NavigationManager) and excluded from the `apps` list.
+		initialState.loadState.mockImplementation((_a: string, key: string, fallback: unknown) => {
+			if (key === 'apps') {
+				return [makeApp({ id: 'files', name: 'Files', active: false })]
+			}
+			if (key === 'settingsNavEntries') {
+				// Object keyed by entry id — matches PHP's serialization shape
+				// (TemplateLayout ships the filtered associative array as-is).
+				return {
+					admin_settings: makeApp({
+						id: 'admin_settings',
+						name: 'Administration settings',
+						type: 'settings',
+						href: '/settings/admin/overview',
+						icon: '/settings/img/admin.svg',
+						active: true,
+					}),
+				}
+			}
+			return fallback
+		})
+		const wrapper = mount(AppMenu, { attachTo: document.body })
+		expect(wrapper.find('.app-menu__current-app').exists()).toBe(true)
+		expect(wrapper.find('.app-menu__current-app-name').text()).toBe('Administration settings')
+	})
+
+	it('prefers the active app over a settings entry when both are marked active', () => {
+		initialState.loadState.mockImplementation((_a: string, key: string, fallback: unknown) => {
+			if (key === 'apps') {
+				return [makeApp({ id: 'files', name: 'Files', active: true })]
+			}
+			if (key === 'settingsNavEntries') {
+				return { admin_settings: makeApp({ id: 'admin_settings', name: 'Administration settings', type: 'settings', active: true }) }
+			}
+			return fallback
+		})
+		const wrapper = mount(AppMenu, { attachTo: document.body })
+		expect(wrapper.find('.app-menu__current-app-name').text()).toBe('Files')
+	})
+
+	it('does not render the current-app button when only the logout entry is active', () => {
+		// Defensive: logout is an action, not a page, so it should never be the
+		// "current section" even though it carries type=settings. NavigationManager
+		// today never marks it active, but a future regression shouldn't leak a
+		// "Log out" label into the header.
+		initialState.loadState.mockImplementation((_a: string, key: string, fallback: unknown) => {
+			if (key === 'apps') {
+				return [makeApp({ id: 'files', name: 'Files', active: false })]
+			}
+			if (key === 'settingsNavEntries') {
+				return { logout: makeApp({ id: 'logout', name: 'Log out', type: 'settings', href: '/logout', active: true }) }
+			}
+			return fallback
+		})
+		const wrapper = mount(AppMenu, { attachTo: document.body })
+		expect(wrapper.find('.app-menu__current-app').exists()).toBe(false)
+	})
 })