Merge pull request #62963 from nextcloud/feat/app-menu-default-order

App menu visual polishing

Author: susnux

apps/appstore/appinfo/info.xml

@@ -21,13 +21,7 @@
 	</dependencies>
 
 	<navigations>
-		<navigation role="admin">
-			<name>Appstore</name>
-			<route>appstore.page.viewApps</route>
-			<icon>app.svg</icon>
-			<order>99</order>
-		</navigation>
-
+		<!-- No app menu entry: the menu already has a "More apps" tile linking here -->
 		<navigation role="admin">
 			<name>Apps</name>
 			<route>appstore.page.viewApps</route>

core/src/components/AppIcon.vue

@@ -93,14 +93,16 @@ $bevel:
 		}
 	}
 
+	// Utility entries ("More apps", "App store") stay subdued: a plain circle
+	// with the icon in the same muted color as the label.
 	&--outlined {
 		background: transparent;
 		background-image: none;
-		box-shadow: inset 0 0 0 2px var(--color-border-maxcontrast);
+		box-shadow: inset 0 0 0 2px var(--color-border);
 	}
 
 	&--outlined &__img {
-		background-color: var(--color-main-text);
+		background-color: var(--color-text-maxcontrast);
 		background-image: none;
 	}
 }

core/src/components/AppItem.vue

@@ -8,6 +8,7 @@
 		class="app-item"
 		:class="{
 			'app-item--active': app.active,
+			'app-item--outlined': outlined,
 		}"
 		:href="app.href"
 		:target="newTab ? '_blank' : undefined"
