Skip to content

Reimplement password toggle as reusable component#6081

Merged
aduth merged 15 commits intomainfrom
aduth-pw-toggle
Apr 6, 2022
Merged

Reimplement password toggle as reusable component#6081
aduth merged 15 commits intomainfrom
aduth-pw-toggle

Conversation

@aduth
Copy link
Copy Markdown
Contributor

@aduth aduth commented Mar 17, 2022

Why:

  • Consolidate common implementation between password and SSN toggle
  • Reduce size and scope of application JavaScript pack
  • Reduce attack surface area by avoiding injected interpolated string data (insertAdjacentHTML)
  • Fix tab order mismatched with visual placement (WCAG 2.1 G59)
  • Improve accessibility by establishing relationship between toggle and input , and announcing state changes via live region (see related resource)
    • Edit: Chose to defer state change announcements to future exploration of benefit

@aduth
Copy link
Copy Markdown
Contributor Author

aduth commented Mar 17, 2022

Not totally convinced it's a good idea to introduce decorators

Expanding on this a bit: Decorators have been around in proposal form for a very long time, and have seen some good adoption in frameworks and in TypeScript. However, it's a bit jarring that the specification is still undergoing significant change, as recently as December 2021 proposing a new syntax for decorator arguments (see also: related Babel blog post). This isn't yet supported in the TypeScript definitions, hence the use of the "legacy" syntax here. It's a bit discouraging to implement something with full expectation it'd need to be rewritten in the future.

On the other hand, I think decorators can be a good complement to custom elements, since they're a simple way to enhance class getters and methods for common behaviors. I expect this is why they're seeing good adoption in custom element frameworks (e.g. Google's Lit, Microsoft's Fast).

@aduth aduth marked this pull request as ready for review March 22, 2022 13:19
@aduth
Copy link
Copy Markdown
Contributor Author

aduth commented Mar 22, 2022

In 18ebfd4b238677396a3e75f0fd9af44197ddb9b3, I removed visible state announcements. I think it could be beneficial, though it requires further exploration outside this refactor, as well as content guidance.

@aduth aduth force-pushed the aduth-pw-toggle branch 2 times, most recently from ec55d85 to 781dbb9 Compare March 28, 2022 14:12
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

aduth added 15 commits April 5, 2022 11:28
**Why**:

- Consolidate common implementation between password and SSN toggle
- Reduce size and scope of application JavaScript pack
- Reduce attack surface area by avoiding injected interpolated string data (insertAdjacentHTML)
- Fix tab order mismatched with visual placement (WCAG 2.1 G59)
- Improve accessibility by establishing relationship between toggle and input, and announcing state changes via live region

changelog: Improvements, Accessibility, Fix tab order and add relational linking for password visibility toggle
Hello Prettier 2.6
**Why**: Because rendering view may have its own descriptor, in addition to the field's own error description. aria-describedby is an "ID reference list", meaning it can support multiple, space-separated IDs.

Ref: https://www.w3.org/TR/wai-aria-1.1/#aria-describedby
@aduth aduth force-pushed the aduth-pw-toggle branch from 781dbb9 to 7dcdbdc Compare April 6, 2022 13:03
form: f,
label: t('forms.password'),
required: true,
input_html: { aria: { describedby: 'password-description' } },
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Was doing a round of last-minute testing on this branch and noticed the describedby was being overridden by ValidatedFieldComponent. Pushed an update in 7dcdbdc to make sure that it's retained.

@aduth aduth merged commit 492369d into main Apr 6, 2022
@aduth aduth deleted the aduth-pw-toggle branch April 6, 2022 16:48
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.

2 participants