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

Feature/focus mgmt main menu #2101

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Feature/focus mgmt main menu #2101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,5 +119,9 @@
"yarn lint --fix",
"git add"
]
},
"dependencies": {
"deepmerge": "^4.0.0",
"inert-polyfill": "^0.2.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting. so we could in theory use this attribute for all elements whenever a modal is open. might need to track the info whether a modal is open in the global store.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inert does make sense in general when there is need for rendering parts of the DOM inert/inactive/unreachable for assistive technologies/keyboard users. The elements "behind" an open modal are the most prominent use case: https://github.com/WICG/inert/blob/master/README.md

}
}
1 change: 1 addition & 0 deletions src/Phoenix.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
</div>
</template>
<script>
import 'inert-polyfill'
import { mapGetters, mapState, mapActions } from 'vuex'
import TopBar from './components/Top-Bar.vue'
import Menu from './components/Menu.vue'
Expand Down
18 changes: 16 additions & 2 deletions src/components/Menu.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
<template>
<oc-application-menu name="coreMenu" v-model="sidebarIsVisible" @close="sidebarIsVisible = false">
<oc-application-menu name="coreMenu" v-model="sidebarIsVisible" :inert="!sidebarIsVisible" @close="sidebarIsVisible = false" ref="sidebar">
<oc-sidebar-nav-item v-for="(n, nid) in nav" :active="isActive(n)" :key="nid" :icon="n.iconMaterial" :target="n.route ? n.route.path : null" @click="openItem(n.url)">{{ translateMenu(n) }}</oc-sidebar-nav-item>

<oc-sidebar-nav-item icon="account_circle" target="/account" :isolate="true">
<translate>Account</translate>
</oc-sidebar-nav-item>
Expand All @@ -26,6 +25,11 @@ export default {
watch: {
$route () {
this.toggleSidebar(false)
},
sidebarIsVisible (val) {
if (val) {
this.focusFirstLink()
}
}
},
computed: {
Expand Down Expand Up @@ -76,6 +80,16 @@ export default {
},
isActive (navItem) {
return navItem.route.name === this.$route.name
},
focusFirstLink () {
/*
* Delay for two reasons:
* - for screen readers Virtual buffer
* - to outsmart uikit's focus management
*/
setTimeout(() => {
this.$refs.sidebar.$el.querySelector('a:first-of-type').focus()
}, 500)
}
}
}
Expand Down
14 changes: 13 additions & 1 deletion src/components/Top-Bar.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<template>
<oc-navbar id="oc-topbar" tag="header" class="oc-topbar">
<oc-navbar-item position="left">
<oc-button icon="menu" variation="primary" class="oc-topbar-menu-burger uk-height-1-1" aria-label="Menu" @click="toggleSidebar(!isSidebarVisible)" v-if="!publicPage()">
<oc-button icon="menu" variation="primary" class="oc-topbar-menu-burger uk-height-1-1" aria-label="Menu" @click="toggleSidebar(!isSidebarVisible)" v-if="!publicPage()" ref="menubutton">
<span class="oc-topbar-menu-burger-label" v-translate>Menu</span>
</oc-button>
</oc-navbar-item>
Expand Down Expand Up @@ -66,6 +66,18 @@ export default {
if (this.intervalId) {
clearInterval(this.intervalId)
}
},
watch: {
isSidebarVisible: function (val) {
if (!val) {
/*
* Delay for screen readers Virtual buffers
*/
setTimeout(() => {
this.$refs.menubutton.$el.focus()
}, 500)
Copy link
Contributor

Choose a reason for hiding this comment

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

does it need to be a fixed delay value or can it be done with this.$next() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

500-100ms is a recommended value. setTimeout 0 or this.$next are too short, we have to wait for the Virtual Buffer of the screen reader. This can't be perceived via JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

since we are using this patterns in multiple places, I wonder if there is a way to make it more Vue-y.

Maybe implement something like this.$next but call it this.$afterScreenReaderInit or something so that it becomes self-explanatory ? The latter implementation can then contain whatever value we chose for the delay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

afterScreenReaderInit sounds like an actual hook/lifecycle event. I would go with delayForScreenreader or the like. Emphasising that it is a form of guessing

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcus-herrmann your proposal sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PVince81 So this would be eventually a Vue mixin which potentially every component can access, and it its core a wrapper around a setTimeout in order to not need to comment it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcus-herrmann yes, that would be a way.

One question that does come to mind is how can developers know when to use it and when not to. Does this need some experimentation with accessibility test systems or is there a common pattern ? If the latter, it could be included in the design guidelines in some form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PVince81 It's always better to test/experiment with assistive technology, but I can devote a particular page in the design guidelines about screen readers and problems with web-apps (which are, predominantly, DOM-changing, asynchronously loading, stateful constructs, all of which makes them kind of hard to grasp for screen readers). Sending focus to elements in dynamic DOM parts of the application (like this, or after routing) would be just one part of the challenges, accessible notifications, live search and filtering would be others.

}
}
}
}
</script>