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

2913: Update domain request screenreader outputs [ES] #3193

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

erinysong
Copy link
Contributor

@erinysong erinysong commented Dec 5, 2024

Ticket

Resolves #2913

Changes

Context for reviewers

Setup

With org model feature turned off, go to a domain request form and verify that all screenreader outputs match the subissues.

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

  • Met the acceptance criteria, or will meet them in a subsequent PR
  • Created/modified automated tests - NA
  • Update documentation in READMEs and/or onboarding guide - NA

Ensured code standards are met (Original Developer)

  • If any updated dependencies on Pipfile, also update dependencies in requirements.txt. - NA
  • Interactions with external systems are wrapped in try/except
  • Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

  • Tag @dotgov-designers in this PR's Reviewers for design review. If code is not user-facing, delete design reviewer checklist
  • Verify new pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
  • Checked keyboard navigability
  • Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

  • Pulled this branch locally and tested it
  • Verified code meets all checks above. Address any checks that are not satisfied
  • Reviewed this code and left comments. Indicate if comments must be addressed before code is merged
  • Checked that all code is adequately covered by tests
  • Verify migrations are valid and do not conflict with existing migrations

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.

  • New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
  • Checked keyboard navigability
  • Meets all designs and user flows provided by design/product
  • Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
  • (Rarely needed) Tested as both an analyst and applicant user

As a designer reviewer, I have

Verified that the changes match the design intention

  • Checked that the design translated visually
  • Checked behavior. Comment any found issues or broken flows.
  • Checked different states (empty, one, some, error)
  • Checked for landmarks, page heading structure, and links

Validated user-facing changes as a designer

  • Checked keyboard navigability
  • Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
  • Tested with multiple browsers (check off which ones were used)
    • Chrome
    • Microsoft Edge
    • FireFox
    • Safari
  • (Rarely needed) Tested as both an analyst and applicant user

References

Screenshots

@erinysong erinysong requested a review from abroddrick as a code owner December 5, 2024 23:56
@erinysong erinysong changed the title 2913: Update domain request screenreader outputs 2913: Update domain request screenreader outputs [ES] Dec 5, 2024
@erinysong erinysong marked this pull request as draft December 5, 2024 23:59
@@ -27,6 +27,10 @@ h2 {
.usa-form,
.usa-form fieldset {
font-size: 1rem;
legend em {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that our field headings and subheadings are included as legend items in the screenreader fix, we're setting font-size to 1rem to preserve their font size 16px.

@@ -149,6 +149,11 @@ footer {
color: color('primary');
}

.usa-radio {
margin-top: 1rem;
font-size: 1.06rem;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to preserve styling of our radio buttons (bigger text, margins)

@@ -34,6 +34,9 @@ h2 {
.usa-form,
.usa-form fieldset {
font-size: 1rem;
.usa-legend {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that our field headings and subheadings are moved to the legends in the screenreader fix, we're updating the font size of our legends to maintain our font size of 16px (originally 16.96px from usa-legend font size default being 1.06rem)

@@ -2,15 +2,25 @@
class="{% if label_classes %} {{ label_classes }}{% endif %}{% if label_tag == 'legend' %} {{ legend_classes }}{% endif %}"
{% if not field.use_fieldset %}for="{{ widget.attrs.id }}"{% endif %}
>
{% if span_for_text %}
<span>{{ field.label }}</span>
{% if legend_heading %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Integrates legends into our labels so that their contents are tied to screenreader output

Copy link

github-actions bot commented Dec 6, 2024

🥳 Successfully deployed to developer sandbox es.

2 similar comments
Copy link

github-actions bot commented Dec 6, 2024

🥳 Successfully deployed to developer sandbox es.

Copy link

github-actions bot commented Dec 6, 2024

🥳 Successfully deployed to developer sandbox es.

@erinysong
Copy link
Contributor Author

erinysong commented Dec 6, 2024

Scope question - do we want expand these screenreader outputs to the portfolio forms as well? I can change them wherever applicable but feel free to let me know if we want this only scoped to our non org model domain request form


{% with add_class="usa-radio__input--tile" add_legend_class="usa-sr-only" %}
<div class="margin-top-2">
{% with add_class="usa-radio__input--tile" add_legend_heading="Are there other employees who can help verify your request?" %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

widget=forms.Textarea(),
widget=forms.Textarea(
attrs={
"aria-label": "You don’t need to provide names of other employees now, \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2918 and #2915

<!-- Toggle -->
<em>Select one. <abbr class="usa-hint usa-hint--required" title="required">*</abbr></em>
{% with add_class="usa-radio__input--tile" add_legend_class="usa-sr-only" %}
{% with add_class="usa-radio__input--tile" add_legend_class="margin-top-0" add_legend_heading="Are you working with a CISA regional representative on your domain request?" %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

{% if legend_heading %}
<h2 class="{{ legend_classes }}">{{ legend_heading }} </h2>
{% if widget.attrs.id == 'id_additional_details-has_cisa_representative' %}
<p>.gov is managed by the Cybersecurity and Infrastructure Security Agency. CISA has <a href="https://www.cisa.gov/about/regions" target="_blank">10 regions</a> that some organizations choose to work with. Regional representatives use titles like protective security advisors, cyber security advisors, or election security advisors.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

<!-- Toggle -->
<em>Select one. <abbr class="usa-hint usa-hint--required" title="required">*</abbr></em>
{% with add_class="usa-radio__input--tile" add_legend_class="usa-sr-only" %}
{% with add_class="usa-radio__input--tile" add_legend_heading="Is there anything else you’d like us to know about your domain request?" %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erinysong erinysong marked this pull request as ready for review December 9, 2024 19:34
@SamiyahKey SamiyahKey requested review from SamiyahKey and removed request for a team December 9, 2024 20:53
Copy link

@SamiyahKey SamiyahKey left a comment

Choose a reason for hiding this comment

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

Approved, thanks Erin!

Copy link

github-actions bot commented Dec 9, 2024

🥳 Successfully deployed to developer sandbox es.

1 similar comment
Copy link

github-actions bot commented Dec 9, 2024

🥳 Successfully deployed to developer sandbox es.

Copy link

🥳 Successfully deployed to developer sandbox es.

Copy link

🥳 Successfully deployed to developer sandbox es.

Copy link

🥳 Successfully deployed to developer sandbox es.

Copy link

🥳 Successfully deployed to developer sandbox es.

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.

Bundle: Request form: Screen reader output
2 participants