-
Notifications
You must be signed in to change notification settings - Fork 159
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.
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
Add focus management for uploads #2542
Conversation
@@ -1,9 +1,9 @@ | |||
<template> | |||
<div id="files" class="uk-flex uk-flex-column"> | |||
<files-app-bar /> | |||
<upload-progress v-show="inProgress.length" class="uk-padding-small uk-background-muted" /> | |||
<files-app-bar @upload-start="onUploadStart" @upload-end="onUploadEnd" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, it starts to feel like we need some event bus to globally detect such events like uploads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event bus established here: https://github.com/owncloud/phoenix/pull/2542/files#diff-c900a0e34358d44a55381e5e63de50caR70
In use for example here: https://github.com/owncloud/phoenix/pull/2542/files#diff-eec759a01d0f06a12081c5e8285c0d85R68
ad49132
to
ebd3f61
Compare
src/phoenix.js
Outdated
@@ -65,6 +65,10 @@ Vue.use(VueMeta, { | |||
Vue.component('drag', Drag) | |||
Vue.component('drop', Drop) | |||
|
|||
// --- Event Bus ---- | |||
|
|||
Vue.prototype.$bus = new Vue() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LukasHirt can you confirm that this approach for a bus makes sense ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I'd rather see event bus avoided. @marcus-herrmann is it necessary here? Would it be much trouble covering this with state? If needed to stay, pls make the bus a constant and import it only where it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Covering it with state was my first approach, I talked with @PVince81 about that and we agreed that this approach was overengeenering/not what the store was meant to be. Keep in mind that there would be more occurances for this, to send for example focus across the interface/components that are not close to each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LukasHirt Here's the first approach mentioned above: #2624
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LukasHirt are we ok with the bus then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now thinking, I remember seeing that it's possible to access the root component using this.$root
, would it be possible to use that component as the bus instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to work as well. PR updated
ebd3f61
to
25c584c
Compare
25c584c
to
08648f1
Compare
Regarding: progressbar, to listwrapper on complete
08648f1
to
a967fbc
Compare
Rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Regarding: progressbar, to listwrapper on complete
Description
Improves upload user flow for screen reader users by making them programmatically aware of
Related Issue
Motivation and Context
While visual users can comprehend that an (larger) upload is running and get visual clues about its end, screen readers stay silent during the progress. This PR tackles this issue by adding two steps:
OcProgress
is improved with aria states/information about the upload so screen reader can pick its state up. In order to do so the progressbar has to have focus.How Has This Been Tested?
document.addEventListener('focusin', () => {console.log(document.activeElement);});
Types of changes
Checklist:
Open tasks: