Fix the lack of tab/focus for password indicators, and add aria-live to it and maxLength.#256
Conversation
davinotdavid
left a comment
davinotdavid
left a comment
There was a problem hiding this comment.
Nice! The a11y related changed looks good to me but some questions on the boolean handling
| <eye-icon class="icon" alt="" v-if="passwordIsVisible === true" /> | ||
| <eye-off-icon class="icon" alt="" v-else-if="passwordIsVisible === false" /> |
There was a problem hiding this comment.
We could shorten this to
| <eye-icon class="icon" alt="" v-if="passwordIsVisible === true" /> | |
| <eye-off-icon class="icon" alt="" v-else-if="passwordIsVisible === false" /> | |
| <eye-icon class="icon" alt="" v-if="passwordIsVisible" /> | |
| <eye-off-icon class="icon" alt="" v-else /> |
| :title="passwordIndicatorText" | ||
| @keyup.space="togglePasswordVisibility" | ||
| @click="togglePasswordVisibility" | ||
| v-if="passwordIsVisible !== null" |
There was a problem hiding this comment.
Similarly, I believe this would be enough instead
| v-if="passwordIsVisible !== null" | |
| v-if="isPasswordField" |
| // The current inputType, this will swap between text and password. | ||
| const inputType = ref(props.type); | ||
| // false: hide password, true: show password | ||
| const passwordIsVisible = ref(isPasswordField ? false : null); |
There was a problem hiding this comment.
Shouldn't this be
| const passwordIsVisible = ref(!isPasswordField); |
instead?
| if (passwordIsVisible.value === true) { | ||
| passwordIndicatorText.value = hidePasswordText; | ||
| } else if (passwordIsVisible.value === false) { | ||
| passwordIndicatorText.value = showPasswordText; | ||
| } |
There was a problem hiding this comment.
If we eliminate the true / false / null to use isPasswordField and passwordIsVisible we could do:
| if (passwordIsVisible.value === true) { | |
| passwordIndicatorText.value = hidePasswordText; | |
| } else if (passwordIsVisible.value === false) { | |
| passwordIndicatorText.value = showPasswordText; | |
| } | |
| passwordIndicatorText.value = passwordIsVisible.value ? hidePasswordText : showPasswordText |
|
We should punt that to a different ticket as the current method of using true, false and null as separate values is a separate issue and requires additional testing here. |
|
Created #257 |
devmount
left a comment
devmount
left a comment
There was a problem hiding this comment.
Nice, thanks for this! I added the missing German bits from this PR and sneaked in one that was missing for a longer time apparently 😇
|
Also: This PR has a nice round PR number of 256 😍 |
3 participants
Fixes #250
Will need some german l10n from @devmount here. 😅
This makes password indicator tabbable which fixes the a11y issue. For a better time I added aria-polite and adjusted the v-ifs to avoid losing element focus when the indicator changes.
I also added a custom aria label for maxLength which works quite well.