Skip to content

LG-11278: Group authentication app setup fields with fieldset#9484

Merged
aduth merged 5 commits intomainfrom
aduth-lg-11278-totp-fieldset
Nov 1, 2023
Merged

LG-11278: Group authentication app setup fields with fieldset#9484
aduth merged 5 commits intomainfrom
aduth-lg-11278-totp-fieldset

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Oct 31, 2023

🎫 Ticket

LG-11278

🛠 Summary of changes

Adds a fieldset grouping to the TOTP setup screen, to logically group related fields.

Related resource: https://www.w3.org/TR/WCAG20-TECHS/H71.html

This is a non-visual change, as it labels the fieldset using aria-labeledby with an existing text on the page "Set up an authentication app to sign in using temporary security codes.".

Recommended to review with whitespace changes hidden due to most view changes being the addition of a wrapping element that increases the tab level of most content.

📜 Testing Plan

You can inspect the grouping element with your browser DevTool's "Accessibility" inspector to confirm the named grouping:

image

You can also confirm with a screen reader that entering the form fields announces the group

image

👀 Screenshots

There are no visual changes, but here is the screen for reference:

Screenshot 2023-10-31 at 10 33 21 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could / should probably move this elsewhere and add tests for it? Unclear if that makes sense and where would be best for a utility only used in specs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine here. How would this differ from the rest of the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My main concern is if we'd want to have a convention of one-class-per-file, and files containing a class only containing that class. It'd also be nice to have some test coverage for it, but we don't typically enforce test coverage for test helpers.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could break it out as accessibility_be_logically_grouped.rb or something like that.
I wonder if the other classes in that test helper should be split off also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll keep this as-is for now, but I do like the idea of maybe creating a new folder within helpers for accessibility helpers, which would be more scalable and let us do one-class/matcher-per-file.

@aduth aduth requested a review from a team October 31, 2023 14:36
aduth added 5 commits October 31, 2023 13:16
changelog: User-Facing Improvements, Accessibility, Improve grouping semantics for authentication app setup form
@aduth aduth force-pushed the aduth-lg-11278-totp-fieldset branch from 4f03939 to 807025d Compare October 31, 2023 17:16
Copy link
Contributor

@kevinsmaster5 kevinsmaster5 left a comment

Choose a reason for hiding this comment

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

solution looks good and validated locally.

@aduth aduth merged commit 6a4d5db into main Nov 1, 2023
@aduth aduth deleted the aduth-lg-11278-totp-fieldset branch November 1, 2023 12:19
@amirbey amirbey mentioned this pull request Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants