From 4a12c1365fd2551c045c2877bdb6797c4339ecbc Mon Sep 17 00:00:00 2001
From: "Grigorii K. Shartsev" <me@shgk.me>
Date: Tue, 23 Jan 2024 15:12:07 +0100
Subject: [PATCH] fix(NcButton): pressed state a11y

- area-pressed should be false when not pressed
- pressed should only be present on buttons, not links
- fix example to not change text on press

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
---
 src/components/NcButton/NcButton.vue          | 27 ++++---
 tests/unit/components/NcButton/button.spec.js | 70 ++++++++++++++-----
 2 files changed, 70 insertions(+), 27 deletions(-)

diff --git a/src/components/NcButton/NcButton.vue b/src/components/NcButton/NcButton.vue
index 33b129928a..961544a1a0 100644
--- a/src/components/NcButton/NcButton.vue
+++ b/src/components/NcButton/NcButton.vue
@@ -261,26 +261,32 @@ export default {
 ```
 
 ### Pressed state
+
 It is possible to make the button stateful by adding a pressed state, e.g. if you like to create a favorite button.
 The button will have the required `aria` attribute for accessibility and visual style (`primary` when pressed, and the configured type otherwise).
 
+Do not change `text` or `aria-label` of the pressed/unpressed button. See: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-pressed
+
 ```vue
 <template>
 	<div>
 		<div style="display: flex; gap: 12px;">
-			<NcButton :pressed.sync="isFavorite" :aria-label="ariaLabel" type="tertiary-no-background">
+			<NcButton :pressed.sync="isFavorite" aria-label="Favorite" type="tertiary-no-background">
 				<template #icon>
 					<IconStar v-if="isFavorite" :size="20" />
 					<IconStarOutline v-else :size="20" />
 				</template>
 			</NcButton>
-			<NcButton :pressed.sync="isFavorite" :aria-label="ariaLabel" type="tertiary">
+
+			<NcButton :pressed.sync="isFavorite" type="tertiary">
 				<template #icon>
 					<IconStar v-if="isFavorite" :size="20" />
 					<IconStarOutline v-else :size="20" />
 				</template>
+				Favorite
 			</NcButton>
-			<NcButton :pressed.sync="isFavorite" :aria-label="ariaLabel">
+
+			<NcButton :pressed.sync="isFavorite" aria-label="Favorite">
 				<template #icon>
 					<IconStar v-if="isFavorite" :size="20" />
 					<IconStarOutline v-else :size="20" />
@@ -306,11 +312,6 @@ export default {
 			isFavorite: false,
 		}
 	},