@@ -125,5 +126,10 @@ const unreadLabel = computed(() => {
 	&--active &__label {
 		font-weight: bold;
 	}
+
+	// Utility entries ("More apps", "App store") are subdued, they are not apps.
+	&--outlined &__label {
+		color: var(--color-text-maxcontrast);
+	}
 }
 </style>

core/src/components/AppMenu.vue

@@ -54,19 +54,17 @@
 			:aria-expanded="opened ? 'true' : 'false'"
 			@click="onTriggerClick('currentApp')">
 			<template #icon>
-				<!-- Settings sub-sections share one generic cog. An inline MDI icon
-					inherits the button's currentColor (--color-background-plain-text),
-					so it stays legible on both bright and dark headers without a filter. -->
+				<!-- Settings sections and entries without an icon show a generic cog. -->
 				<IconCog
-					v-if="currentApp.type === 'settings'"
+					v-if="isSettingsSection || !currentAppIcon"
 					class="app-menu__current-app-cog"
 					:size="20" />
-				<img
-					v-else
-					class="app-menu__current-app-icon"
-					:src="currentApp.icon"
-					alt=""
-					aria-hidden="true">
+				<!-- Outer element carries the header fade, inner one the icon shape. -->
+				<span v-else class="app-menu__current-app-icon">
+					<span
+						class="app-menu__current-app-glyph"
+						:style="currentAppIconStyle" />
+				</span>
 			</template>
 			<span class="app-menu__current-app-name">
 				{{ displayName }}
@@ -94,6 +92,15 @@ import logger from '../logger.js'
 // Settings IDs that represent actions, not navigable pages.
 const SETTINGS_ACTION_IDS = new Set(['logout'])
 
+const SETTINGS_SECTION_IDS = new Set(['settings_personal', 'settings_administration', 'accessibility_settings'])
+
+// The profile entry is named "View profile" for the account menu, which is an
+// action rather than a page name. The header shows where you are instead.
+const PROFILE_ID = 'profile'
+
+// Entry of the app management page, the target of the "More apps" tile.
+const APP_MANAGEMENT_ID = 'appstore'
+
 export default defineComponent({
 	name: 'AppMenu',
 
@@ -169,18 +176,36 @@ export default defineComponent({
 				?? Object.values(this.settingsList).find((entry) => entry.active && !SETTINGS_ACTION_IDS.has(entry.id))
 		},
 
-		// Trigger label. Settings sub-section names ("Personal info",
-		// "Appearance and accessibility", ...) are too long and varied to
-		// surface in the header; collapse them all to a single "Settings".
+		isSettingsSection(): boolean {
+			return this.currentApp !== undefined && SETTINGS_SECTION_IDS.has(this.currentApp.id)
+		},
+
 		displayName(): string {
 			if (!this.currentApp) {
 				return ''
 			}
-			return this.currentApp.type === 'settings'
-				? t('core', 'Settings')
+			if (this.isSettingsSection) {
+				return t('core', 'Settings')
+			}
+			return this.currentApp.id === PROFILE_ID
+				? t('core', 'Profile')
 				: this.currentApp.name
 		},
 
+		// The profile entry ships no icon of its own, so use the generic one.
+		currentAppIcon(): string {
+			if (this.currentApp?.id === PROFILE_ID) {
+				return imagePath('core', 'actions/user.svg')
+			}
+			return this.currentApp?.icon ?? ''
+		},
+
+		// Masked like AppIcon.vue, so dark icons stay legible on the header.
+		// Escaped so a crafted path cannot break out of the url() token.
+		currentAppIconStyle(): Record<string, string> {
+			return { '--app-icon-url': `url("${this.currentAppIcon.replace(/["\\]/g, '\\$&')}")` }
+		},
+
 		// aria-label overrides the inner span text, so the displayed name
 		// has to be duplicated here for screen readers.
 		currentAppLabel(): string {
@@ -193,7 +218,9 @@ export default defineComponent({
 		// utility tile is "More apps" (local app management) for admins and
 		// "App store" (apps.nextcloud.com) for everyone else.
 		gridItems(): INavigationEntry[] {
-			const tail = this.isAdmin ? this.moreAppsEntry : this.appStoreEntry
+			const tail = this.isAdmin
+				? { ...this.moreAppsEntry, active: this.currentApp?.id === APP_MANAGEMENT_ID }
+				: this.appStoreEntry
 			return [...this.appList, tail]
 		},
 	},
@@ -459,13 +486,29 @@ export default defineComponent({
 	}
 
 	&__current-app-icon {
+		display: flex;
 		width: calc(var(--default-grid-baseline) * 5);
 		height: calc(var(--default-grid-baseline) * 5);
-		// Theme-aware inversion + vertical alpha fade via --header-menu-icon-mask.
-		filter: var(--background-image-invert-if-bright);
+		// Vertical alpha fade, like the cog and the other header icons.
 		mask: var(--header-menu-icon-mask);
 	}
 
+	&__current-app-glyph {
+		width: 100%;
+		height: 100%;
+		// Masked rather than shown: app icons ship a hardcoded fill, so the
+		// color has to come from the background. Matches AppIcon.vue.
+		background-color: var(--color-background-plain-text);
+		mask: var(--app-icon-url) center / contain no-repeat;
+	}
+
+	// Masked backgrounds are not force-adjusted the way <img> is.
+	@media (forced-colors: active) {
+		&__current-app-glyph {
+			background-color: CanvasText;
+		}
+	}
+
 	&__current-app-cog {
 		mask: var(--header-menu-icon-mask);
 	}

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

@@ -65,6 +65,22 @@ function fakeApps(): INavigationEntry[] {
 	]
 }
 
+// Mimics a page where the active entry is a settings one, so it is excluded
+// from the `apps` list. The object shape matches PHP's serialization, which
+// ships getAll('settings') keyed by entry id.
+function mockActiveSettingsEntry(overrides: Partial<INavigationEntry>): void {
+	const entry = makeApp({ type: 'settings', active: true, ...overrides })
+	initialState.loadState.mockImplementation((_a: string, key: string, fallback: unknown) => {
+		if (key === 'apps') {
+			return [makeApp({ id: 'files', name: 'Files', active: false })]
+		}
+		if (key === 'settingsNavEntries') {
+			return { [entry.id]: entry }
+		}
+		return fallback
+	})
+}
+
 function eightApps(activeIndex: number = -1): INavigationEntry[] {
 	const ids = ['files', 'mail', 'calendar', 'contacts', 'notes', 'photos', 'talk', 'deck']
 	return ids.map((id, i) => makeApp({
@@ -137,6 +153,18 @@ describe('core: AppMenu', () => {
 		expect(moreApps).toBeTruthy()
 	})
 
+	it('marks the "More apps" tile active on the app management page', async () => {
+		auth.getCurrentUser.mockReturnValue({ isAdmin: true })
+		mockActiveSettingsEntry({ id: 'appstore', name: 'Apps', href: '/settings/apps' })
+		const wrapper = mount(AppMenu, { attachTo: document.body })
+		await openPopover(wrapper)
+
+		const moreApps = Array.from(document.querySelectorAll('[role="menuitem"]'))
+			.find((el) => el.textContent?.includes('More apps'))
+		expect(moreApps?.classList.contains('app-item--active')).toBe(true)
+		expect(moreApps?.getAttribute('aria-current')).toBe('page')
+	})
+
 	it('ArrowRight moves the roving stop from index 0 to index 1 and focuses it', async () => {
 		initialState.loadState.mockImplementation((_a: string, key: string, fallback: unknown) => key === 'apps' ? eightApps() : fallback)
 		const wrapper = mount(AppMenu, { attachTo: document.body })
@@ -174,41 +202,54 @@ describe('core: AppMenu', () => {
 	})
 
 	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
+		// Mimics being on /settings/admin/*
+		mockActiveSettingsEntry({
+			id: 'settings_administration',
+			name: 'Administration settings',
+			href: '/settings/admin/overview',
+			icon: '/settings/img/admin.svg',
 		})
 		const wrapper = mount(AppMenu, { attachTo: document.body })
 		expect(wrapper.find('.app-menu__current-app').exists()).toBe(true)
 		// Settings sub-section names are collapsed to a single "Settings" label.
 		expect(wrapper.find('.app-menu__current-app-name').text()).toBe('Settings')
 	})
 
+	it('keeps the own name of settings entries outside the settings app', () => {
+		// On /settings/apps the active entry is the app management one, which
+		// shows "Apps" here and in the account menu, not "Settings".
+		mockActiveSettingsEntry({ id: 'appstore', name: 'Apps', href: '/settings/apps', icon: '/apps/appstore/img/app-dark.svg' })
+		const wrapper = mount(AppMenu, { attachTo: document.body })
+		expect(wrapper.find('.app-menu__current-app-name').text()).toBe('Apps')
+		// Its own icon, not the generic cog of the settings sections
+		expect(wrapper.find('.app-menu__current-app-glyph').attributes('style'))
+			.toContain('/apps/appstore/img/app-dark.svg')
+	})
+
+	it('shows the profile page as "Profile" with the generic user icon', () => {
+		// The profile app names its entry "View profile" for the account menu
+		// and ships no icon, so both are replaced here.
+		mockActiveSettingsEntry({ id: 'profile', name: 'View profile', href: '/u/admin', icon: '' })
+		const wrapper = mount(AppMenu, { attachTo: document.body })
+		expect(wrapper.find('.app-menu__current-app-name').text()).toBe('Profile')
+		expect(wrapper.find('.app-menu__current-app-glyph').attributes('style'))
+			.toContain('/core/img/actions/user.svg')
+	})
+
+	it('falls back to the cog for entries without an icon', () => {
+		mockActiveSettingsEntry({ id: 'help', name: 'Help & privacy', href: '/settings/help', icon: '' })
+		const wrapper = mount(AppMenu, { attachTo: document.body })
+		expect(wrapper.find('.app-menu__current-app-cog').exists()).toBe(true)
+		expect(wrapper.find('.app-menu__current-app-glyph').exists()).toBe(false)
+	})
+
 	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 { settings_administration: makeApp({ id: 'settings_administration', name: 'Administration settings', type: 'settings', active: true }) }
 			}
 			return fallback
 		})

dist/core-main.js

@@ -54,7 +54,9 @@ adminTest.describe('Header: App menu (waffle launcher) – admin', () => {
 		await expectWaffleMenuContainsApps(navigationHeader, [
 			{ name: 'Files', href: '/apps/files' },
 			{ name: 'Dashboard', href: '/apps/dashboard' },
-			{ name: 'Appstore', href: '/settings/apps' },
+			// The app management page is only linked by the "More apps" tile,
+			// the app itself does not add an entry to the menu
+			{ name: 'More apps', href: '/settings/apps' },
 		])
 	})
 })