Merge pull request #954 from nextcloud-libraries/brackport/934

[stable4] Fix FilePicker keyboard handling

README.md

@@ -101,7 +101,19 @@ const paths = await filepicker.pick()
 </script>
 ```
 
-## Releasing a new version
+## Development
+### Testing
+For testing all components provide `data-testid` attributes as selectors, so the tests are independent from code or styling changes.
+
+### Test selectors
+`data-testid` | Intended purpose
+---|---
+`select-all-checkbox` | The select all checkbox of the file list
+`file-list-row`       | A row in the file list (`tr`), can be identified by `data-filename`
+`row-checkbox`        | Checkbox for selecting a row
+`row-name`            | Name of the row / file
+
+### Releasing a new version
 
 - Pull the latest changes from `master` or `stableX`;
 - Checkout a new branch with the tag name (e.g `v4.0.1`): `git checkout -b v<version>`;

l10n/messages.pot

@@ -102,7 +102,7 @@ msgstr ""
 msgid "Undo"
 msgstr ""
 
-#: lib/components/FilePicker/FileListRow.vue:26
+#: lib/components/FilePicker/FileListRow.vue:34
 msgid "Unset"
 msgstr ""
 

lib/components/FilePicker/FileList.spec.ts

@@ -120,11 +120,12 @@ describe('FilePicker FileList', () => {
 				path: '/',
 			},
 		})
-		expect(wrapper.find('th.row-checkbox').exists()).toBe(true)
+		const selectAll = wrapper.find('[data-testid="select-all-checkbox"]')
+		expect(selectAll.exists()).toBe(true)
 		// there is an aria label
-		expect(wrapper.find('[data-test="file-picker_select-all"]').attributes('aria-label')).toBeTruthy()
+		expect(selectAll.attributes('aria-label')).toBeTruthy()
 		// no checked
-		expect(wrapper.find('[data-test="file-picker_select-all"]').props('checked')).toBe(false)
+		expect(selectAll.props('checked')).toBe(false)
 	})
 
 	it('header checkbox is checked when all nodes are selected', async () => {
@@ -140,7 +141,7 @@ describe('FilePicker FileList', () => {
 			},
 		})
 
-		const selectAll = wrapper.find('[data-test="file-picker_select-all"]')
+		const selectAll = wrapper.find('[data-testid="select-all-checkbox"]')
 		expect(selectAll.props('checked')).toBe(true)
 	})
 
@@ -158,15 +159,15 @@ describe('FilePicker FileList', () => {
 				},
 			})
 
-			const rows = wrapper.findAll('.file-picker__row')
+			const rows = wrapper.findAll('[data-testid="file-list-row"]')
 			// all nodes are shown
 			expect(rows.length).toBe(nodes.length)
 			// folder are sorted first
-			expect(rows.at(0).attributes('data-file')).toBe('directory')
+			expect(rows.at(0).attributes('data-filename')).toBe('directory')
 			// other files are ascending
-			expect(rows.at(1).attributes('data-file')).toBe('a-file.txt')
-			expect(rows.at(2).attributes('data-file')).toBe('b-file.txt')
-			expect(rows.at(3).attributes('data-file')).toBe('favorite.txt')
+			expect(rows.at(1).attributes('data-filename')).toBe('a-file.txt')
+			expect(rows.at(2).attributes('data-filename')).toBe('b-file.txt')
+			expect(rows.at(3).attributes('data-filename')).toBe('favorite.txt')
 		})
 
 		it('can sort descending by name', async () => {
@@ -188,11 +189,11 @@ describe('FilePicker FileList', () => {
 			// all nodes are shown
 			expect(rows.length).toBe(nodes.length)
 			// folder are sorted first
-			expect(rows.at(0).attributes('data-file')).toBe('directory')
+			expect(rows.at(0).attributes('data-filename')).toBe('directory')
 			// other files are descending
-			expect(rows.at(1).attributes('data-file')).toBe('favorite.txt')
-			expect(rows.at(2).attributes('data-file')).toBe('b-file.txt')
-			expect(rows.at(3).attributes('data-file')).toBe('a-file.txt')
+			expect(rows.at(1).attributes('data-filename')).toBe('favorite.txt')
+			expect(rows.at(2).attributes('data-filename')).toBe('b-file.txt')
+			expect(rows.at(3).attributes('data-filename')).toBe('a-file.txt')
 		})
 	})
 })

lib/components/FilePicker/FileList.vue

@@ -10,7 +10,7 @@
 						<NcCheckboxRadioSwitch v-if="multiselect"
 							:aria-label="t('Select all entries')"
 							:checked="allSelected"
-							data-test="file-picker_select-all"
+							data-testid="select-all-checkbox"
 							@update:checked="onSelectAll" />
 					</th>
 					<th :aria-sort="sortByName" class="row-name">

lib/components/FilePicker/FileListRow.spec.ts

@@ -0,0 +1,154 @@
+/**
+ * @copyright Copyright (c) 2023 Ferdinand Thiessen <opensource@fthiessen.de>
+ *
+ * @author Ferdinand Thiessen <opensource@fthiessen.de>
+ *
+ * @license AGPL-3.0-or-later
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+import { afterEach, describe, expect, it, vi } from 'vitest'
+import { mount } from '@vue/test-utils'
+import { File } from '@nextcloud/files'
+
+import FileListRow from './FileListRow.vue'
+
+// Mock OC.MimeType
+window.OC = {
+	MimeType: {
+		getIconUrl: (mime: string) => `/icon/${mime}`,
+	},
+} as never
+
+describe('FilePicker: FileListRow', () => {
+	const node = new File({
+		owner: null,
+		mtime: new Date(),
+		mime: 'text/plain',
+		source: 'https://example.com/dav/a.txt',
+		root: '/',
+	})
+
+	afterEach(() => {
+		vi.restoreAllMocks()
+	})
+
+	it('is mountable', () => {
+		const consoleWarn = vi.spyOn(console, 'warn')
+		const consoleError = vi.spyOn(console, 'error')
+
+		const wrapper = mount(FileListRow, {
+			propsData: {
+				allowPickDirectory: true,
+				selected: false,
+				showCheckbox: true,
+				canPick: true,
+				node,
+			},
+		})
+
+		// No console errors
+		expect(consoleWarn).not.toBeCalled()
+		expect(consoleError).not.toBeCalled()
+		// mounted
+		expect(wrapper.exists()).toBe(true)
+		expect(wrapper.element.tagName.toLowerCase()).toBe('tr')
+		expect(wrapper.find('[data-testid="file-list-row"]').isEmpty()).toBe(false)
+	})
+
+	it('shows checkbox based on `showCheckbox` property', async () => {
+		const wrapper = mount(FileListRow, {
+			propsData: {
+				allowPickDirectory: true,
+				selected: false,
+				showCheckbox: true,
+				canPick: true,
+				node,
+			},
+		})
+
+		expect(wrapper.find('[data-testid="row-checkbox"]').exists()).toBe(true)
+		await wrapper.setProps({ showCheckbox: false })
+		expect(wrapper.find('[data-testid="row-checkbox"]').exists()).toBe(false)
+	})
+
+	it('Click checkbox triggers select', async () => {
+		const wrapper = mount(FileListRow, {
+			propsData: {
+				allowPickDirectory: false,
+				selected: false,
+				showCheckbox: true,
+				canPick: true,
+				node,
+			},
+		})
+
+		await wrapper.find('[data-testid="row-checkbox"]').trigger('click')
+
+		// one event with payload `true` is expected
+		expect(wrapper.emitted('update:selected')).toEqual([[true]])
+	})
+
+	it('Click element triggers select', async () => {
+		const wrapper = mount(FileListRow, {
+			propsData: {
+				allowPickDirectory: false,
+				selected: false,
+				showCheckbox: true,
+				canPick: true,
+				node,
+			},
+		})
+
+		await wrapper.find('[data-testid="row-name"]').trigger('click')
+
+		// one event with payload `true` is expected
+		expect(wrapper.emitted('update:selected')).toEqual([[true]])
+	})
+
+	it('Click element without checkbox triggers select', async () => {
+		const wrapper = mount(FileListRow, {
+			propsData: {
+				allowPickDirectory: false,
+				selected: false,
+				showCheckbox: false,
+				canPick: true,
+				node,
+			},
+		})
+
+		await wrapper.find('[data-testid="row-name"]').trigger('click')
+
+		// one event with payload `true` is expected
+		expect(wrapper.emitted('update:selected')).toEqual([[true]])
+	})
+
+	it('Enter triggers select', async () => {
+		const wrapper = mount(FileListRow, {
+			propsData: {
+				allowPickDirectory: false,
+				selected: false,
+				showCheckbox: false,
+				canPick: true,
+				node,
+			},
+		})
+
+		await wrapper.trigger('keydown', { key: 'Enter', bubbles: true })
+
+		expect(wrapper.emitted('update:selected')).toEqual([[true]])
+	})
+})

lib/components/FilePicker/FileListRow.vue

@@ -1,18 +1,26 @@
 <template>
-	<tr tabindex="0"
+	<tr :tabindex="(showCheckbox && !isDirectory) ? undefined : 0"
+		:aria-selected="!isPickable ? undefined : selected"
 		:class="['file-picker__row', {
 			'file-picker__row--selected': selected && !showCheckbox
 		}]"
-		:data-file="node.basename"
-		@key-down="handleKeyDown">
+		:data-filename="node.basename"
+		data-testid="file-list-row"
+		@click="handleClick"
+		v-on="
+			/* same as tabindex -> if we hide the checkbox or this is a directory we need keyboard access to enter the directory or select the node */
+			(!showCheckbox || isDirectory) ? { keydown: handleKeyDown } : {}
+		">
 		<td v-if="showCheckbox" class="row-checkbox">
 			<NcCheckboxRadioSwitch :disabled="!isPickable"
 				:checked="selected"
 				:aria-label="t('Select the row for {nodename}', { nodename: displayName })"
+				data-testid="row-checkbox"
+				@click.stop="/* just stop the click event */"
 				@update:checked="toggleSelected" />
 		</td>
-		<td class="row-name" @click="handleClick">
-			<div class="file-picker__name-container">
+		<td class="row-name">
+			<div class="file-picker__name-container" data-testid="row-name">
 				<div class="file-picker__file-icon" :style="{ backgroundImage }" />
 				<div class="file-picker__file-name" :title="displayName" v-text="displayName" />
 				<div class="file-picker__file-extension" v-text="fileExtension" />
@@ -65,10 +73,15 @@ const displayName = computed(() => props.node.attributes?.displayName || props.n
  */
 const fileExtension = computed(() => props.node.extension)
 
+/**
+ * Check if the node is a directory
+ */
+const isDirectory = computed(() => props.node.type === FileType.Folder)
+
 /**
  * If this node can be picked, basically just check if picking a directory is allowed
  */
-const isPickable = computed(() => props.canPick && (props.allowPickDirectory || props.node.mime !== 'httpd/unix-directory'))
+const isPickable = computed(() => props.canPick && (props.allowPickDirectory || !isDirectory.value))
 
 /**
  * Background image url for the given nodes mime type
@@ -86,7 +99,7 @@ function toggleSelected() {
  * Handle clicking the table row, if it is a directory it is opened, else selected
  */
 function handleClick() {
-	if (props.node.mime === 'httpd/unix-directory') {
+	if (isDirectory.value) {
 		emit('enter-directory', props.node)
 	} else {
 		toggleSelected()
@@ -98,7 +111,7 @@ function handleClick() {
  * @param event The Keydown event
  */
 function handleKeyDown(event: KeyboardEvent) {
-	if (event.key === 'enter') {
+	if (event.key === 'Enter') {
 		handleClick()
 	}
 }

vitest.config.ts

@@ -4,6 +4,7 @@ import config from './vite.config'
 export default defineConfig(async (env) => {
 	const viteConfig = (await config(env))
 	delete viteConfig.define
+
 	return {
 		...viteConfig,
 		test: {
@@ -13,6 +14,7 @@ export default defineConfig(async (env) => {
 				include: ['lib/**/*.ts', 'lib/*.ts'],
 				exclude: ['lib/**/*.spec.ts'],
 			},
+			// Fix unresolvable .css extension for ssr
 			server: {
 				deps: {
 					inline: [/@nextcloud\/vue/],