fix(NcAppNavigation): add focus trap on mobile and improve a11y

src/components/NcAppNavigation/NcAppNavigation.vue

@@ -54,14 +54,17 @@ emit('toggle-navigation', {
 </docs>
 
 <template>
-	<div id="app-navigation-vue"
+	<div ref="appNavigationContainer"
 		class="app-navigation"
-		role="navigation"
 		:class="{'app-navigation--close':!open }">
 		<NcAppNavigationToggle :open="open" @update:open="toggleNavigation" />
-		<div :aria-hidden="ariaHidden"
+		<nav id="app-navigation-vue"
+			:aria-hidden="open ? 'false' : 'true'"
+			:aria-label="ariaLabel || undefined"
+			:aria-labelledby="ariaLabelledby || undefined"
 			class="app-navigation__content"
-			:inert="!open || null">
+			:inert="!open || undefined"
+			@keydown.esc="handleEsc">
 			<slot />
 			<!-- List for Navigation li-items -->
 			<ul class="app-navigation__list">
@@ -70,16 +73,19 @@ emit('toggle-navigation', {
 
 			<!-- Footer for e.g. AppNavigationSettings -->
 			<slot name="footer" />
-		</div>
+		</nav>
 	</div>
 </template>
 
 <script>
 import NcAppNavigationToggle from '../NcAppNavigationToggle/index.js'
 import isMobile from '../../mixins/isMobile/index.js'
+import { getTrapStack } from '../../utils/focusTrap.js'
 
 import { emit, subscribe, unsubscribe } from '@nextcloud/event-bus'
 
+import { createFocusTrap } from 'focus-trap'
+
 export default {
 	name: 'NcAppNavigation',
 
@@ -89,20 +95,38 @@ export default {
 
 	mixins: [isMobile],
 
+	props: {
+		/**
+		 * The aria label to describe the navigation
+		 */
+		ariaLabel: {
+			type: String,
+			default: '',
+		},
+
+		/**
+		 * aria-labelledby attribute to describe the navigation
+		 */
+		ariaLabelledby: {
+			type: String,
+			default: '',
+		},
+	},
+
 	data() {
 		return {
 			open: true,
+			focusTrap: null,
 		}
 	},
-	computed: {
-		ariaHidden() {
-			return this.open ? 'false' : 'true'
-		},
-	},
 
 	watch: {
 		isMobile() {
 			this.open = !this.isMobile
+			this.toggleFocusTrap()
+		},
+		open() {
+			this.toggleFocusTrap()
 		},
 	},
 
@@ -112,9 +136,18 @@ export default {
 		emit('navigation-toggled', {
 			open: this.open,
 		})
+
+		this.focusTrap = createFocusTrap(this.$refs.appNavigationContainer, {
+			allowOutsideClick: true,
+			fallbackFocus: this.$refs.appNavigationContainer,
+			trapStack: getTrapStack(),
+			escapeDeactivates: false,
+		})
+		this.toggleFocusTrap()
 	},
 	unmounted() {
 		unsubscribe('toggle-navigation', this.toggleNavigationByEventBus)
+		this.focusTrap.deactivate()
 	},
 
 	methods: {
@@ -135,9 +168,27 @@ export default {
 			// We wait for 1.5 times the animation length to give the animation time to really finish.
 			}, 1.5 * animationLength)
 		},
+
 		toggleNavigationByEventBus({ open }) {
 			this.toggleNavigation(open)
 		},
+
+		/**
+		 * Activate focus trap if it is currently needed, otherwise deactivate
+		 */
+		toggleFocusTrap() {
+			if (this.isMobile && this.open) {
+				this.focusTrap.activate()
+			} else {
+				this.focusTrap.deactivate()
+			}
+		},
+
+		handleEsc() {
+			if (this.isMobile) {
+				this.toggleNavigation(false)
+			}
+		},
 	},
 }
 </script>

tests/unit/components/NcAppNavigation/NcAppNavigation.spec.js

@@ -19,33 +19,172 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  */
-
+import { describe, it, expect, afterEach } from '@jest/globals'
 import { mount } from '@vue/test-utils'
 import { emit } from '@nextcloud/event-bus'
+import { nextTick } from 'vue'
 import NcAppNavigation from '../../../../src/components/NcAppNavigation/NcAppNavigation.vue'
+import { IsMobileState } from '../../../../src/utils/IsMobileState.js'
 
-describe('NcAppNavigation.vue', () => {
-	'use strict'
+const NAVIGATION__SELECTOR = 'nav'
+const TOGGLE_BUTTON__SELECTOR = 'button[aria-controls="app-navigation-vue"]'
+const NAVIGATION_CLOSE__CLASS = 'app-navigation--close'
 
+const findNavigation = (wrapper) => wrapper.get(NAVIGATION__SELECTOR)
+const findToggleButton = (wrapper) => wrapper.get(TOGGLE_BUTTON__SELECTOR)
+
+describe('NcAppNavigation.vue', () => {
 	describe('by default', () => {
 		it('is open', () => {
 			const wrapper = mount(NcAppNavigation)
-			expect(wrapper.vm.$data.open).toBe(true)
+			const navigation = findNavigation(wrapper)
+
+			expect(navigation.classes(NAVIGATION_CLOSE__CLASS)).toBeFalsy()
 		})
 	})
+
 	describe('toggle via event bus', () => {
-		it('toggle to turn it off', () => {
+		it('toggles to turn it off', async () => {
+			const wrapper = mount(NcAppNavigation)
+
+			emit('toggle-navigation', { open: undefined })
+			await nextTick()
+
+			expect(wrapper.classes(NAVIGATION_CLOSE__CLASS)).toBeTruthy()
+		})
+
+		it('toggles with open: false keeps it closed', async () => {
+			const wrapper = mount(NcAppNavigation)
+
+			emit('toggle-navigation', { open: false })
+			emit('toggle-navigation', { open: false })
+			await nextTick()
+
+			expect(wrapper.classes(NAVIGATION_CLOSE__CLASS)).toBeTruthy()
+		})
+	})
+
+	describe('toggle via toggle button', () => {
+		it('opens on toggle button click in closed navigation', async () => {
+			const wrapper = mount(NcAppNavigation)
+			const togglebutton = findToggleButton(wrapper)
+
+			await togglebutton.trigger('click')
+
+			expect(wrapper.classes(NAVIGATION_CLOSE__CLASS)).toBeTruthy()
+		})
+	})
+
+	describe('toggle via mobile state', () => {
+		afterEach(() => {
+			IsMobileState.isMobile = false
+		})
+
+		it('closes on switch to mobile', async () => {
+			const wrapper = mount(NcAppNavigation)
+
+			IsMobileState.isMobile = true
+			await nextTick()
+
+			expect(wrapper.classes(NAVIGATION_CLOSE__CLASS)).toBeTruthy()
+		})
+
+		it('opens on switch to desktop', async () => {
+			IsMobileState.isMobile = true
+			const wrapper = mount(NcAppNavigation)
+			const togglebutton = findToggleButton(wrapper)
+
+			// Close
+			await togglebutton.trigger('click')
+			// Switch to desktop
+			IsMobileState.isMobile = false
+			await nextTick()
+
+			expect(wrapper.classes(NAVIGATION_CLOSE__CLASS)).toBeFalsy()
+		})
+
+		it('closes by ESC key on mobile', async () => {
+			IsMobileState.isMobile = true
+			const wrapper = mount(NcAppNavigation)
+			const navigation = findNavigation(wrapper)
+
+			await navigation.trigger('keydown', { key: 'Escape' })
+
+			expect(wrapper.classes(NAVIGATION_CLOSE__CLASS)).toBeTruthy()
+		})
+
+		it("doesn't close by ESC key on desktop", async () => {
 			const wrapper = mount(NcAppNavigation)
-			emit('toggle-navigation', {open: undefined})
-			expect(wrapper.vm.$data.open).toBe(false)
+			const navigation = findNavigation(wrapper)
+
+			await navigation.trigger('keydown', { key: 'Escape' })
+
+			expect(wrapper.classes(NAVIGATION_CLOSE__CLASS)).toBeFalsy()
 		})
+	})
 
-		it('toggle with open: false keeps it closed', () => {
+	describe('focus trap', () => {
+		// TODO
+	})
+
+	describe('accessibility', () => {
+		it('has navigation role', () => {
 			const wrapper = mount(NcAppNavigation)
-			emit('toggle-navigation', {open: false})
-			emit('toggle-navigation', {open: false})
-			expect(wrapper.vm.$data.open).toBe(false)
+			const navigation = findNavigation(wrapper)
+
+			expect(
+				navigation.attributes('role') === 'navigation'
+				|| navigation.element.tagName === 'NAV',
+			).toBeTruthy()
 		})
 
+		it('has toggle button, connected by aria-controls', () => {
+			const wrapper = mount(NcAppNavigation)
+			const navigation = findNavigation(wrapper)
+			const togglebutton = findToggleButton(wrapper)
+
+			expect(togglebutton.attributes('aria-controls')).toBe(navigation.attributes('id'))
+		})
+
+		it('has correct aria attributes and inert on open navigation', () => {
+			const wrapper = mount(NcAppNavigation)
+			const navigation = findNavigation(wrapper)
+			const togglebutton = findToggleButton(wrapper)
+
+			expect(navigation.attributes('aria-hidden')).toBe('false')
+			expect(navigation.attributes('inert')).toBeFalsy()
+			expect(togglebutton.attributes('aria-expanded')).toBe('true')
+			expect(togglebutton.attributes('aria-label')).toBe('Close navigation')
+		})
+
+		it('has correct aria attributes and inert on closed navigation', async () => {
+			const wrapper = mount(NcAppNavigation)
+			const navigation = findNavigation(wrapper)
+			const togglebutton = findToggleButton(wrapper)
+
+			// Close navigation
+			await togglebutton.trigger('click')
+
+			expect(navigation.attributes('aria-hidden')).toBe('true')
+			expect(navigation.attributes('inert')).toBeTruthy()
+			expect(togglebutton.attributes('aria-expanded')).toBe('false')
+			expect(togglebutton.attributes('aria-label')).toBe('Open navigation')
+		})
+
+		it('has aria-label from corresponding prop on navigation', () => {
+			const wrapper = mount(NcAppNavigation, {
+				propsData: { ariaLabel: 'Navigation' },
+			})
+			const navigation = findNavigation(wrapper)
+			expect(navigation.attributes('aria-label')).toBe('Navigation')
+		})
+
+		it('has aria-labelledby from corresponding prop on navigation', () => {
+			const wrapper = mount(NcAppNavigation, {
+				propsData: { ariaLabelledby: 'navHeaderId' },
+			})
+			const navigation = findNavigation(wrapper)
+			expect(navigation.attributes('aria-labelledby')).toBe('navHeaderId')
+		})
 	})
 })