Skip to content

LG-11278: Group authentication app setup fields with fieldset #9484

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.

Sign up for GitHub

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

Merged
aduth merged 5 commits into from
Nov 1, 2023
Merged

LG-11278: Group authentication app setup fields with fieldset #9484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4de988c
LG-11278: Group TOTP fields with fieldset
aduth Oct 31, 2023
148116a
Add spec assertion for logical grouping
aduth Oct 31, 2023
3bc49c9
Adapt common accessible name utility
aduth Oct 31, 2023
b087833
Simplify diff
aduth Oct 31, 2023
807025d
Simplify diff
aduth Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 42 additions & 37 deletions app/views/users/totp_setup/new.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<%= render PageHeadingComponent.new.with_content(t('headings.totp_setup.new')) %>

<p>
<%= t('forms.totp_setup.totp_intro') %>
<span id="totp-intro"><%= t('forms.totp_setup.totp_intro') %></span>
<%= new_tab_link_to(
t('links.what_is_totp'),
help_center_redirect_path(
Expand All @@ -17,46 +17,51 @@
</p>

<%= 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 %>
<p><%= t('forms.totp_setup.totp_step_1a') %></p>
<%= 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 %>
<div class="text-center">
<%= image_tag @qrcode, size: 240, skip_pipeline: true, alt: t('image_description.totp_qrcode') %>
</div>
<p><%= t('instructions.mfa.authenticator.manual_entry') %></p>
<code class="display-block margin-y-2 font-family-mono padding-y-2 padding-x-1 border-base-lighter border-1px text-bold text-wrap-anywhere" id="qr-code">
<%= @code %>
</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: {
<fieldset aria-labelledby="totp-intro" class="padding-0 border-0 margin-y-4 margin-x-0">
<%= render ProcessListComponent.new(connected: true) do |c| %>
<%= c.with_item(heading: t('forms.totp_setup.totp_step_1')) do %>
<p><%= t('forms.totp_setup.totp_step_1a') %></p>
<%= 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 %>
<div class="text-center">
<%= image_tag @qrcode, size: 240, skip_pipeline: true, alt: t('image_description.totp_qrcode') %>
</div>
<p><%= t('instructions.mfa.authenticator.manual_entry') %></p>
<code class="display-block margin-y-2 font-family-mono padding-y-2 padding-x-1 border-base-lighter border-1px text-bold text-wrap-anywhere" id="qr-code">
<%= @code %>
</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 %>
</fieldset>
<%= f.input(
:remember_device,
as: :boolean,
Expand Down
7 changes: 7 additions & 0 deletions spec/features/users/totp_management_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
73 changes: 60 additions & 13 deletions spec/support/matchers/accessibility.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,24 @@
end
end

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]')
accessible_name.computed_name(group)
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)
Expand Down Expand Up @@ -112,8 +130,24 @@ 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

class AccessibleName
Copy link
Contributor Author

@aduth aduth Oct 31, 2023

Choose a reason for hiding this comment

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

Could / should probably move this elsewhere and add tests for it? Unclear if that makes sense and where would be best for a utility only used in specs.

Copy link
Contributor

@jmdembe jmdembe Oct 31, 2023

Choose a reason for hiding this comment

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

I think this is fine here. How would this differ from the rest of the file?

Copy link
Contributor Author

@aduth aduth Oct 31, 2023

Choose a reason for hiding this comment

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

My main concern is if we'd want to have a convention of one-class-per-file, and files containing a class only containing that class. It'd also be nice to have some test coverage for it, but we don't typically enforce test coverage for test helpers.

Copy link
Contributor

@kevinsmaster5 kevinsmaster5 Oct 31, 2023

Choose a reason for hiding this comment

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

You could break it out as accessibility_be_logically_grouped.rb or something like that.
I wonder if the other classes in that test helper should be split off also?

Copy link
Contributor Author

@aduth aduth Nov 1, 2023

Choose a reason for hiding this comment

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

I think I'll keep this as-is for now, but I do like the idea of maybe creating a new folder within helpers for accessibility helpers, which would be more scalable and let us do one-class/matcher-per-file.

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.
Expand All @@ -124,6 +158,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."
#
Expand Down Expand Up @@ -193,21 +242,19 @@ 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

Expand Down