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.

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

Files & Core accessibility fixes #26548

Merged
merged 8 commits into from
Apr 20, 2021
Merged

Conversation

jancborchardt
Copy link
Member

Before After
Lighthouse score for files Accessibility: 81 Lighthouse score for files Accessibility: 94

Also tested with keyboard only and a screenreader.

The only thing remaining seems to be the aria-controls attribute of the search button. But I can’t figure out why, cause it does correspond to an element. Any idea @skjnldsv @GabeGabeT @szet0018 @RubyDo?
Lighthouse accessibility notice about aria attribute of the search button


Here some demo of before/after of the focus feedback for file list actions, since the other ones are mostly screenreader label fixes:

Focus.effect.before.mp4
Focus.effect.afterwards.mp4

@jancborchardt
Copy link
Member Author

/backport to stable21

Copy link

@MarcoZehe MarcoZehe left a comment

Choose a reason for hiding this comment

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

The file core/js/dist/unified-search.js is impossible to review, because its diff has over 42,554 changed lines. Is there something wrong with line endings maybe so that each line apears as a changed one? Please update the patch with a more readable diff.

@ChristophWurst ChristophWurst removed their request for review April 14, 2021 06:25
@skjnldsv
Copy link
Member

The file core/js/dist/unified-search.js is impossible to review, because its diff has over 42,554 changed lines. Is there something wrong with line endings maybe so that each line apears as a changed one? Please update the patch with a more readable diff.

@MarcoZehe this one is the compiled bundle.
You need to review the source files: core/src/views/UnifiedSearch.vue

@MarcoZehe
Copy link

@skjnldsv Ah, so these patches include two compiled sources, one of them minified? Geez... Yeah that vue source file didn't have anything suspicious, but the questions and requests from my other comments are still valid.

@jancborchardt jancborchardt force-pushed the design/files-accessibility branch from 11cedce to 1708014 Compare April 15, 2021 10:51
@jancborchardt
Copy link
Member Author

Fixed the issues with the "Profile picture" section. But for @MarcoZehe’s other remark, a Javascript expert would have to take over.
@skjnldsv do you want to fix the remark in here, or would you prefer we merge this and do it in a follow-up? The div should ideally become a button, and if possible we can use the ActionMenu there since our components are accessible?

And thanks a ton @MarcoZehe for the review!

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

🐘

@jancborchardt
Copy link
Member Author

@MarcoZehe I fixed your feedback about the label, the issue with the "divs which should be buttons" I would open as a separate issue and handle it separately, as for that I need a Javascripter to take over and it ideally shouldn’t block the merging of these fixes. Would that be ok?

@MarcoZehe
Copy link

Yes, that would be fine.

@jancborchardt
Copy link
Member Author

Ah and for the record: The element is a div, but I tested with keyboard and screen reader and it does read "Change privacy level of profile picture" and the menu can be opened using space (and closed with Escape).

So mainly it is about just changing it to a button – I’m already working on preparing the follow-up pull request.

@jancborchardt
Copy link
Member Author

The failures seem unrelated. Can you confirm @rullzer @skjnldsv?

@jancborchardt
Copy link
Member Author

Actually almost fixed it already, but needed to use <a> instead of <button> – commit coming soon.

@jancborchardt
Copy link
Member Author

Ok, resolved the basics of the menu, but this will be properly fixed once we move the rest of settings to the Vue components as well which are accessible by default. :)

@jancborchardt jancborchardt self-assigned this Apr 16, 2021
@jancborchardt jancborchardt force-pushed the design/files-accessibility branch from 766e157 to d3ad1c0 Compare April 16, 2021 13:19
Signed-off-by: Jan C. Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan C. Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan C. Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan C. Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan C. Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan C. Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan C. Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan C. Borchardt <hey@jancborchardt.net>
@jancborchardt jancborchardt force-pushed the design/files-accessibility branch from cb4bd1d to 738ac61 Compare April 19, 2021 19:09
@jancborchardt
Copy link
Member Author

Now rebased several times, checks still failing. Seem unrelated, could you confirm @rullzer @ma12-co?

@jancborchardt
Copy link
Member Author

Failures unrelated: #26314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug design Design, UI, UX, etc. feature: accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants