LG-12465 VPAT 1.1.1 Safari VoiceOver svg with null alt by kevinsmaster5 · Pull Request #10200 · 18F/identity-idp

kevinsmaster5 · 2024-03-04T19:28:58Z

🎫 Ticket

Link to the relevant ticket:
LG-12465

🛠 Summary of changes

Instead of skipping over SVG images with alt="" attribute, VoiceOver in Safari is reading them as 'image'. Multi-part SVG would read 'image, image, image, etc.' Adding role='img' to the SVG eliminates the bug.
https://a11y-101.com/development/svg

📜 Testing Plan

Using MacOS with Safari and VoiceOver capability

go to http://localhost:3000 and setup a new account
Add 1 MFA method
When you get to /auth_method_confirmation go into Mac settings and pick Accessibility
Click VoiceOver, Open VoiceOver Utility and click the slider to activate
In the Authentication method confirmation window click refresh. The screen should reload and VoiceOver should be reading through the document.
Observe that it no longer stops on the svg but skips to the heading below as intended.

Before fix
https://github.com/18F/identity-idp/assets/135744319/6ae9386e-7ef5-4690-b8b1-f74e7b04b24b

After fix
https://github.com/18F/identity-idp/assets/135744319/9891c134-1aa4-4363-8c55-71ad0dc5e808

zachmargolis · 2024-03-04T19:44:08Z

Is there a way we can catch these with a regression spec? Like augmenting the aXe checks or adding a new check for alt='' and adding role=img here?

https://github.com/18F/identity-idp/blob/main/spec/support/matchers/accessibility.rb#L291

kevinsmaster5 · 2024-03-04T22:18:21Z

Is there a way we can catch these with a regression spec? Like augmenting the aXe checks or adding a new check for alt='' and adding role=img here?

https://github.com/18F/identity-idp/blob/main/spec/support/matchers/accessibility.rb#L291

Would that correct for the failing tests?
It seems like SVG should be allowed to use the role "img" especially in this context.
https://dequeuniversity.com/rules/axe/4.7/aria-allowed-role?application=axeAPI
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/img_role#svg_and_roleimg

zachmargolis · 2024-03-04T22:55:55Z

Would that correct for the failing tests?
It seems like SVG should be allowed to use the role "img" especially in this context.

I think so? Here's the gist of what I was thinking, we could probably update the error description a bit

diff --git a/spec/support/matchers/accessibility.rb b/spec/support/matchers/accessibility.rb
index 19659c2b5..fc9798125 100644
--- a/spec/support/matchers/accessibility.rb
+++ b/spec/support/matchers/accessibility.rb
@@ -170,6 +170,27 @@ RSpec::Matchers.define :have_unique_form_landmark_labels do
   end
 end
 
+RSpec::Matchers.define :tag_decorative_svgs_with_role do
+  def decorative_svgs(page)
+    page.all(:css, 'img[alt=""][src$=".svg" i]')
+  end
+
+  match do |page|
+    expect(decorative_svgs(page)).to all satisfy { |img| img[:role] == 'img' }
+  end
+
+  failure_message do |page|
+    img_tags = decorative_svgs(page).reject { |img| img[:role] == 'img' }
+                .map { |img| %|<img alt="#{img[:alt]}" src="#{img[:src]}" class="#{img[:class]}">| }
+                .join("\n")
+
+    <<~STR
+      Expect all decorative SVGs to have role="img", but found ones without:
+      #{img_tags}
+    STR
+  end
+end
+
 class AccessibleName
   attr_reader :page
 
@@ -293,6 +314,7 @@ def expect_page_to_have_no_accessibility_violations(page, validate_markup: true)
   expect(page).to have_valid_idrefs
   expect(page).to label_required_fields
   expect(page).to have_valid_markup if validate_markup
+  expect(page).to tag_decorative_svgs_with_role
 end
 
 def activate_skip_link

that gives us a failure message like this:

identity-idp:main> SKIP_BUILD=true bundle exec rspec spec/features/accessibility/user_pages_spec.rb --fail-fast

Randomized with seed 46916

Accessibility on pages that require authentication
  set up authenticator app page (FAILED - 1)

Failures:

  1) Accessibility on pages that require authentication set up authenticator app page
     Failure/Error: expect(page).to tag_decorative_svgs_with_role
     
       Expect all decorative SVGs to have role="img", but found ones without:
       <img alt="" src="http://127.0.0.1:54889/assets/sp-logos/square-gsa-43d77906.svg" class="float-left margin-right-1">
     # ./spec/support/matchers/accessibility.rb:317:in `expect_page_to_have_no_accessibility_violations'
     # ./spec/features/accessibility/user_pages_spec.rb:172:in `block (2 levels) in <top (required)>'
     # ./spec/rails_helper.rb:130:in `block (2 levels) in <top (required)>'
     # ./spec/support/fake_analytics.rb:174:in `block (2 levels) in <main>'

