From 426274ba25a9eec22ef9f20b4edeadf1a342819c Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 3 May 2023 12:14:59 -0400 Subject: [PATCH 1/3] LG-9424: Simplify MFA setup option labels changelog: User-Facing Improvements, Accessibility, Improve labels for MFA setup options --- .../_mfa_selection.html.erb | 52 +++++---- app/views/users/mfa_selection/index.html.erb | 2 +- .../index.html.erb | 2 +- .../features/accessibility/user_pages_spec.rb | 7 ++ spec/support/matchers/accessibility.rb | 102 ++++++++++++++++++ 5 files changed, 141 insertions(+), 24 deletions(-) diff --git a/app/views/partials/multi_factor_authentication/_mfa_selection.html.erb b/app/views/partials/multi_factor_authentication/_mfa_selection.html.erb index f268fde2769..453d4f50783 100644 --- a/app/views/partials/multi_factor_authentication/_mfa_selection.html.erb +++ b/app/views/partials/multi_factor_authentication/_mfa_selection.html.erb @@ -1,22 +1,30 @@ -
- <%= check_box_tag( - 'two_factor_options_form[selection][]', - option.type, - false, - disabled: option.disabled?, - checked: option.disabled?, - class: 'usa-checkbox__input usa-checkbox__input--tile', - id: "two_factor_options_form_selection_#{option.type}", - ) %> - <%= label_tag( - "two_factor_options_form_selection_#{option.type}", - class: 'usa-checkbox__label usa-checkbox__label--illustrated', - ) do %> - <%= image_tag(asset_url("mfa-options/#{option.type}.svg"), alt: "#{option.label} icon", class: 'usa-checkbox__image') %> -
<%= option.label %> <%= option.mfa_configuration_description %> - - <%= option.info %> - -
- <% end %> -
+<%= check_box_tag( + 'two_factor_options_form[selection][]', + option.type, + false, + disabled: option.disabled?, + checked: option.disabled?, + class: 'usa-checkbox__input usa-checkbox__input--tile', + id: "two_factor_options_form_selection_#{option.type}", + aria: { + describedby: [ + "two_factor_options_form_selection_description_#{option.type}", + "two_factor_options_form_selection_info_#{option.type}", + ], + }, + ) %> +<%= label_tag( + "two_factor_options_form_selection_#{option.type}", + class: 'usa-checkbox__label usa-checkbox__label--illustrated', + ) do %> + <%= image_tag(asset_url("mfa-options/#{option.type}.svg"), alt: '', class: 'usa-checkbox__image') %> +
+ <%= option.label %> + + +
+<% end %> diff --git a/app/views/users/mfa_selection/index.html.erb b/app/views/users/mfa_selection/index.html.erb index 7ecd64e0bf0..457343d024d 100644 --- a/app/views/users/mfa_selection/index.html.erb +++ b/app/views/users/mfa_selection/index.html.erb @@ -15,7 +15,7 @@ <% @presenter.options.each do |option| %>
" class="<%= option.html_class %>"> <%= render partial: 'partials/multi_factor_authentication/mfa_selection', - locals: { form: f, option: option } %> + locals: { option: option } %>
<% end %> diff --git a/app/views/users/two_factor_authentication_setup/index.html.erb b/app/views/users/two_factor_authentication_setup/index.html.erb index d9e173e090c..7bc97e5d615 100644 --- a/app/views/users/two_factor_authentication_setup/index.html.erb +++ b/app/views/users/two_factor_authentication_setup/index.html.erb @@ -25,7 +25,7 @@ <% @presenter.options.each do |option| %>
" class="<%= option.html_class %>"> <%= render partial: 'partials/multi_factor_authentication/mfa_selection', - locals: { form: f, option: option } %> + locals: { option: option } %>
<% end %> diff --git a/spec/features/accessibility/user_pages_spec.rb b/spec/features/accessibility/user_pages_spec.rb index 52eb5fff708..4db16b14c20 100644 --- a/spec/features/accessibility/user_pages_spec.rb +++ b/spec/features/accessibility/user_pages_spec.rb @@ -46,6 +46,13 @@ expect(current_path).to eq(authentication_methods_setup_path) expect_page_to_have_no_accessibility_violations(page) + phone_checkbox = page.find_field('two_factor_options_form_selection_phone', visible: :all) + expect(phone_checkbox).to have_name( + t('two_factor_authentication.two_factor_choice_options.phone'), + ) + expect(phone_checkbox).to have_description( + t('two_factor_authentication.two_factor_choice_options.phone_info'), + ) end scenario 'phone setup page' do diff --git a/spec/support/matchers/accessibility.rb b/spec/support/matchers/accessibility.rb index 65f7bb6870c..51df9c23059 100644 --- a/spec/support/matchers/accessibility.rb +++ b/spec/support/matchers/accessibility.rb @@ -109,6 +109,108 @@ def descriptors(element) end end +RSpec::Matchers.define :have_name do |name| + # Implements a best effort approximation of W3C Accessible Name and Description Computation 1.2 + # See: https://www.w3.org/TR/accname-1.2/#mapping_additional_nd_te + # + # In the future, consider implementing using Chrome DevTools Accessibility features. At time of + # writing, these are experimental and return a `nil` value in local testing. + # + # See: https://chromedevtools.github.io/devtools-protocol/tot/Accessibility/ + # + # We can use the Capybara driver "bridge" to call these commands. + # + # Example: https://github.com/18F/identity-idp/blob/3c7d3be/spec/support/features/browser_emulation_helper.rb + + def hidden_name(element) + # "If the current node is hidden [...] return the empty string." + # + # Note: This should also check page visibility, but Capybara's Element#visible? considers off- + # screen elements as non-visible, which is not the same as how it's considered for names. + '' if element['aria-hidden'] == 'true' + end + + def aria_labelledby_name(element) + # "if computing a name, and the current node has an aria-labelledby attribute that contains at + # least one valid IDREF, and the current node is not already part of an aria-labelledby + # traversal, process its IDREFs in the order they occur" + valid_labels = element['aria-labelledby']&. + split(' ')&. + map { |label_id| page.find("##{label_id}")&.text }&. + compact + + valid_labels.join('') if valid_labels.present? + end + + def aria_label_name(element) + # "Otherwise, if computing a name, and if the current node has an aria-label attribute whose + # value is not the empty string, nor, when trimmed of white space, is not the empty string:" + element['aria-label'] + end + + def referenced_label_name(element) + # "Otherwise, if the current node's native markup provides an attribute (e.g. title) or element + # (e.g. HTML label) that defines a text alternative, return that alternative in the form of a + # flat string as defined by the host language" + descendent_name(page.find("label[for='#{element['id']}']")) + rescue Capybara::ElementNotFound + nil + end + + def ancestor_label_text_name(element) + # "Otherwise, if the current node is a control embedded within the label" + descendent_name(element.find(:xpath, 'ancestor::label')) + rescue Capybara::ElementNotFound + nil + end + + def descendent_name(element) + # "Otherwise, if the current node is a descendant of an element whose Accessible Name or + # Accessible Description is being computed, and contains descendants, proceed to 2F.i." + # + # Note: There should probably be some recursion here with the other methods, but Capybara only + # allows us to work with elements and not text nodes, which presents a challenge for computing + # the descendent name. + visible_text_js = <<~JS + (function getVisibleChildNodeText(childNodes) { + return Array.from(childNodes).flatMap((child) => { + switch (child.nodeType) { + case Node.TEXT_NODE: + return child.nodeValue; + case Node.ELEMENT_NODE: + if (child.getAttribute('aria-hidden') !== 'true') { + return child.nodeName === 'IMG' ? + child.getAttribute('alt') : + getVisibleChildNodeText(child.childNodes); + } + } + }); + })(arguments[0].childNodes).filter(Boolean).join('').trim(); + JS + + page.evaluate_script(visible_text_js, element) + end + + def computed_name(element) + Enumerator.new do |yielder| + yielder << hidden_name(element) + yielder << aria_labelledby_name(element) + yielder << aria_label_name(element) + yielder << referenced_label_name(element) + yielder << ancestor_label_text_name(element) + end.lazy.compact.first + end + + match { |element| computed_name(element) == name } + + failure_message do |element| + <<-STR.squish + Expected element would have computed name "#{name}". + Found #{computed_name(element)}. + STR + end +end + RSpec::Matchers.define :be_uniquely_titled do # Attempts to validate conformance to WCAG Success Criteria 2.4.2: Page Titled # From f86945b03fff2e6ac9925c48417f630f5e216e69 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 3 May 2023 13:06:10 -0400 Subject: [PATCH 2/3] Update _mfa_selection.html.erb_spec.rb --- .../_mfa_selection.html.erb_spec.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/spec/views/partials/multi_factor_authentication/_mfa_selection.html.erb_spec.rb b/spec/views/partials/multi_factor_authentication/_mfa_selection.html.erb_spec.rb index ceda12d10b2..63c822a7b00 100644 --- a/spec/views/partials/multi_factor_authentication/_mfa_selection.html.erb_spec.rb +++ b/spec/views/partials/multi_factor_authentication/_mfa_selection.html.erb_spec.rb @@ -21,8 +21,12 @@ } end - it 'renders a field with mfa-selection class' do - expect(rendered).to have_css('.mfa-selection') + it 'renders an unchecked, enabled checkbox field' do + expect(rendered).to have_field( + 'two_factor_options_form[selection][]', + checked: false, + disabled: false, + ) end end From 216adb7a40274dc9fd89140be79bfcbae9be1c7a Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 5 May 2023 08:00:06 -0400 Subject: [PATCH 3/3] Simplify computed name --- spec/support/matchers/accessibility.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/spec/support/matchers/accessibility.rb b/spec/support/matchers/accessibility.rb index 51df9c23059..72d1418fff8 100644 --- a/spec/support/matchers/accessibility.rb +++ b/spec/support/matchers/accessibility.rb @@ -192,13 +192,11 @@ def descendent_name(element) end def computed_name(element) - Enumerator.new do |yielder| - yielder << hidden_name(element) - yielder << aria_labelledby_name(element) - yielder << aria_label_name(element) - yielder << referenced_label_name(element) - yielder << ancestor_label_text_name(element) - end.lazy.compact.first + hidden_name(element) || + aria_labelledby_name(element) || + aria_label_name(element) || + referenced_label_name(element) || + ancestor_label_text_name(element) end match { |element| computed_name(element) == name }