Nj 78 axe a11y #5323

mmazanec22 · 2025-01-07T22:41:10Z

Link to pivotal/JIRA issue

Most issues fixed in this PR were identified previously in https://github.com/newjersey/affordability-pm/issues/78

Those previously identified issues are explained in greater detail in these tickets (which should be publicly visible):
newjersey/accessibility-pm#1
newjersey/accessibility-pm#2
newjersey/accessibility-pm#15
newjersey/accessibility-pm#8

And this critical issue is fixed as well:
newjersey/accessibility-pm#9

Is PM acceptance required? (delete one)

Yes - don't merge until JIRA issue is accepted!

We probably also want CfA design review to make sure there are no unwanted visual changes.

What was done?

Tackle a handful of small accessibility issues and improve e2e testing.

How to test?

Navigate using the catchall persona to see as many screens as possible.

There should be no visual changes besides the keyboard focus (the critical issue linked above), back button color contrast, and dev button color contrast.

CfA reviewers, please check that the landmark changes don't affect anything that our team wouldn't notice.

Screenshots (for visual changes)

Used the :focus-visible pseudo class to make sure that there's no focus on click for the reveal, but there is a focus for keyboard users

Back button color contrast is improved
Before

After

github-actions · 2025-01-07T22:45:31Z

Heroku app: https://gyr-review-app-5323-3ed2fa7daa99.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5323 (optionally add --tail)

mluedke2 · 2025-01-08T18:11:08Z

app/views/ctc/ctc_pages/_question.html.erb

@@ -14,4 +14,4 @@
      </div>
    </div>
  </div>
-</main>
+</div>


[dust] I confess I don't understand this change -- which of the issues does this fix? I'm used to introducing main when possible as being more semantic, so on a surface level this surprises me.

Edit: After looking at more of this diff, I wonder if it is because this ERB feeds into a larger page and thus the main here would conflict with a main from others?

Yes, we should only have one main landmark per page. See newjersey/accessibility-pm#1

got it, thanks!

mluedke2 · 2025-01-08T18:26:44Z

app/views/state_file/questions/nj_eligibility_health_insurance/edit.html.erb

@@ -17,7 +17,6 @@
                  label: t("general.negative"),
                  input_html: {
                    "data-follow-up": "#household-members",
-                    "aria-controls": "household-members" 


[dust] I also don't understand the removal of this -- was it not effective or correct as it was?

Axe flagged it, so I think it was either not correct, or needs to be added with js when the associated element appears, or was a problem bc of nested fieldsets. Going to surface this with Damian after the fieldset PR lands

mluedke2 · 2025-01-08T18:42:56Z

spec/features/state_file/complete_intake_spec.rb

@@ -651,31 +651,40 @@

  context "NJ", :flow_explorer_screenshot, js: true do

-    def advance_to_start_of_intake(df_persona_name)
+    def advance_to_start_of_intake(df_persona_name, check_a11y: false)


[dust] I'm wondering why this is a toggle as opposed to always checking accessibility. Is checking costly in terms of time, so turning it off for most tests would reduce test suite time?

Yes. I want to avoid duplicate checks of the same pages

Makes sense! I'm curious about the scale of time saved, for future reference. But sounds good to me! (Also makes me wonder if there are other places in the slower tests like E2E or PDF filling that we can toggle off to save time... the CI taking 15 minutes is ok but feels like it could be quicker).

mluedke2

thanks for addressing my questions -- looks good to me!

jachan

Looks good! Had one question about alt text.

jachan · 2025-01-08T21:35:06Z

app/views/state_file/state_file_pages/about_page.html.erb

@@ -25,7 +25,7 @@
      </div>
    <% end %>
  </div>
-  <%= image_tag 'questions/welcome.svg', class: 'fyst-home-image' %>
+  <%= image_tag 'questions/welcome.svg', class: 'fyst-home-image', alt: '' %>


[DUST] Would it be helpful to fill in the alt text here for the visually impaired, or is that unnecessary/overkill?

This specifically needs an empty alt tag because it is a decorative image

jachan · 2025-01-08T21:35:47Z

spec/features/state_file/complete_intake_spec.rb

-      advance_to_property_tax_page("Minimal", skip_health_insurance_eligibility: true)
+      advance_to_start_of_intake("O neal walker catchall mfj", check_a11y: true)
+
+      expect(page).to be_axe_clean


[DUST] Yay expectations on a11y issues!

mrotondo

I have a few questions (one visual, one about html structure) but overall (with my very limited understanding of modern html, accessibility and landmarks) this seems like a reasonable change.

mrotondo · 2025-01-08T22:05:01Z

app/assets/stylesheets/templates/_question.scss

@@ -10,17 +10,17 @@
        padding-right: .5rem;
      }
      font-size: 1.6rem;
