Feature/focus mgmt main menu #2101
Conversation
marcus-herrmann
requested review from
DeepDiver1975,
individual-it and
LukasHirt
|
|
| */ | ||
| setTimeout(() => { | ||
| this.$refs.menubutton.$el.focus() | ||
| }, 500) |
There was a problem hiding this comment.
does it need to be a fixed delay value or can it be done with this.$next() ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
afterScreenReaderInit sounds like an actual hook/lifecycle event. I would go with delayForScreenreader or the like. Emphasising that it is a form of guessing
There was a problem hiding this comment.
@marcus-herrmann your proposal sounds good
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
| @@ -118,6 +118,7 @@ | |||
| ] | |||
| }, | |||
| "dependencies": { | |||
| "deepmerge": "^4.0.0" | |||
| "deepmerge": "^4.0.0", | |||
| "inert-polyfill": "^0.2.5" | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
I had a quick test and focus management looks fine for the regular case |
|
After the click I get a broken cla-assistant.io page and: "Error - There is no CLA to sign for owncloud/phoenix" |
|
@DeepDiver1975 can you confirm the "no CLA" case ? I also get a rather broken page from the CLA bot when I open https://cla-assistant.io/owncloud/phoenix?pullRequest=2101 |
|
@DeepDiver1975 Could you point me towards what you mean by dependency conflict? |
Conflict inside of package.json |
💥 Acceptance tests Files failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/phoenix/5767/
|
Just was able to sign the CLA. |
|
@marcus-herrmann @LukasHirt can we move this forward ? I'll need the inert functionality soon |
|
@PVince81 From my part yes, although I am not sure what exactly is causing the failing build |
|
@marcus-herrmann please rebase. There were random failures in the past and they will hopefully disappear now. If not I can have a look later on. |
marcus-herrmann
force-pushed
the
feature/focus-mgmt-main-menu
branch
from
5cea244
to
af52a39
Compare
Add focus management regarding off canvas main nav, #1647
marcus-herrmann
force-pushed
the
feature/focus-mgmt-main-menu
branch
from
af52a39
to
249b1ba
Compare
|
@PVince81 Done |
|
seems the tests would pass. the two failures are due to "npm install" not going through... I've restarted the build |
Description
The content of this PR deals with keyboard focus management in the context of the off canvas navigation. It has got to major parts
Related Issue
Motivation and Context
Improve Accessibility: Keyboard navigation is not only crucial for keyboard only users, but all screen reader users
How Has This Been Tested?
Manually by hitting tab key and logging the document.activeElement in the browser's console.
Screenshots (if appropriate):
Types of changes
Checklist:
Open tasks: