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

fix: a11y issues in files page #1512

Merged
merged 10 commits into from
Jun 15, 2020
Merged

fix: a11y issues in files page #1512

merged 10 commits into from
Jun 15, 2020

Conversation

rafaelramalho19
Copy link
Contributor

fixes #1498

Impacted areas:

  • Changed clickable items that were not-buttons to buttons.
  • Added eslint plugin to prevent further a11y issues
  • Fixed keyboard navigation all across the webui
  • Added missing labels for important items

Any questions or concerns please raise them 😄

Copy link
Contributor

@jessicaschilling jessicaschilling left a comment

Choose a reason for hiding this comment

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

I only looked over the text you added (plus animations you shared) but those are great. Let me know if you want to me to dig deeper, but otherwise LGTM aside from that one line of conflict 😊

@lidel
Copy link
Member

lidel commented Jun 2, 2020

I'll look at E2E tests

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you so much @rafaelramalho19, this is very important work! 🙌
Really happy about build safeguards that will do a11y checks automatically going forward.

Some notes / asks before we merge this:

  • I fixed E2E tests, should be green now :)
  • nit: when there are many files, and I navigate down the list with keyboard arrows, I get bit jittery animation (scrolling down a bit, and the jumping back to the top. Then I get to the the middle of the list, then it disappears – demo)
    • @rafaelramalho19 is this something we can easily fix? if not, I don't think it should block this PR
  • needs to be rebased on top of master to ensure the updated LanguageModal also gets fixes

@rafaelramalho19
Copy link
Contributor Author

@lidel thank you so much for checking that up and fixing it 😄

Related to your demo, it looks like you're using React-virtualized table arrow navigation, which I have no control over (you're using arrow down & up instead of tab).

There's some fixes related with improving tab navigation on the webui as a whole that I want to do when I have the time 😓

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@rafaelramalho19 I understand. If its a rabbit hole, the arrow navigation jitter should not block important work done in this PR.

Are you able to resolve conflict in FilesPage?
After that, ping me if you have any new issues with E2E.

@rafaelramalho19
Copy link
Contributor Author

The arrow navigation is being handled on a separate branch, so don't worry about it 😄

@lidel
Copy link
Member

lidel commented Jun 8, 2020

@rafaelramalho19 is this ready for final review, or do you want to add anything?

@rafaelramalho19
Copy link
Contributor Author

@rafaelramalho19 is this ready for final review, or do you want to add anything?

It's good for final review 👌

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Made another pass, glooks good, but found small visual regressions that would be good to address before merging:

(1) Button labels

... are now aligned to left:

Status on new install:

1-buttons

Settings:

1-buttons--Screenshot_2020-06-09 Settings - IPFS

Connection screen:

1-buttons-connection screen Screenshot_2020-06-09 Welcome to IPFS

etc..

Is this on purpose? Before this PR, we centered them:

download-Screenshot_2020-06-09 Status - IPFS

(2) Delete option is now too prominent

a11y changes highlight the very first item, which happens to be "delete" – could we change the order?

2-menu-order-2020-06-09--16-05-18

Something like (purely subjective order, the important piece is to have "share link" at the top)

  • Share link
  • Copy CID
  • Inspect
  • Pin
  • Rename
  • Delete

@jessicaschilling
Copy link
Contributor

+1 on the menu order. Thank you so much for the careful review, @lidel.

@rafaelramalho19
Copy link
Contributor Author

Nice catch @lidel , will fix it now 😄

@rafaelramalho19
Copy link
Contributor Author

@lidel @jessicaschilling you like this order? https://i.imgur.com/AZMDSWT.png

Fixed uncentered buttons across the app, it was due to me forgetting to check all the buttons after removing the text-align: center property 😅

@jessicaschilling
Copy link
Contributor

Should we swap download and pin?

@rafaelramalho19
Copy link
Contributor Author

Should we swap download and pin?

Here's how it looks: https://i.imgur.com/gpr4DEB.png

@jessicaschilling
Copy link
Contributor

I like that if @lidel is good with it. Thanks!

@lidel
Copy link
Member

lidel commented Jun 9, 2020

Note to self: I suspect CI will be fixed by #1521 – rebasing this PR should help.

Update: rebased, but we may need #1523 to solve all remaining flakiness (seems circleci got slower in past weeks..)

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

I resolved CI issues (see #1521 and #1523) and rebased this PR to take advantage of those fixes.

CI is green, and LGTM.


One more thing: when I select a single file, the action bar that slides in from the bottom gets this dotted highlight to indicate a11y context:

2020-06-11--02-34-53

This is pretty common UI interaction and I worry some people not familiar with a11y may report this as a bug. At the same time not sure how to avoid this. Don't want this to block the PR, just mentioning it here in case there in case you already have thoughts on how to deal with this (but ok to fix in separate PR).

@rafaelramalho19 when you are back on Monday, feel free to merge.

@rafaelramalho19 rafaelramalho19 merged commit 42479e7 into master Jun 15, 2020
@rafaelramalho19 rafaelramalho19 deleted the fix/files-a11y branch June 15, 2020 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Files screen content only semi-functional with a screen reader
3 participants