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

Add accessibility label to show password checkbox #16642

Merged
merged 1 commit into from
Dec 12, 2019

Conversation

nesb0t
Copy link
Contributor

@nesb0t nesb0t commented Aug 2, 2019

Fixes #15965.

Added aria-label to show password checkbox. Tested and verified with NVDA.

core/templates/installation.php Outdated Show resolved Hide resolved
@jancborchardt
Copy link
Member

Thank you @nesb0t for the contribution! :)

Based on the comments from @skjnldsv and @MarcoZehe, could I ask you to change the following:

  • Instead of aria-label on the checkbox, the text can be put into the label for="show" block directly after.
  • The label for="show" then also needs class="hidden-visually" just like the checkbox so it’s only visible for screenreaders.
  • It should be translatable in the way @skjnldsv suggested.

Let us know if you need any help. ♥

@MorrisJobke MorrisJobke mentioned this pull request Aug 8, 2019
28 tasks
This was referenced Aug 18, 2019
@rullzer rullzer mentioned this pull request Aug 29, 2019
16 tasks
@rullzer rullzer modified the milestones: Nextcloud 17, Nextcloud 18 Sep 5, 2019
@rullzer
Copy link
Member

rullzer commented Sep 5, 2019

Master is no 18 development. If this needs to go into 17 please follow the normal backport procedures after merging.

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Dec 10, 2019
@kesselb
Copy link
Contributor

kesselb commented Dec 10, 2019

Thank you @nesb0t for the contribution! :)

Based on the comments from @skjnldsv and @MarcoZehe, could I ask you to change the following:

* Instead of `aria-label` on the checkbox, the text can be put into the `label for="show"` block directly after.

* The `label for="show"` then also needs `class="hidden-visually"` just like the checkbox so it’s only visible for screenreaders.

* It should be translatable in the way @skjnldsv suggested.

Let us know if you need any help. ♥

@skjnldsv is this done? ;)

@skjnldsv
Copy link
Member

God dammit

@kesselb
Copy link
Contributor

kesselb commented Dec 10, 2019

Sorry 🙈

@skjnldsv
Copy link
Member

Sorry

All good ;)
I missed the other comments! It's indeed the thing to do!

@kesselb
Copy link
Contributor

kesselb commented Dec 10, 2019

The label for="show" then also needs class="hidden-visually" just like the checkbox so it’s only visible for screenreaders.

Don't forget ;)

@skjnldsv
Copy link
Member

god dammit

@rullzer rullzer mentioned this pull request Dec 11, 2019
43 tasks
@skjnldsv skjnldsv merged commit 5e0f820 into nextcloud:master Dec 12, 2019
@welcome
Copy link

welcome bot commented Dec 12, 2019

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!

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 bug feature: accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Accessibility] "Show password" checkbox doesn't provide label or accessible name
8 participants