Finished in 6.48 seconds (files took 1.93 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/features/accessibility/user_pages_spec.rb:167 # Accessibility on pages that require authentication set up authenticator app page

kevinsmaster5 · 2024-03-06T13:07:51Z

Thank you, @zachmargolis that snippet works great. Is there something more that needs to be done to ./spec/support/matchers/accessibility because it still doesn't like the use of the img role.

It seems to be caught under :"best-practice"

Accessibility on IDV pages IDV pages doc auth steps accessibility
     Failure/Error: expect(page).to be_axe_clean.according_to :section508, :"best-practice", :wcag21aa
       Found 1 accessibility violation:
       1) aria-allowed-role: ARIA role should be appropriate for the element (minor)
           https://dequeuniversity.com/rules/axe/4.7/aria-allowed-role?application=axeAPI
           The following 1 node violate this rule:
           
               Selector: .float-left
               HTML: <img alt="" class="float-left margin-right-1" role="img" src="http://127.0.0.1:33497/assets/sp-logos/square-gsa-43d77906.svg" width="20" height="20">
               Fix any of the following:
               - ARIA role img is not allowed for given element

aduth · 2024-03-06T13:30:41Z

Originally I thought Axe might be flagging it since it'd be redundant with <img /> default role, but based on the logic of "Implicit ARIA role" described on the element's MDN article, maybe it should be assigned as "presentation", which makes sense since an empty alt would imply it's decorative and therefore not meaningful to be exposed to assistive technology.

But I'm curious if that'd conflict with the fix being effective in Safari. Can you link to a resource about why we'd want to add role="img" to a decorative image? The examples in the linked a11y-101 article all have non-empty alt values.

kevinsmaster5 · 2024-03-06T15:50:14Z

I should have tested locally before going right to pushing change to 'presentation'. The bug manifests with the 'presentation' role also.
I'm struggling to find examples with empty alt attributes to be honest. It does remedy the bug and what mdn is recommending here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#identifying_svg_as_an_image

kevinsmaster5 · 2024-03-06T21:46:19Z

spec/support/matchers/accessibility.rb

Not sure if skipping all role attributes is a good solution here.

Can I modify tag_decorative_svgs_with_role to compensate, I wonder.

I had a feeling that if we need to bypass typical best practices for img role that we might have to do something like this, so it's not totally unexpected to me. I'd wonder if we could at least limit it to the very specific case we're trying to exempt, i.e. img role and not anything else that Axe might be looking for.

I haven't done any research to validate the idea, but I'd also wonder if we could mark the image as aria-hidden="true" if it's decorative anyways. My understanding is that the expected behavior for a screen reader with a decorative image (alt="") is to skip it entirely and omit it from the page structure, which is the same as what happens with aria-hidden="true".

I missed that you'd revised this to target the image specifically, so my previous comment may not be necessary. I'll review shortly.

Yeah, Zach's selector up above for the decorative svg function worked great there. 🙂

…alt=null images

aduth

LGTM, and tested locally 👍

One suggestion about adding a code comment to explain the solution.

aduth · 2024-03-07T16:29:48Z

spec/support/matchers/accessibility.rb

+  expect(page).to be_axe_clean.according_to(
+    :section508, :"best-practice",
+    :wcag21aa
+  ).excluding('img[alt=""][src$=".svg" i]')


Can we add an inline code comment to help explain why this is here?

Suggested change

).excluding('img[alt=""][src$=".svg" i]')

).

# Axe flags redundant img role on img elements, but is necessary for a Safari bug

# See: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#identifying_svg_as_an_image

excluding('img[alt=""][src$=".svg" i]')

What is the i portion of this selector doing? I've not seen that before.

It's there to flag case-insensitive. The test passes without it though so I'm not sure if it's needed here.
https://caniuse.com/css-case-insensitive

kevinsmaster5 commented Mar 6, 2024

View reviewed changes

kevinsmaster5 added 8 commits March 7, 2024 10:56

changelog: Bug Fixes, Accessibility, Safari use of VoiceOver reading …

bb6b6d8

…alt=null images

add ability to check for role in svg

f52ab33

change role from 'img' to 'presentation'

f79c01b

reset role to img

a0f56d6

revert test for svg role

7ec66ee

fix lint

5907711

add skipping method to axe_clean violation test

f502e5d

make axe exclude more specific

0a6a2f1

kevinsmaster5 force-pushed the kmas-lg-12465-vpat1111-svg-role branch from 8e31585 to 0a6a2f1 Compare March 7, 2024 15:56

kevinsmaster5 marked this pull request as ready for review March 7, 2024 16:12

kevinsmaster5 requested a review from a team March 7, 2024 16:13

aduth approved these changes Mar 7, 2024

View reviewed changes

add comment explaining exclude matcher

c435eaf

kevinsmaster5 merged commit 1eee4f9 into main Mar 7, 2024

kevinsmaster5 deleted the kmas-lg-12465-vpat1111-svg-role branch March 7, 2024 17:51

zachmargolis mentioned this pull request Mar 11, 2024

LG-12595 Fix Selfie Screenreader Behavior by adding <FullScreen /> #10228

Merged

12 tasks

amirbey mentioned this pull request Mar 12, 2024

Deploy RC 363 to Production #10234

Merged

aduth mentioned this pull request Jul 25, 2024

Use aria-hidden for decorative SVG images #10986

Merged

Conversation

kevinsmaster5 commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Uh oh!

zachmargolis commented Mar 4, 2024

Uh oh!

kevinsmaster5 commented Mar 4, 2024

Uh oh!

zachmargolis commented Mar 4, 2024

Uh oh!

kevinsmaster5 commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented Mar 6, 2024

Uh oh!

kevinsmaster5 commented Mar 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevinsmaster5 commented Mar 4, 2024 •

edited

Loading

kevinsmaster5 commented Mar 6, 2024 •

edited

Loading

aduth Mar 6, 2024 •

edited

Loading