From 4de988cb2b4441a0b1e48a60cf8e36a1d5c3929b Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 31 Oct 2023 09:50:43 -0400 Subject: [PATCH 1/5] LG-11278: Group TOTP fields with fieldset changelog: User-Facing Improvements, Accessibility, Improve grouping semantics for authentication app setup form --- app/views/users/totp_setup/new.html.erb | 79 +++++++++++++------------ 1 file changed, 42 insertions(+), 37 deletions(-) diff --git a/app/views/users/totp_setup/new.html.erb b/app/views/users/totp_setup/new.html.erb index fa6e8228d61..df08d180fca 100644 --- a/app/views/users/totp_setup/new.html.erb +++ b/app/views/users/totp_setup/new.html.erb @@ -3,7 +3,7 @@ <%= render PageHeadingComponent.new.with_content(t('headings.totp_setup.new')) %>

- <%= t('forms.totp_setup.totp_intro') %> + <%= t('forms.totp_setup.totp_intro') %> <%= new_tab_link_to( t('links.what_is_totp'), help_center_redirect_path( @@ -17,46 +17,51 @@

<%= simple_form_for('', method: :patch, html: { class: 'margin-bottom-4' }) do |f| %> - <%= render ProcessListComponent.new(connected: true, class: 'margin-y-4') do |c| %> - <%= c.with_item(heading: t('forms.totp_setup.totp_step_1')) do %> -

<%= t('forms.totp_setup.totp_step_1a') %>

- <%= render ValidatedFieldComponent.new( - form: f, - name: :name, - required: true, - label: false, - wrapper_html: { class: 'margin-bottom-0' }, - input_html: { - aria: { label: t('forms.totp_setup.totp_step_1') }, - maxlength: 20, - }, - ) %> - <% end %> - <%= c.with_item(heading: t('forms.totp_setup.totp_step_2')) %> - <%= c.with_item(heading: t('forms.totp_setup.totp_step_3')) do %> -
- <%= image_tag @qrcode, size: 240, skip_pipeline: true, alt: t('image_description.totp_qrcode') %> -
-

<%= t('instructions.mfa.authenticator.manual_entry') %>

- - <%= @code %> - - <%= render ClipboardButtonComponent.new(clipboard_text: @code.upcase, outline: true) %> - <% end %> - <%= c.with_item(heading: t('forms.totp_setup.totp_step_4')) do %> - <%= render OneTimeCodeInputComponent.new( - form: f, - transport: nil, - code_length: TwoFactorAuthenticatable::OTP_LENGTH, - field_options: { +
+ <%= render ProcessListComponent.new(connected: true) do |c| %> + <%= c.with_item(heading: t('forms.totp_setup.totp_step_1')) do %> +

<%= t('forms.totp_setup.totp_step_1a') %>

+ <%= render ValidatedFieldComponent.new( + form: f, + name: :name, + required: true, label: false, + wrapper_html: { class: 'margin-bottom-0' }, input_html: { - aria: { label: t('forms.totp_setup.totp_step_4') }, + aria: { label: t('forms.totp_setup.totp_step_1') }, + maxlength: 20, + }, + ) %> + <% end %> + <%= c.with_item(heading: t('forms.totp_setup.totp_step_2')) %> + <%= c.with_item(heading: t('forms.totp_setup.totp_step_3')) do %> +
+ <%= image_tag @qrcode, size: 240, skip_pipeline: true, alt: t('image_description.totp_qrcode') %> +
+

<%= t('instructions.mfa.authenticator.manual_entry') %>

+ + <%= @code %> + + <%= render ClipboardButtonComponent.new(clipboard_text: @code.upcase, outline: true) %> + <% end %> + <%= c.with_item(heading: t('forms.totp_setup.totp_step_4')) do %> + <%= render OneTimeCodeInputComponent.new( + form: f, + transport: nil, + code_length: TwoFactorAuthenticatable::OTP_LENGTH, + field_options: { + label: false, + wrapper_html: { + class: 'margin-bottom-0', + }, + input_html: { + aria: { label: t('forms.totp_setup.totp_step_4') }, + }, }, - }, - ) %> + ) %> + <% end %> <% end %> - <% end %> +
<%= f.input( :remember_device, as: :boolean, From 148116ae1aa53c25d8d0ea835e7c6347725c29f8 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 31 Oct 2023 10:05:57 -0400 Subject: [PATCH 2/5] Add spec assertion for logical grouping --- spec/features/users/totp_management_spec.rb | 7 ++++ spec/support/matchers/accessibility.rb | 46 +++++++++++++++------ 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/spec/features/users/totp_management_spec.rb b/spec/features/users/totp_management_spec.rb index e02c2df3e3d..e7610030323 100644 --- a/spec/features/users/totp_management_spec.rb +++ b/spec/features/users/totp_management_spec.rb @@ -43,6 +43,13 @@ click_link t('account.index.auth_app_add'), href: authenticator_setup_url + expect( + [ + page.find_field(t('forms.totp_setup.totp_step_1')), + page.find_field(t('forms.totp_setup.totp_step_4')), + ], + ).to be_logically_grouped(t('forms.totp_setup.totp_intro')) + secret = find('#qr-code').text fill_in 'name', with: 'foo' fill_in 'code', with: generate_totp_code(secret) diff --git a/spec/support/matchers/accessibility.rb b/spec/support/matchers/accessibility.rb index 5e2eca1095b..356ed713fc6 100644 --- a/spec/support/matchers/accessibility.rb +++ b/spec/support/matchers/accessibility.rb @@ -50,6 +50,28 @@ end end +RSpec::Matchers.define :be_logically_grouped do |name| + match do |inputs| + group_names = inputs.map do |input| + group = input.ancestor('fieldset,[role=group]') + name = group['aria-label'] + begin + name ||= group.find('legend').text if group.tag_name == 'fieldset' + rescue Capybara::ElementNotFound; end + name ||= aria_labelledby_name(group) + name + end + + group_names.compact.uniq == [name] + end + + failure_message do + <<-STR.squish + Expected inputs to be logically grouped with as "#{name}" + STR + end +end + RSpec::Matchers.define :have_valid_markup do def page_html if page.driver.is_a?(Capybara::Selenium::Driver) @@ -132,18 +154,6 @@ def hidden_name(element) '' 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:" @@ -224,3 +234,15 @@ def activate_skip_link expect(page.active_element).to have_content(t('shared.skip_link'), wait: 5) page.active_element.send_keys(:enter) 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 From 3bc49c90da28e30d1e655a76528c0089b0c55ca8 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 31 Oct 2023 10:28:33 -0400 Subject: [PATCH 3/5] Adapt common accessible name utility --- spec/support/matchers/accessibility.rb | 115 +++++++++++++++---------- 1 file changed, 70 insertions(+), 45 deletions(-) diff --git a/spec/support/matchers/accessibility.rb b/spec/support/matchers/accessibility.rb index 356ed713fc6..fadd70f19d3 100644 --- a/spec/support/matchers/accessibility.rb +++ b/spec/support/matchers/accessibility.rb @@ -52,14 +52,10 @@ RSpec::Matchers.define :be_logically_grouped do |name| match do |inputs| + accessible_name = AccessibleName.new(page:) group_names = inputs.map do |input| group = input.ancestor('fieldset,[role=group]') - name = group['aria-label'] - begin - name ||= group.find('legend').text if group.tag_name == 'fieldset' - rescue Capybara::ElementNotFound; end - name ||= aria_labelledby_name(group) - name + accessible_name.computed_name(group) end group_names.compact.uniq == [name] @@ -134,8 +130,38 @@ def descriptors(element) end RSpec::Matchers.define :have_name do |name| + match { |element| AccessibleName.new(page:).computed_name(element) == name } + + failure_message do |element| + <<-STR.squish + Expected element would have computed name "#{name}". + Found #{computed_name(element)}. + STR + end +end + +def expect_page_to_have_no_accessibility_violations(page, validate_markup: true) + expect(page).to be_axe_clean.according_to :section508, :"best-practice", :wcag21aa + expect(page).to have_valid_idrefs + expect(page).to label_required_fields + expect(page).to have_valid_markup if validate_markup +end + +def activate_skip_link + page.evaluate_script('document.activeElement.blur()') + page.active_element.send_keys(:tab) + expect(page.active_element).to have_content(t('shared.skip_link'), wait: 5) + page.active_element.send_keys(:enter) +end + +class AccessibleName + attr_reader :page + # Implements a best effort approximation of W3C Accessible Name and Description Computation 1.2 + # and naming behaviors described in the HTML Accessibility API Mappings 1.0 + # # See: https://www.w3.org/TR/accname-1.2/#mapping_additional_nd_te + # See: https://w3c.github.io/html-aam/ # # 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. @@ -146,6 +172,21 @@ def descriptors(element) # # Example: https://github.com/18F/identity-idp/blob/3c7d3be/spec/support/features/browser_emulation_helper.rb + def initialize(page:) + @page = page + end + + def computed_name(element) + hidden_name(element) || + aria_labelledby_name(element) || + aria_label_name(element) || + referenced_label_name(element) || + ancestor_label_text_name(element) || + fieldset_legend_name(element) + end + + private + def hidden_name(element) # "If the current node is hidden [...] return the empty string." # @@ -160,6 +201,18 @@ def aria_label_name(element) element['aria-label'] 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 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 @@ -203,46 +256,18 @@ def descendent_name(element) page.evaluate_script(visible_text_js, element) end - def computed_name(element) - hidden_name(element) || - aria_labelledby_name(element) || - aria_label_name(element) || - referenced_label_name(element) || - ancestor_label_text_name(element) - end + def fieldset_legend_name(element) + # See: https://w3c.github.io/html-aam/#fieldset-element-accessible-name-computation + return if element.tag_name != 'fieldset' - match { |element| computed_name(element) == name } + # "If the accessible name is still empty, then: if the fieldset element has a child that is a + # legend element, then use the subtree of the first such element." + begin + return element.find('legend').text + rescue Capybara::ElementNotFound; end - failure_message do |element| - <<-STR.squish - Expected element would have computed name "#{name}". - Found #{computed_name(element)}. - STR + # "If the accessible name is still empty, then:, if the fieldset element has a title + # attribute, then use that attribute." + element['title'] end end - -def expect_page_to_have_no_accessibility_violations(page, validate_markup: true) - expect(page).to be_axe_clean.according_to :section508, :"best-practice", :wcag21aa - expect(page).to have_valid_idrefs - expect(page).to label_required_fields - expect(page).to have_valid_markup if validate_markup -end - -def activate_skip_link - page.evaluate_script('document.activeElement.blur()') - page.active_element.send_keys(:tab) - expect(page.active_element).to have_content(t('shared.skip_link'), wait: 5) - page.active_element.send_keys(:enter) -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 From b0878331194d3ffea19ca03f9a2f0cc63233f910 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 31 Oct 2023 10:29:03 -0400 Subject: [PATCH 4/5] Simplify diff --- spec/support/matchers/accessibility.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/support/matchers/accessibility.rb b/spec/support/matchers/accessibility.rb index fadd70f19d3..5823264c74c 100644 --- a/spec/support/matchers/accessibility.rb +++ b/spec/support/matchers/accessibility.rb @@ -195,12 +195,6 @@ def hidden_name(element) '' if element['aria-hidden'] == 'true' 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 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 @@ -213,6 +207,12 @@ def aria_labelledby_name(element) 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 From 807025da784b8bb06220eb301cecabb0a62729be Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 31 Oct 2023 10:29:27 -0400 Subject: [PATCH 5/5] Simplify diff --- spec/support/matchers/accessibility.rb | 28 +++++++++++++------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/spec/support/matchers/accessibility.rb b/spec/support/matchers/accessibility.rb index 5823264c74c..d24c5b6e50d 100644 --- a/spec/support/matchers/accessibility.rb +++ b/spec/support/matchers/accessibility.rb @@ -140,20 +140,6 @@ def descriptors(element) end end -def expect_page_to_have_no_accessibility_violations(page, validate_markup: true) - expect(page).to be_axe_clean.according_to :section508, :"best-practice", :wcag21aa - expect(page).to have_valid_idrefs - expect(page).to label_required_fields - expect(page).to have_valid_markup if validate_markup -end - -def activate_skip_link - page.evaluate_script('document.activeElement.blur()') - page.active_element.send_keys(:tab) - expect(page.active_element).to have_content(t('shared.skip_link'), wait: 5) - page.active_element.send_keys(:enter) -end - class AccessibleName attr_reader :page @@ -271,3 +257,17 @@ def fieldset_legend_name(element) element['title'] end end + +def expect_page_to_have_no_accessibility_violations(page, validate_markup: true) + expect(page).to be_axe_clean.according_to :section508, :"best-practice", :wcag21aa + expect(page).to have_valid_idrefs + expect(page).to label_required_fields + expect(page).to have_valid_markup if validate_markup +end + +def activate_skip_link + page.evaluate_script('document.activeElement.blur()') + page.active_element.send_keys(:tab) + expect(page.active_element).to have_content(t('shared.skip_link'), wait: 5) + page.active_element.send_keys(:enter) +end