Merge pull request #41024 from nextcloud/fix/follow-up-app-order

apps/theming/lib/Settings/Personal.php

@@ -74,7 +74,10 @@ class Personal implements ISettings {
 		$this->initialStateService->provideInitialState('themes', array_values($themes));
 		$this->initialStateService->provideInitialState('enforceTheme', $enforcedTheme);
 		$this->initialStateService->provideInitialState('isUserThemingDisabled', $this->themingDefaults->isUserThemingDisabled());
-		$this->initialStateService->provideInitialState('enforcedDefaultApp', $forcedDefaultApp);
+		$this->initialStateService->provideInitialState('navigationBar', [
+			'userAppOrder' => json_decode($this->config->getUserValue($this->userId, 'core', 'apporder', '[]'), true, flags:JSON_THROW_ON_ERROR),
+			'enforcedDefaultApp' => $forcedDefaultApp
+		]);
 
 		Util::addScript($this->appName, 'personal-theming');
 

apps/theming/src/components/AppOrderSelector.vue

@@ -18,11 +18,13 @@ import { PropType, computed, defineComponent, ref } from 'vue'
 
 import AppOrderSelectorElement from './AppOrderSelectorElement.vue'
 
-interface IApp {
+export interface IApp {
 	id: string // app id
 	icon: string // path to the icon svg
-	label?: string // display name
+	label: string // display name
 	default?: boolean // force app as default app
+	app: string
+	key: number
 }
 
 export default defineComponent({

apps/theming/src/components/AppOrderSelectorElement.vue

@@ -23,20 +23,22 @@
 
 		<div class="order-selector-element__actions">
 			<NcButton v-show="!isFirst && !app.default"
+				ref="buttonUp"
 				:aria-label="t('settings', 'Move up')"
 				data-cy-app-order-button="up"
 				type="tertiary-no-background"
-				@click="$emit('move:up')">
+				@click="moveUp">
 				<template #icon>
 					<IconArrowUp :size="20" />
 				</template>
 			</NcButton>
 			<div v-show="isFirst || !!app.default" aria-hidden="true" class="order-selector-element__placeholder" />
 			<NcButton v-show="!isLast && !app.default"
+				ref="buttonDown"
 				:aria-label="t('settings', 'Move down')"
 				data-cy-app-order-button="down"
 				type="tertiary-no-background"
-				@click="$emit('move:down')">
+				@click="moveDown">
 				<template #icon>
 					<IconArrowDown :size="20" />
 				</template>
@@ -47,8 +49,10 @@
 </template>
 
 <script lang="ts">
+import type { PropType } from 'vue'
+
 import { translate as t } from '@nextcloud/l10n'
-import { PropType, defineComponent } from 'vue'
+import { defineComponent, nextTick, onUpdated, ref } from 'vue'
 
 import IconArrowDown from 'vue-material-design-icons/ArrowDown.vue'
 import IconArrowUp from 'vue-material-design-icons/ArrowUp.vue'
@@ -86,8 +90,55 @@ export default defineComponent({
 		'move:up': () => true,
 		'move:down': () => true,
 	},
-	setup() {
+	setup(props, { emit }) {
+		const buttonUp = ref()
+		const buttonDown = ref()
+
+		/**
+		 * Used to decide if we need to trigger focus() an a button on update
+		 */
+		let needsFocus = 0
+
+		/**
+		 * Handle move up, ensure focus is kept on the button
+		 */
+		const moveUp = () => {
+			emit('move:up')
+			needsFocus = 1 // request focus on buttonUp
+		}
+
+		/**
+		 * Handle move down, ensure focus is kept on the button
+		 */
+		 const moveDown = () => {
+			emit('move:down')
+			needsFocus = -1 // request focus on buttonDown
+		}
+
+		/**
+		 * onUpdated hook is used to reset the focus on the last used button (if requested)
+		 * If the button is now visible anymore (because this element is the first/last) then the opposite button is focussed
+		 */
+		onUpdated(() => {
+			if (needsFocus !== 0) {
+				// focus requested
+				if ((needsFocus === 1 || props.isLast) && !props.isFirst) {
+					// either requested to btn up and it is not the first, or it was requested to btn down but it is the last
+					nextTick(() => buttonUp.value.$el.focus())
+				} else {
+					nextTick(() => buttonDown.value.$el.focus())
+				}
+			}
+			needsFocus = 0
+		})
+
 		return {
+			buttonUp,
+			buttonDown,
+
+			moveUp,
+			moveDown,
+
 			t,
 		}
 	},

apps/theming/src/components/UserAppMenuSection.vue

@@ -3,13 +3,27 @@
 		<p>
 			{{ t('theming', 'You can configure the app order used for the navigation bar. The first entry will be the default app, opened after login or when clicking on the logo.') }}
 		</p>
-		<NcNoteCard v-if="!!appOrder[0]?.default" type="info">
+		<NcNoteCard v-if="enforcedDefaultApp" :id="elementIdEnforcedDefaultApp" type="info">
 			{{ t('theming', 'The default app can not be changed because it was configured by the administrator.') }}
 		</NcNoteCard>
-		<NcNoteCard v-if="hasAppOrderChanged" type="info">
+		<NcNoteCard v-if="hasAppOrderChanged" :id="elementIdAppOrderChanged" type="info">
 			{{ t('theming', 'The app order was changed, to see it in action you have to reload the page.') }}
 		</NcNoteCard>
-		<AppOrderSelector class="user-app-menu-order" :value.sync="appOrder" />
+
+		<AppOrderSelector class="user-app-menu-order"
+			:aria-details="ariaDetailsAppOrder"
+			:value="appOrder"
+			@update:value="updateAppOrder" />
+
+		<NcButton data-test-id="btn-apporder-reset"
+			:disabled="!hasCustomAppOrder"
+			type="tertiary"
+			@click="resetAppOrder">
+			<template #icon>
+				<IconUndo :size="20" />
+			</template>
+			{{ t('theming', 'Reset default app order') }}
+		</NcButton>
 	</NcSettingsSection>
 </template>
 
@@ -21,7 +35,9 @@ import { generateOcsUrl } from '@nextcloud/router'
 import { computed, defineComponent, ref } from 'vue'
 
 import axios from '@nextcloud/axios'
-import AppOrderSelector from './AppOrderSelector.vue'
+import AppOrderSelector, { IApp } from './AppOrderSelector.vue'
+import IconUndo from 'vue-material-design-icons/Undo.vue'
+import NcButton from '@nextcloud/vue/dist/Components/NcButton.js'
 import NcNoteCard from '@nextcloud/vue/dist/Components/NcNoteCard.js'
 import NcSettingsSection from '@nextcloud/vue/dist/Components/NcSettingsSection.js'
 
@@ -47,53 +63,109 @@ interface INavigationEntry {
 	key: number
 }
 
+/** The app order user setting */
+type IAppOrder = Record<string, Record<number, number>>
+
+/** OCS responses */
+interface IOCSResponse<T> {
+	ocs: {
+		meta: unknown
+		data: T
+	}
+}
+
 export default defineComponent({
 	name: 'UserAppMenuSection',
 	components: {
 		AppOrderSelector,
+		IconUndo,
+		NcButton,
 		NcNoteCard,
 		NcSettingsSection,
 	},
 	setup() {
-		/**
-		 * Track if the app order has changed, so the user can be informed to reload
-		 */
-		const hasAppOrderChanged = ref(false)
-
-		/** The enforced default app set by the administrator (if any) */
-		const enforcedDefaultApp = loadState<string|null>('theming', 'enforcedDefaultApp', null)
+		const {
+			/** The app order currently defined by the user */
+			userAppOrder,
+			/** The enforced default app set by the administrator (if any) */
+			enforcedDefaultApp,
+		} = loadState<{ userAppOrder: IAppOrder, enforcedDefaultApp: string }>('theming', 'navigationBar')
 
 		/**
 		 * Array of all available apps, it is set by a core controller for the app menu, so it is always available
 		 */
-		const allApps = ref(
-			Object.values(loadState<Record<string, INavigationEntry>>('core', 'apps'))
-				.filter(({ type }) => type === 'link')
-				.map((app) => ({ ...app, label: app.name, default: app.default && app.app === enforcedDefaultApp })),
-		)
+		 const initialAppOrder = Object.values(loadState<Record<string, INavigationEntry>>('core', 'apps'))
+			.filter(({ type }) => type === 'link')
+			.map((app) => ({ ...app, label: app.name, default: app.default && app.app === enforcedDefaultApp }))
 
 		/**
-		 * Wrapper around the sortedApps list with a setter for saving any changes
+		 * Check if a custom app order is used or the default is shown
 		 */
-		const appOrder = computed({
-			get: () => allApps.value,
-			set: (value) => {
-				const order = {} as Record<string, Record<number, number>>
-				value.forEach(({ app, key }, index) => {
-					order[app] = { ...order[app], [key]: index }
-				})
+		const hasCustomAppOrder = ref(!Array.isArray(userAppOrder) || Object.values(userAppOrder).length > 0)
 
-				saveSetting('apporder', order)
-					.then(() => {
-						allApps.value = value
-						hasAppOrderChanged.value = true
-					})
-					.catch((error) => {
-						console.warn('Could not set the app order', error)
-						showError(t('theming', 'Could not set the app order'))
-					})
-			},
-		})
+		/**
+		 * Track if the app order has changed, so the user can be informed to reload
+		 */
+		const hasAppOrderChanged = computed(() => initialAppOrder.some(({ id }, index) => id !== appOrder.value[index].id))
+
+		/** ID of the "app order has changed" NcNodeCard, used for the aria-details of the apporder */
+		const elementIdAppOrderChanged = 'theming-apporder-changed-infocard'
+
+		/** ID of the "you can not change the default app" NcNodeCard, used for the aria-details of the apporder */
+		const elementIdEnforcedDefaultApp = 'theming-apporder-changed-infocard'
+
+		/**
+		 * The aria-details value of the app order selector
+		 * contains the space separated list of element ids of NcNoteCards
+		 */
+		const ariaDetailsAppOrder = computed(() => (hasAppOrderChanged.value ? `${elementIdAppOrderChanged} ` : '') + (enforcedDefaultApp ? elementIdEnforcedDefaultApp : ''))
+
+		/**
+		 * The current apporder (sorted by user)
+		 */
+		const appOrder = ref([...initialAppOrder])
+
+		/**
+		 * Update the app order, called when the user sorts entries
+		 * @param value The new app order value
+		 */
+		const updateAppOrder = (value: IApp[]) => {
+			const order: IAppOrder = {}
+			value.forEach(({ app, key }, index) => {
+				order[app] = { ...order[app], [key]: index }
+			})
+
+			saveSetting('apporder', order)
+				.then(() => {
+					appOrder.value = value as never
+					hasCustomAppOrder.value = true
+				})
+				.catch((error) => {
+					console.warn('Could not set the app order', error)
+					showError(t('theming', 'Could not set the app order'))
+				})
+		}
+
+		/**
+		 * Reset the app order to the default
+		 */
+		const resetAppOrder = async () => {
+			try {
+				await saveSetting('apporder', [])
+				hasCustomAppOrder.value = false
+
+				// Reset our app order list
+				const { data } = await axios.get<IOCSResponse<INavigationEntry[]>>(generateOcsUrl('/core/navigation/apps'), {
+					headers: {
+						'OCS-APIRequest': 'true',
+					},
+				})
+				appOrder.value = data.ocs.data.map((app) => ({ ...app, label: app.name, default: app.default && app.app === enforcedDefaultApp }))
+			} catch (error) {
+				console.warn(error)
+				showError(t('theming', 'Could not reset the app order'))
+			}
+		}
 
 		const saveSetting = async (key: string, value: unknown) => {
 			const url = generateOcsUrl('apps/provisioning_api/api/v1/config/users/{appId}/{configKey}', {
@@ -107,7 +179,16 @@ export default defineComponent({
 
 		return {
 			appOrder,
+			updateAppOrder,
+			resetAppOrder,
+
+			enforcedDefaultApp,
 			hasAppOrderChanged,
+			hasCustomAppOrder,
+
+			ariaDetailsAppOrder,
+			elementIdAppOrderChanged,
+			elementIdEnforcedDefaultApp,
 
 			t,
 		}

apps/theming/tests/Settings/PersonalTest.php

@@ -116,6 +116,11 @@ class PersonalTest extends TestCase {
 			->with('enforce_theme', '')
 			->willReturn($enforcedTheme);
 
+		$this->config->expects($this->once())
+			->method('getUserValue')
+			->with('admin', 'core', 'apporder')
+			->willReturn('[]');
+
 		$this->appManager->expects($this->once())
 			->method('getDefaultAppForUser')
 			->willReturn('forcedapp');
@@ -126,7 +131,7 @@ class PersonalTest extends TestCase {
 				['themes', $themesState],
 				['enforceTheme', $enforcedTheme],
 				['isUserThemingDisabled', false],
-				['enforcedDefaultApp', 'forcedapp'],
+				['navigationBar', ['userAppOrder' => [], 'enforcedDefaultApp' => 'forcedapp']],
 			);
 
 		$expected = new TemplateResponse('theming', 'settings-personal');

cypress/e2e/theming/navigation-bar-settings.cy.ts

@@ -94,15 +94,18 @@ describe('Admin theming set default apps', () => {
 })
 
 describe('User theming set app order', () => {
+	let user: User
+
 	before(() => {
 		cy.resetAdminTheming()
 		// Create random user for this test
-		cy.createRandomUser().then((user) => {
-			cy.login(user)
+		cy.createRandomUser().then(($user) => {
+			user = $user
+			cy.login($user)
 		})
 	})
 
-	after(() => cy.logout())
+	after(() => cy.deleteUser(user))
 
 	it('See the app order settings', () => {
 		cy.visit('/settings/user/theming')
@@ -144,6 +147,8 @@ describe('User theming set app order', () => {
 })
 
 describe('User theming set app order with default app', () => {
+	let user: User
+
 	before(() => {
 		cy.resetAdminTheming()
 		// install a third app
@@ -152,13 +157,14 @@ describe('User theming set app order with default app', () => {
 		cy.runOccCommand('config:system:set --value "calendar,files" defaultapp')
 
 		// Create random user for this test
-		cy.createRandomUser().then((user) => {
-			cy.login(user)
+		cy.createRandomUser().then(($user) => {
+			user = $user
+			cy.login($user)
 		})
 	})
 
 	after(() => {
-		cy.logout()
+		cy.deleteUser(user)
 		cy.runOccCommand('app:remove calendar')
 	})
 
@@ -186,11 +192,12 @@ describe('User theming set app order with default app', () => {
 
 		cy.get('[data-cy-app-order] [data-cy-app-order-element="dashboard"] [data-cy-app-order-button="up"]').should('not.be.visible')
 		cy.get('[data-cy-app-order] [data-cy-app-order-element="dashboard"] [data-cy-app-order-button="down"]').should('be.visible')
-		cy.get('[data-cy-app-order] [data-cy-app-order-element="files"] [data-cy-app-order-button="down"]').should('not.be.visible')
+
 		cy.get('[data-cy-app-order] [data-cy-app-order-element="files"] [data-cy-app-order-button="up"]').should('be.visible')
+		cy.get('[data-cy-app-order] [data-cy-app-order-element="files"] [data-cy-app-order-button="down"]').should('not.be.visible')
 	})
 
-	it('Change the other apps order', () => {
+	it('Change the order of the other apps', () => {
 		cy.get('[data-cy-app-order] [data-cy-app-order-element="files"] [data-cy-app-order-button="up"]').click()
 		cy.get('[data-cy-app-order] [data-cy-app-order-element="files"] [data-cy-app-order-button="up"]').should('not.be.visible')
 
@@ -210,3 +217,114 @@ describe('User theming set app order with default app', () => {
 		})
 	})
 })
+
+describe('User theming app order list accessibility', () => {
+	let user: User
+
+	before(() => {
+		cy.resetAdminTheming()
+		// Create random user for this test
+		cy.createRandomUser().then(($user) => {
+			user = $user
+			cy.login($user)
+		})
+	})
+
+	after(() => {
+		cy.deleteUser(user)
+	})
+
+	it('See the app order settings', () => {
+		cy.visit('/settings/user/theming')
+		cy.get('[data-cy-app-order]').scrollIntoView()
+		cy.get('[data-cy-app-order] [data-cy-app-order-element]').should('have.length', 2)
+	})
+
+	it('click the first button', () => {
+		cy.get('[data-cy-app-order] [data-cy-app-order-element]:first-of-type [data-cy-app-order-button="down"]').should('be.visible').click()
+	})
+
+	it('see the same app kept the focus', () => {
+		cy.get('[data-cy-app-order] [data-cy-app-order-element]:first-of-type [data-cy-app-order-button="down"]').should('not.have.focus')
+		cy.get('[data-cy-app-order] [data-cy-app-order-element]:last-of-type [data-cy-app-order-button="up"]').should('have.focus')
+	})
+
+	it('click the last button', () => {
+		cy.get('[data-cy-app-order] [data-cy-app-order-element]:last-of-type [data-cy-app-order-button="up"]').should('be.visible').click()
+	})
+
+	it('see the same app kept the focus', () => {
+		cy.get('[data-cy-app-order] [data-cy-app-order-element]:first-of-type [data-cy-app-order-button="down"]').should('have.focus')
+		cy.get('[data-cy-app-order] [data-cy-app-order-element]:last-of-type [data-cy-app-order-button="up"]').should('not.have.focus')
+	})
+})
+
+describe('User theming reset app order', () => {
+	let user: User
+
+	before(() => {
+		cy.resetAdminTheming()
+		// Create random user for this test
+		cy.createRandomUser().then(($user) => {
+			user = $user
+			cy.login($user)
+		})
+	})
+
+	after(() => cy.deleteUser(user))
+
+	it('See the app order settings', () => {
+		cy.visit('/settings/user/theming')
+
+		cy.get('.settings-section').contains('Navigation bar settings').should('exist')
+		cy.get('[data-cy-app-order]').scrollIntoView()
+	})
+
+	it('See that the dashboard app is the first one', () => {
+		cy.get('[data-cy-app-order] [data-cy-app-order-element]').each(($el, idx) => {
+			if (idx === 0) cy.wrap($el).should('have.attr', 'data-cy-app-order-element', 'dashboard')
+			else cy.wrap($el).should('have.attr', 'data-cy-app-order-element', 'files')
+		})
+
+		cy.get('.app-menu-main .app-menu-entry').each(($el, idx) => {
+			if (idx === 0) cy.wrap($el).should('have.attr', 'data-app-id', 'dashboard')
+			else cy.wrap($el).should('have.attr', 'data-app-id', 'files')
+		})
+	})
+
+	it('See the reset button is disabled', () => {
+		cy.get('[data-test-id="btn-apporder-reset"]').scrollIntoView()
+		cy.get('[data-test-id="btn-apporder-reset"]').should('be.visible').and('have.attr', 'disabled')
+	})
+
+	it('Change the app order', () => {
+		cy.get('[data-cy-app-order] [data-cy-app-order-element="files"] [data-cy-app-order-button="up"]').should('be.visible')
+		cy.get('[data-cy-app-order] [data-cy-app-order-element="files"] [data-cy-app-order-button="up"]').click()
+		cy.get('[data-cy-app-order] [data-cy-app-order-element="files"] [data-cy-app-order-button="up"]').should('not.be.visible')
+
+		cy.get('[data-cy-app-order] [data-cy-app-order-element]').each(($el, idx) => {
+			if (idx === 0) cy.wrap($el).should('have.attr', 'data-cy-app-order-element', 'files')
+			else cy.wrap($el).should('have.attr', 'data-cy-app-order-element', 'dashboard')
+		})
+	})
+
+	it('See the reset button is no longer disabled', () => {
+		cy.get('[data-test-id="btn-apporder-reset"]').scrollIntoView()
+		cy.get('[data-test-id="btn-apporder-reset"]').should('be.visible').and('not.have.attr', 'disabled')
+	})
+
+	it('Reset the app order', () => {
+		cy.get('[data-test-id="btn-apporder-reset"]').click({ force: true })
+	})
+
+	it('See the app order is restored', () => {
+		cy.get('[data-cy-app-order] [data-cy-app-order-element]').each(($el, idx) => {
+			if (idx === 0) cy.wrap($el).should('have.attr', 'data-cy-app-order-element', 'dashboard')
+			else cy.wrap($el).should('have.attr', 'data-cy-app-order-element', 'files')
+		})
+	})
+
+	it('See the reset button is disabled again', () => {
+		cy.get('[data-test-id="btn-apporder-reset"]').should('be.visible').and('have.attr', 'disabled')
+	})
+})