-      border: 1px solid $color-grey-medium;
+      border: 1px solid $color-grey-dark;


I'm not sure why this wouldn't result in a visual change, can you explain?

This is one of the visual changes I called out, and included a screen shot for

Sorry I missed that! Please post both before & after screenshots for visual changes, and please tag Anu & Madison on the slack thread for this PR to make sure they've gotten eyes on it before it goes in.

mrotondo · 2025-01-08T23:07:41Z

app/views/layouts/application.html.erb

-        </div>
+        </main>
+
+        <% if content_for :footer %>


Looks like <header> and <footer> are defined in different places as of this change - should that be the case? Is there a benefit to bringing the <footer> element up into this layout file, or is it impractical?

TBH I was trying to make the lightest touch possible given the scope of the other changes I had to make, and it seemed like the footer didn't need intervention. Happy to bring it in here though if that's better!

That's reasonable - just wanted to make sure I understood the rationale for the different treatment. I'm fine with it staying like this!

mrotondo

lgtm, but as noted in my comment please post before & after screenshots and ping our designers to make sure they know about this change

Remove extra header tags Remove extra main tags Move footer, add empty alt tags, add more a11y checks, adapt css to new page structure Address header and focus issues on landing page

mmazanec22 · 2025-01-10T19:36:29Z

Image should have been globally removed??

mluedke2 reviewed Jan 8, 2025

View reviewed changes

mluedke2 approved these changes Jan 8, 2025

View reviewed changes

mmazanec22 assigned mmazanec22 and unassigned mmazanec22 Jan 8, 2025

jachan approved these changes Jan 8, 2025

View reviewed changes

mrotondo reviewed Jan 8, 2025

View reviewed changes

mmazanec22 force-pushed the nj-78-axe-a11y branch from fd03469 to 7396c29 Compare January 9, 2025 19:41

mrotondo approved these changes Jan 9, 2025

View reviewed changes

mmazanec22 force-pushed the nj-78-axe-a11y branch from 7396c29 to efc2fe4 Compare January 9, 2025 21:55

Move content into landmarks

fbbeb2c

Remove extra header tags Remove extra main tags Move footer, add empty alt tags, add more a11y checks, adapt css to new page structure Address header and focus issues on landing page

mmazanec22 force-pushed the nj-78-axe-a11y branch from efc2fe4 to fbbeb2c Compare January 9, 2025 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nj 78 axe a11y #5323

Nj 78 axe a11y #5323

mmazanec22 commented Jan 7, 2025 •

edited

Loading

github-actions bot commented Jan 7, 2025

mluedke2 Jan 8, 2025 •

edited

Loading

mmazanec22 Jan 8, 2025

mluedke2 Jan 8, 2025

mluedke2 Jan 8, 2025

mmazanec22 Jan 8, 2025

mluedke2 Jan 8, 2025

mmazanec22 Jan 8, 2025

mluedke2 Jan 8, 2025

mluedke2 left a comment

jachan left a comment

jachan Jan 8, 2025

mmazanec22 Jan 8, 2025

jachan Jan 8, 2025

mrotondo left a comment

mrotondo Jan 8, 2025

mmazanec22 Jan 9, 2025

mrotondo Jan 9, 2025

mrotondo Jan 8, 2025 •

edited

Loading

mmazanec22 Jan 9, 2025

mrotondo Jan 9, 2025

mrotondo left a comment

mmazanec22 commented Jan 10, 2025

@@ @@ -14,4 +14,4 @@ @@
                     </div>
                   </div>
                 </div>
-              </main>
+              </div>

Nj 78 axe a11y #5323

Are you sure you want to change the base?

Nj 78 axe a11y #5323

Conversation

mmazanec22 commented Jan 7, 2025 • edited Loading

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

What was done?

How to test?

Screenshots (for visual changes)

github-actions bot commented Jan 7, 2025

mluedke2 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mluedke2 left a comment

Choose a reason for hiding this comment

jachan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrotondo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrotondo Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrotondo left a comment

Choose a reason for hiding this comment

mmazanec22 commented Jan 10, 2025

mmazanec22 commented Jan 7, 2025 •

edited

Loading

mluedke2 Jan 8, 2025 •

edited

Loading

mrotondo Jan 8, 2025 •

edited

Loading