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

Improve accessibility with more visible focus indication for non vue apps #31584

Merged
merged 1 commit into from
May 16, 2022

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Mar 16, 2022

  • Add a focus effect on each header entry
    image

  • Add a focus effect on app navigation
    image

  • Add a focus effect on menus
    image

  • Add a focus effect on file app
    image

  • Add a focus effect on non vue input elements
    image

  • Show big focus outline when using focus-visible instead of focus

  • Add polyfill for focus-visible since it's only very recently available on WebKit

  • Add a few small screen readers improvements in the file app (checkbox)

I used https://www.sarasoueidan.com/blog/focus-indicators/ to make
sure the focus effect was WCAG 2 and WCAG 2.1 compliant.

Signed-off-by: Carl Schwan carl@carlschwan.eu

@CarlSchwan CarlSchwan added this to the Nextcloud 24 milestone Mar 16, 2022
@CarlSchwan CarlSchwan self-assigned this Mar 16, 2022
@CarlSchwan CarlSchwan changed the title Improve accessibility of header Improve accessibility with more visible focus indication Mar 16, 2022
core/css/apps.scss Outdated Show resolved Hide resolved
core/css/header.scss Outdated Show resolved Hide resolved
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
@CarlSchwan CarlSchwan requested a review from PVince81 April 4, 2022 22:14
@blizzz blizzz mentioned this pull request Apr 7, 2022
core/css/apps.scss Outdated Show resolved Hide resolved
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.

See comments

@blizzz blizzz mentioned this pull request Apr 13, 2022
@blizzz blizzz removed this from the Nextcloud 24 milestone Apr 21, 2022
@CarlSchwan
Copy link
Member Author

@CarlSchwan any update here? Would be good to get this in early in the 25 cycle then. :)

I just rebased everything and fixed/commented on the review from @skjnldsv :)

@PVince81
Copy link
Member

PVince81 commented May 9, 2022

some visual issues:

  • bug: missing padding on left of the box, it's there for the other columns

image

  • bug: border a bit cropped and also not fully centered:

image

apart from that it looks rather good!

I found some elements that are not focussable yet like the expanding arrow in the left sidebar, likely something for a separate PR as it's not directly related

@PVince81
Copy link
Member

PVince81 commented May 9, 2022

@CarlSchwan regarding the left navigation, I suspect that we'll want to add this focus border also in nextcloud-vue so we need to make sure there is no conflict there when it happens.

can you check if the issue you fixed is specific to the files app navigation panel or affects other apps ?

@JuliaKirschenheuter
Copy link
Contributor

Thank you for working on this issue and taking over accessibility topics! And sorry for late response!
I'm just trying to understand, to which issue (ticket) is this PR related to? There are much changes and i don't really know what should i actually check.

I think there are a lot of stuff which are related to all components (nc-vue). Sorry if i have missed something, but why this changes are not in nc-vue, but here?

Have you fixed mostly visual stuff in this ticket? (or have you also adopted it to screen reader)? *not all buttons have (aria-label)
Could you explain, what this focus should cause (i haven't get it, sorry)?
image

"Shares" on the left sidebar have 2 stops.

Contacts have no focus
image

Some<li> on a left sidebar are focusible with other design
image
same here in dropdown
image

"Toggle" have 2 stops
image

Focus of this dropdown menu will not go away with Esc (will not disappear)

image
image
image

And i think some of this visual issues would be better done in nc-vue? (@skjnldsv or i'm wrong in this point?)

image
image
image
image

@CarlSchwan sorry if i checked "too much". It is great, that you work on it! I think it would be better to split this "elephant" improvements into smaller tickets - would be better to work on and to review! If you want - i could take over of a part of this tickets, you should't be alone =))

@CarlSchwan
Copy link
Member Author

Thank you for working on this issue and taking over accessibility topics! And sorry for late response! I'm just trying to understand, to which issue (ticket) is this PR related to? There are much changes and i don't really know what should i actually check.

I think there are a lot of stuff which are related to all components (nc-vue). Sorry if i have missed something, but why this changes are not in nc-vue, but here?

These changes are mostly for the parts that are not yet ported to vue. E.g. the files app and the navbar.

Have you fixed mostly visual stuff in this ticket? (or have you also adopted it to screen reader)? *not all buttons have (aria-label) Could you explain, what this focus should cause (i haven't get it, sorry)? image

It's mostly about keyboard navigation where having a focus effect visible is important. Screen reader navigation is a separate task unfortunately :(

"Shares" on the left sidebar have 2 stops.

Looks correct to me

image

image

Contacts have no focus image

Let's move that to another PR, it's fairly separated from the rest of this PR in the cod and like you already this PR is already big.

Some<li> on a left sidebar are focusible with other design image same here in dropdown image

fixed

"Toggle" have 2 stops image

fixed

Focus of this dropdown menu will not go away with Esc (will not disappear)

image image image

preexisting issue :( Probably for the contact view it would be easier to port it to vue than to fix it

And i think some of this visual issues would be better done in nc-vue? (@skjnldsv or i'm wrong in this point?)

image

I fixed the patch to make even more sure we don't modify vue apps, as like you said those should be modified in nc-vue

image
image
image

This is not a vue app so we can't use nc-vue here :(

@CarlSchwan sorry if i checked "too much". It is great, that you work on it! I think it would be better to split this "elephant" improvements into smaller tickets - would be better to work on and to review! If you want - i could take over of a part of this tickets, you should't be alone =))

One stuff we could separate is the contact search who like you pointed out is not really working (both visual focus and keyboard navigation)

@CarlSchwan CarlSchwan changed the title Improve accessibility with more visible focus indication Improve accessibility with more visible focus indication for non vue apps May 9, 2022
@JuliaKirschenheuter
Copy link
Contributor

JuliaKirschenheuter commented May 10, 2022

Thanks for your reply / fix!

Could you explain, what this focus should cause (i haven't get it, sorry)? image

It's mostly about keyboard navigation where having a focus effect visible is important. Screen reader navigation is a separate task unfortunately :(

Ok, sure. I meant here, that a user cannot interact with this focus area. I would suggest to stop on the next interaction element: "Shared".

"Shares" on the left sidebar have 2 stops.

Looks correct to me

image

image

Ok, now for me too. I think a "shares" element should be somehow reworked in another PR, that it would have only 1 stop on interaction element (without whole "Shares", it makes not much sense).

"Toggle" have 2 stops image

fixed

*still have 2 stops)

Lets move other stuff to separate tickets and do them one-by-one =)

Would you make only one commit to this PR?

@AndyScherzinger
Copy link
Member

Resolves #31239

@AndyScherzinger AndyScherzinger linked an issue May 12, 2022 that may be closed by this pull request
3 tasks
@CarlSchwan CarlSchwan force-pushed the fix/accessibility branch 7 times, most recently from 825bdd4 to 7b33766 Compare May 12, 2022 19:01
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 let's move forward

@CarlSchwan
Copy link
Member Author

Need a rebase :) Going to do it today and then merge it

…apps

- Add visible-focus effect on each header entry
- Show focus outline when using focus-visible (keyboard navigation)
- Add polyfy for focus-visible since it's only very recently available
  on webkit
- Change text for link to home button to describe the destination and
  not the current page
- Improve focus effect in app sidebar navigation

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan force-pushed the fix/accessibility branch from 7b33766 to c149951 Compare May 16, 2022 11:22
@skjnldsv skjnldsv merged commit ab0548e into master May 16, 2022
@skjnldsv skjnldsv deleted the fix/accessibility branch May 16, 2022 20:24
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BITV/A11Y]: Header accessibility
8 participants