Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(arrowNavigation) - update way to listen for focus/blur events #10326

Merged
merged 3 commits into from
Aug 30, 2023
Merged

fix(arrowNavigation) - update way to listen for focus/blur events #10326

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d07f80a
listen for focus/blur events in container
Antreesy Aug 27, 2023
b513640
add method to reset current navigation
Antreesy Aug 27, 2023
8a2bf82
replace id on navigation items with data-* attribute
Antreesy Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/components/LeftSidebar/LeftSidebar.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
-
- @author Marco Ambrosini <marcoambrosini@icloud.com>
-
- @license GNU AGPL version 3 or any later version
- @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
Expand Down Expand Up @@ -167,6 +167,7 @@
<NcAppNavigationCaption :title="t('spreed', 'Users')" />
<NcListItem v-for="item of searchResultsUsers"
:key="`user_${item.id}`"
:anchor-id="`user_${item.id}`"
:title="item.label"
@click="createAndJoinConversation(item)">
<template #icon>
Expand All @@ -182,6 +183,7 @@
<NcAppNavigationCaption :title="t('spreed', 'Groups')" />
<NcListItem v-for="item of searchResultsGroups"
:key="`group_${item.id}`"
:anchor-id="`group_${item.id}`"
:title="item.label"
@click="createAndJoinConversation(item)">
<template #icon>
Expand All @@ -195,6 +197,7 @@
<NcAppNavigationCaption :title="t('spreed', 'Circles')" />
<NcListItem v-for="item of searchResultsCircles"
:key="`circle_${item.id}`"
:anchor-id="`circle_${item.id}`"
:title="item.label"
@click="createAndJoinConversation(item)">
<template #icon>
Expand Down Expand Up @@ -309,10 +312,11 @@ export default {
const leftSidebar = ref(null)
const searchBox = ref(null)

const { initializeNavigation } = useArrowNavigation(leftSidebar, searchBox)
const { initializeNavigation, resetNavigation } = useArrowNavigation(leftSidebar, searchBox, '.list-item')

return {
initializeNavigation,
resetNavigation,
leftSidebar,
searchBox,
}
Expand Down Expand Up @@ -510,6 +514,7 @@ export default {
}, 500)
},
debounceFetchSearchResults: debounce(function() {
this.resetNavigation()
if (this.isSearching) {
this.fetchSearchResults()
}
Expand Down Expand Up @@ -539,9 +544,6 @@ export default {
this.searchResultsGroups = this.searchResults.filter((match) => match.source === 'groups')
this.searchResultsCircles = this.searchResults.filter((match) => match.source === 'circles')
this.contactsLoading = false
this.$nextTick(() => {
this.initializeNavigation('.list-item')
})
} catch (exception) {
if (CancelableRequest.isCancel(exception)) {
return
Expand All @@ -563,9 +565,6 @@ export default {
const response = await request({ searchText: this.searchText })
this.searchResultsListedConversations = response.data.ocs.data
this.listedConversationsLoading = false
this.$nextTick(() => {
this.initializeNavigation('.list-item')
})
} catch (exception) {
if (CancelableRequest.isCancel(exception)) {
return
Expand All @@ -577,6 +576,7 @@ export default {

async fetchSearchResults() {
await Promise.all([this.fetchPossibleConversations(), this.fetchListedConversations()])
this.initializeNavigation()
},

/**
Expand Down
8 changes: 5 additions & 3 deletions src/components/LeftSidebar/NewGroupConversation/SetContacts/SetContacts.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
-
- @author Marco Ambrosini <marcoambrosini@icloud.com>
-
- @license GNU AGPL version 3 or any later version
- @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
Expand Down Expand Up @@ -97,10 +97,11 @@ export default {
const wrapper = ref(null)
const setContacts = ref(null)

const { initializeNavigation } = useArrowNavigation(wrapper, setContacts)
const { initializeNavigation, resetNavigation } = useArrowNavigation(wrapper, setContacts, '.participant-row')

return {
initializeNavigation,
resetNavigation,
wrapper,
setContacts,
}
Expand Down Expand Up @@ -176,6 +177,7 @@ export default {
},

debounceFetchSearchResults: debounce(function() {
this.resetNavigation()
this.fetchSearchResults()
}, 250),

Expand All @@ -196,7 +198,7 @@ export default {
this.cachedFullSearchResults = this.searchResults
}
this.$nextTick(() => {
this.initializeNavigation('.participant-row')
this.initializeNavigation()
})
} catch (exception) {
if (CancelableRequest.isCancel(exception)) {
Expand Down
3 changes: 2 additions & 1 deletion src/components/RightSidebar/Participants/ParticipantsList/Participant/Participant.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
-->

<template>
<li class="participant-row"
<li :id="`${participant.source}_${participant.id}`"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really required to be id here and anchor in the LeftSidebar?

ID must be unique on a whole page, and I'm really not sure that IDs like user_${item.id} are guaranteed to be unique across all applications on the page.

If it is actually only used as additional Node data to get item id from event.target?.id can we use some custom attr here? For example, data-arrow-navigation-id or at least data-id?

P.S. duplicated IDs is one of the most large a11y issue we currently have in Nextcloud...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the LeftSidebar they are used by RouterLink in <Conversation/> components, but not for the <NcListItem/> and <Participant/>
So I think, we could find a way to avoid it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the LeftSidebar they are used by RouterLink in <Conversation/>

But they were not used in Talk, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Router works without them, and I couldn't find anythig related or based on DOM anchor element id

They have an empty id attribute, but that's an upstream regression of NcListItem
image

class="participant-row"
:class="{
'offline': isOffline,
'currentUser': isSelf,
Expand Down
72 changes: 51 additions & 21 deletions src/composables/useArrowNavigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { onMounted, ref, unref } from 'vue'
import { computed, onMounted, ref, unref } from 'vue'

/**
* Mount navigation according to https://www.w3.org/WAI/GL/wiki/Using_ARIA_menus
* Item elements should have a specific selector and unique id
* ArrowDown or ArrowUp keys - to move through the itemElements list
* Enter key - to focus first element and click it
* (if confirmEnter = true) first Enter keydown to focus first element, second - to click selected
Expand All @@ -31,16 +32,41 @@ import { onMounted, ref, unref } from 'vue'
*
* @param {import('vue').Ref | HTMLElement} listElementRef component ref to mount navigation
* @param {import('vue').Ref} defaultElementRef component ref to return focus to // Vue component
* @param {string} selector native selector of elements to look for
* @param {object} options navigation options
* @param {boolean} [options.confirmEnter=false] flag to confirm Enter click
*/
export function useArrowNavigation(listElementRef, defaultElementRef, options = { confirmEnter: false }) {
export function useArrowNavigation(listElementRef, defaultElementRef, selector, options = { confirmEnter: false }) {
const listRef = ref(null)
const defaultRef = ref(null)

const itemElements = ref([])
const itemElementsIdMap = computed(() => itemElements.value.map(item => item.id))
const itemSelector = ref(selector)

const focusedIndex = ref(null)
const isConfirmationEnabled = ref(null)

// Set focused index according to selected element
const handleFocusEvent = (event) => {
const newIndex = itemElementsIdMap.value.indexOf(event.target?.id)

// Quit if triggered by arrow navigation as already handled
// or if using Tab key to navigate, and going through NcActions
if (focusedIndex.value !== newIndex && newIndex !== -1) {
focusedIndex.value = newIndex
}
}

// Reset focused index if focus moved out of navigation area or moved to the defaultRef
const handleBlurEvent = (event) => {
if (!listRef.value.contains(event.relatedTarget)
|| defaultRef.value?.$el.contains(event.relatedTarget)
|| defaultRef.value.contains?.(event.relatedTarget)) {
DorraJaouad marked this conversation as resolved.
Show resolved Hide resolved
focusedIndex.value = null
}
}

// Add event listeners for navigation list and set a default focus element
onMounted(() => {
// depending on ref, listElementRef could be either a component or a DOM element
Expand All @@ -49,7 +75,7 @@ export function useArrowNavigation(listElementRef, defaultElementRef, options =
isConfirmationEnabled.value = options.confirmEnter

listRef.value.addEventListener('keydown', (event) => {
if (itemElements.value?.length) {
if (itemElementsIdMap.value?.length) {
if (event.key === 'ArrowDown') {
focusNextElement(event)
} else if (event.key === 'ArrowUp') {
Expand All @@ -64,25 +90,26 @@ export function useArrowNavigation(listElementRef, defaultElementRef, options =
})

/**
* Collect all DOM elements specified by selector
*
* @param {string} selector selector to look for
* Update list of navigate-able elements specified by selector.
* Put a listener for focus/blur events on navigation area
*/
function initializeNavigation(selector) {
itemElements.value = Array.from(listRef.value.querySelectorAll(selector))
function initializeNavigation() {
itemElements.value = Array.from(listRef.value.querySelectorAll(itemSelector.value))
focusedIndex.value = null

itemElements.value.forEach((item, index) => {
item.addEventListener('focus', (event) => {
focusedIndex.value = index
})
listRef.value.addEventListener('focus', handleFocusEvent, true)
listRef.value.addEventListener('blur', handleBlurEvent, true)
}

item.addEventListener('blur', (event) => {
if (!itemElements.value.includes(event.relatedTarget)) {
focusedIndex.value = null
}
})
})
/**
* Remove listeners from navigation area, reset list of elements
* (to made navigation unavailable during fetching results)
*/
function resetNavigation() {
itemElements.value = []

listRef.value.removeEventListener('focus', handleFocusEvent, true)
listRef.value.removeEventListener('blur', handleBlurEvent, true)
}

/**
Expand Down Expand Up @@ -127,7 +154,9 @@ export function useArrowNavigation(listElementRef, defaultElementRef, options =
nativelyFocusElement(0)

// if confirmEnter = false, first Enter keydown clicks on item, otherwise only focuses it
if (!isConfirmationEnabled.value && event?.key === 'Enter') {
// Additionally check whether the Element is still in the DOM
if (!isConfirmationEnabled.value && event?.key === 'Enter'
&& listRef.value.contains(itemElements.value[0])) {
itemElements.value[0].click()
}
}
Expand All @@ -145,7 +174,7 @@ export function useArrowNavigation(listElementRef, defaultElementRef, options =
return
}

if (focusedIndex.value < itemElements.value.length - 1) {
if (focusedIndex.value < itemElementsIdMap.value.length - 1) {
nativelyFocusElement(focusedIndex.value + 1)
} else {
nativelyFocusElement(0)
Expand All @@ -166,11 +195,12 @@ export function useArrowNavigation(listElementRef, defaultElementRef, options =
if (focusedIndex.value > 0) {
nativelyFocusElement(focusedIndex.value - 1)
} else {
nativelyFocusElement(itemElements.value.length - 1)
nativelyFocusElement(itemElementsIdMap.value.length - 1)
}
}

return {
initializeNavigation,
resetNavigation,
}
}