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 password components for Vue 2 and Vue 3 #21

Merged
merged 29 commits into from
May 2, 2022

Conversation

meduzen-immoweb
Copy link
Collaborator

@meduzen-immoweb meduzen-immoweb commented Jan 19, 2022

The Vue 2 component is in packages/vue-2-password.
The Vue 3+ component is in packages/vue-password.

None of them are currently published. But we can surely publish a 1.0.0-rc.1 to test them.

The component

It is the same as in this PR, with the following changes:

Aside from the component

Each folder has:

  • a README.md file with instructions for installation, usage and various stuff (license, development instructions…)
  • a CHANGELOG.md
  • a demo app (src/App.vue) that can be run with npm run serve
  • tests (not done yet)

(Side note: Vue 2 component has a Password-ts.vue file in addition to Password.vue. It’s useless, but I kept it for history.)

Next steps

  • Review the PR
  • Publish packages
  • Add installation and usage instructions to the Serenity docs.
  • Celebrate
  • Add tests (this one can be done before/after any step)

@meduzen-immoweb meduzen-immoweb added the enhancement New feature or request label Jan 19, 2022
@EmmaPepin
Copy link
Collaborator

@meduzen We can improve a little bit the accessibility by adding :

  • aria-live polite to the span to signify to user the status : password shown|password hidden.
  • aria-pressed to the button (true|false, depending the click)
<div class="password">
<span class="sr-only" aria-live="polite">Password hidden</span>
<input type="password" autocomplete="current-password" class="input--text" id="ZRh1R9" name="login-password" aria-labelledby="label-ZRh1R9 " aria-invalid="">
<button type="button" class="password__btn button button--transparent button--text" id="show-login-password-ZRh1R9" aria-label="Show password" title="Show password" aria-pressed="false">
<svg width="24" height="24" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" aria-hidden="true" fill="currentColor"><path fill-rule="evenodd" clip-rule="evenodd" d="M12 4.5C7 4.5 2.73 7.61 1 12c1.73 4.39 6 7.5 11 7.5s9.27-3.11 11-7.5c-1.73-4.39-6-7.5-11-7.5zM12 17c-2.76 0-5-2.24-5-5s2.24-5 5-5 5 2.24 5 5-2.24 5-5 5zm-3-5c0-1.66 1.34-3 3-3s3 1.34 3 3-1.34 3-3 3-3-1.34-3-3z"></path></svg>
</button>
</div>

@meduzen-immoweb
Copy link
Collaborator Author

@meduzen We can improve a little bit the accessibility by adding :

  • aria-live polite to the span to signify to user the status : password shown|password hidden.
  • aria-pressed to the button (true|false, depending the click)

Indeed, the user wasn’t warned of anything when using the button. Nice catch! Actually your suggestion is the only one working everywhere (using Voice Over).

So, when the button is activated, 3 things must change:

  1. <input type="password"> becomes <input type="text">
  2. <button aria-pressed="false"> becomes <button aria-pressed="true">
  3. Separate <span class="sr-only" aria-live="polite">Password is hidden</span> becomes <span class="sr-only" aria-live="polite">Password is shown</span>

Changes I’ll do

  • remove title on <button> (see below);
  • add a separate <span> to announce the visibility state, and 2 component props to express the visibility state (passwordHiddenText and passwordShownText);
  • remove the hidePasswordLabel prop, now useless.

At the beginning I wanted to keep the title attribute for sighted mouse users, and make it change so the right action is in the button title, but it’s announced by Safari, even when the <button> has an aria-label 🙃 .

Results

Results using Voice Over (macOS Big Sur 11.6.4)

Firefox (97.0.1) doesn’t care about the separated aria-live="polite" and only announces the state change of aria-pressed:

Selected, show password, toggle button.

Safari (15.3) does the reverse and only announces the state separated aria-live="polite":

The password is visible.

Chromium (Edge 100.0.1167.0) browsers announce the button state change then the separated aria-live="polite" 👍 :

Selected, show password, toggle button. The password is visible.

A the end, the <button> label needs to remain the same. So the only text content that needs to change on activation is the separate aria-polite announcing the state of the password visibility, otherwise it’s confusing.

Here is why it’s confusing.

Add the beginning, I tried combinations of aria-pressed (on the button) and aria-live="polite" on the button label (instead of a separate element 🙃 ). Here’s what I got:

aria-pressed on button aria-live="polite" on label password visible (Voice Over) password not visible (Voice Over) observation
no no - - announcement is missing
no yes Show password” Hide password” See note (1): label is okay, but the user may not know the focus is a button
yes no “Show password, toggle button” Sélectionné, Show password, toggle button” The state is clear, but the user needs to know what a “toggle button” is.
yes yes Show password, toggle button” Sélectionné, Hide password, toggle button” When it shows the password, it says “hide password” is selected, which is the reverse logic (double negation cause by changing label + aria-pressed)

(1) It actually says “Show password, bouton” when focusing the button, but it doesn’t repeat « bouton » when we’re already on it and we activate it. When using aria-pressed, it always repeat “toggle button” (« bouton de basculement » in French).

In the current state of the PR, both the aria-label and the inner button label ({{ showPasswordLabel }}) are changing when hitting the button, which sometimes cause Voice Over to says the same sentence twice. Without aria-live="polite", it even says it twice at the same time with a micro-delay between both. With aria-live, it waits the end of the aria-label announcement to read the <button> content.