-	computed: {
-		ariaLabel() {
-			return this.isFavorite ? 'Remove as favorite' : 'Add as favorite'
-		},
-	},
 	methods: {
 		toggleFavorite() {
 			this.isFavorite = !this.isFavorite
@@ -558,6 +559,8 @@ export default {
 		/**
 		 * The pressed state of the button if it has a checked state
 		 * This will add the `aria-pressed` attribute and for the button to have the primary style in checked state.
+		 *
+		 * Pressed state is not supported for links
 		 */
 		pressed: {
 			type: Boolean,
@@ -621,6 +624,9 @@ export default {
 		}
 
 		const isLink = (this.to || this.href)
+
+		const hasPressed = !isLink && typeof this.pressed === 'boolean'
+
 		const renderButton = ({ href, navigate, isActive, isExactActive } = {}) => h(isLink ? 'a' : 'button',
 			{
 				class: [
@@ -639,7 +645,7 @@ export default {
 				],
 				attrs: {
 					'aria-label': this.ariaLabel,
-					'aria-pressed': this.pressed,
+					'aria-pressed': hasPressed ? this.pressed.toString() : undefined,
 					disabled: this.disabled,
 					type: isLink ? null : this.nativeType,
 					role: isLink ? 'button' : null,
@@ -655,8 +661,7 @@ export default {
 				on: {
 					...this.$listeners,
 					click: ($event) => {
-						// Update pressed prop on click if it is set
-						if (typeof this.pressed === 'boolean') {
+						if (hasPressed) {
 							/**
 							 * Update the current pressed state of the button (if the `pressed` property was configured)
 							 *
diff --git a/tests/unit/components/NcButton/button.spec.js b/tests/unit/components/NcButton/button.spec.js
index e360a5d62e..920eea14f7 100644
--- a/tests/unit/components/NcButton/button.spec.js
+++ b/tests/unit/components/NcButton/button.spec.js
@@ -24,22 +24,60 @@ import { shallowMount } from '@vue/test-utils'
 import NcButton from '../../../../src/components/NcButton/NcButton.vue'
 
 describe('NcButton', () => {
-	it('emits update:pressed', async () => {
-		const wrapper = shallowMount(NcButton, { propsData: { pressed: true, ariaLabel: 'button' } })
-		wrapper.findComponent('button').trigger('click')
-		expect(wrapper.emitted('update:pressed')?.length).toBe(1)
-		expect(wrapper.emitted('update:pressed')[0]).toEqual([false])
-
-		// Now the same but when pressed was false
-		await wrapper.setProps({ pressed: false })
-		wrapper.findComponent('button').trigger('click')
-		expect(wrapper.emitted('update:pressed')?.length).toBe(2)
-		expect(wrapper.emitted('update:pressed')[1]).toEqual([true])
-	})
+	describe('pressed', () => {
+		it('has aria-pressed="true" when pressed', () => {
+			const wrapper = shallowMount(NcButton, { propsData: { ariaLabel: 'button', pressed: true } })
+			const button = wrapper.find('button')
+			expect(button.attributes('aria-pressed')).toBe('true')
+		})
+
+		it('has aria-pressed="false" when not pressed', () => {
+			const wrapper = shallowMount(NcButton, { propsData: { ariaLabel: 'button', pressed: false } })
+			const button = wrapper.find('button')
+			expect(button.attributes('aria-pressed')).toBe('false')
+		})
+
+		it('has no aria-pressed when pressed is not specified', () => {
+			const wrapper = shallowMount(NcButton, { propsData: { ariaLabel: 'button' } })
+			const button = wrapper.find('button')
+			expect(button.attributes('aria-pressed')).not.toBeDefined()
+		})
+
+		it('has no aria-pressed when pressed is null', () => {
+			const wrapper = shallowMount(NcButton, { propsData: { ariaLabel: 'button', pressed: null } })
+			const button = wrapper.find('button')
+			expect(button.attributes('aria-pressed')).not.toBeDefined()
+		})
+
+		it('has no aria-pressed for a link', () => {
+			const wrapper = shallowMount(NcButton, { propsData: { ariaLabel: 'button', pressed: true, href: 'http://example.com' } })
+			const button = wrapper.find('a')
+			expect(button.attributes('aria-pressed')).not.toBeDefined()
+		})
+
+		it('toggles pressed on pressed update', async () => {
+			const wrapper = shallowMount(NcButton, { propsData: { ariaLabel: 'button', pressed: true } })
+			await wrapper.setProps({ pressed: false })
+			expect(wrapper.find('button').attributes('aria-pressed')).toBe('false')
+		})
+
+		it('emits update:pressed on press with a new pressed state', () => {
+			const wrapper = shallowMount(NcButton, { propsData: { ariaLabel: 'button', pressed: true } })
+			wrapper.find('button').trigger('click')
+			expect(wrapper.emitted('update:pressed')?.length).toBe(1)
+			expect(wrapper.emitted('update:pressed')[0]).toEqual([false])
+		})
+
+		it('does not emit update:pressed on press when pressed is not specified', async () => {
+			const wrapper = shallowMount(NcButton, { propsData: { ariaLabel: 'button' } })
+			wrapper.find('button').trigger('click')
+			expect(wrapper.emitted('update:pressed')).toBe(undefined)
+		})
 
-	it('does not emit update:pressed when not configured', async () => {
-		const wrapper = shallowMount(NcButton, { propsData: { ariaLabel: 'button' } })
-		wrapper.findComponent('button').trigger('click')
-		expect(wrapper.emitted('update:pressed')).toBe(undefined)
+		it('does not emit update:pressed on press for a link', async () => {
+			const wrapper = shallowMount(NcButton, { propsData: { ariaLabel: 'button', pressed: true, href: 'http://example.com' } })
+			wrapper.find('a').trigger('click')
+			expect(wrapper.emitted('update:pressed')).toBe(undefined)
+		})
 	})
 })