Resources

  1. Playing with states: name changes (Sarah Mhigley)

As a final note, never change both the name and aria-pressed state in tandem. That way, confusion lies. Just imagine trying to parse the meaning of "play button, on" vs. "pause button, off".

  1. aria-pressed (MDN)

Do not change the contents of the label on a toggle when the state changes. If a button label says "pause", do not change it to "play" when pressed. In this example, when the pressed state is true, the label remains "Pause" so a screen reader would say something like "Pause toggle button pressed".

If you want the label to toggle between "Paused" and "Play", don't use aria-pressed.

The first rule of ARIA use is "if you can use a native feature with the semantics and behavior you require already built in, instead of repurposing an element and adding an ARIA role, state or property to make it accessible, then do so." If we employ native HTML semantics with <button>, we can toggle the label instead of toggling the pressed state, removing the need for the aria-pressed attribute.

  1. Toggle Buttons - Changing labels (Heydon Pickering).

As a rule of thumb, you should never change pressed state and label together. If the label changes, the button has already changed state in a sense, just not via explicit WAI-ARIA state management.

The problem with this method is that the label change is not announced as it happens. That is, when you click the play button, feedback equivalent to “pressed” is absent. Instead, you have to unfocus and refocus the button manually to hear that it has changed. Not an issue for sighted users, but less ergonomic for blind screen reader users.

That match the note (1) about the new state not being announced without aria-pressed, when the button is activated.

  1. Tests for <button> inner change

Only changing the inner text of a button mostly fails everywhere.

  1. Under-Engineered Toggles Too (Adrian Roselli)

  2. Show/Hide password accessibility and password hints tutorial (Nicolas Steenhout)

  3. Simple things are complicated: making a show password option (Andy Sellick), and the resulting experimental password component

They went for the same approach, except they prefer to change the text label instead of using aria-pressed. There’s a trade-off: without aria-pressed, you have no hint that you are still focusing a <button>, but then by changing the content of the button, it reflects its next action… It makes sense in their usage because they use plain text for the action while we use an icon.

image image

Also, various test cases (mostly from previous links):

@meduzen-immoweb
Copy link
Collaborator Author

Re-reviewable. @EmmaPepin

@meduzen-immoweb
Copy link
Collaborator Author

Self-note: import from iwb-serenity-password-vue-2 may not work in a non-TS project (likely a Webpack loader configuration issue). Workaround awaiting a fix is to import from /node_modules/iwb-serenity-password-vue-2/src/components/Password.vue.

Copy link
Collaborator

@EmmaPepin EmmaPepin left a comment

Choose a reason for hiding this comment

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

Remark about error management : with the empty data check, it is not possible to check if the field is required.
For example
image
Solution : remove completely the empty check or add another boolean prop, as 'required' or 'emptyAllowed', to make possible the required check.

@meduzen-immoweb
Copy link
Collaborator Author

with the empty data check, it is not possible to check if the field is required. […] Solution : remove completely the empty check or add another boolean prop, as 'required' or 'emptyAllowed', to make possible the required check.

  1. The “required” check is currently only done server side, so I propose to remove the empty check. If we want to indicate the user that the password is required (which is always the case), then the parent component/form should probably be the one dealing with this.

  2. Alternatively, there’s the required HTML attribute, but maybe there’s a reason why we don’t use it (accessibility? letting the server be the only “checker” for data?). Do you have any information about that?

  3. Another thing: right now it’s possible to initialize a default value for the password (like in the demo app for the one with an error), and then the password component initialize its empty prop by checking the value, but it doesn’t make sense to have a default value for a password. So I propose to remove this possibility.

What do you think?

@EmmaPepin
Copy link
Collaborator

@meduzen-immoweb

  1. 👍
  2. About required, i think that a password input will be always required in form (login, change password, user's creation).
    I don't see a case where it won't. So, we can add required and aria-required. If you see a case where it will be, maybe we can use a prop required to manage these attribute as an option.
  3. 👍

Another note, we have to improve the management of the aria-invalid attribute.
For example, in Back Office, when password is saved in browser, this attribute is set to is true, although no error detected.
image

@meduzen-immoweb
Copy link
Collaborator Author

I made some updates. There’s rather a lot, so I also published version v0.3.0 so we can test the changes. If something isn’t suitable, let’s discuss and make choice for a potential 0.4.

  • I went for aria-required because it pairs well with server validation, and required unfortunately causes accessibility issues.
  • aria-invalid fixed. Can you check it works properly with password autocompletion? On macOS + Chromium, it’s now fine.
  • A new autocomplete prop.
  • Fix input border-color when the field has an error and the visibility button is focused, as per discussion with the designer.
  • Documentation of accessibility choices.

Everything is normally in the changelog and the docs.

@meduzen-immoweb
Copy link
Collaborator Author

I’ve noticed using v-model="password" is useless when the <form> is submittable without JavaScript. But the component currently requires it. I think we can safely make it optional.

@meduzen-immoweb
Copy link
Collaborator Author

Also: when password is long, doesn’t look nice:

image

@meduzen-immoweb
Copy link
Collaborator Author

Merging. 🎉 We will open issue for remaining comments.

@meduzen-immoweb
Copy link
Collaborator Author

Merging. 🎉 We will open issue for remaining comments.

Done: #30 #31.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants