LG-7949: Change behavior of "skip to content link" on account page by jc-gsa · Pull Request #7310 · 18F/identity-idp

jc-gsa · 2022-11-07T22:23:33Z

🎫 Ticket

🛠 Summary of changes

This PR changes the behavior of the accessibility link Skip to main content on the page /account.

The layout found at app/views/layouts/account_side_nav.html.erb overrides portions of the base layout. On the account page, the location of the tag main is changed so that the accessibility link Skip to main content jumps to the desired location.

Suggestions and improvements to these two conflicting layouts are welcome.

This reverts commit 07e9eeb.

changelog: Improvements, Accessibility, Improve accessibility link (LG-7949)

zachmargolis · 2022-11-07T23:28:09Z

app/views/layouts/base.html.erb


  <% if IdentityConfig.store.newrelic_browser_key.present? && IdentityConfig.store.newrelic_browser_app_id.present? %>
-  <%= render 'shared/newrelic/browser_instrumentation' %>
+    <%= render 'shared/newrelic/browser_instrumentation' %>


❤️ thanks for fixing this indentation while we're here!

zachmargolis · 2022-11-07T23:28:42Z

app/views/layouts/base.html.erb

+  <%= content_tag(local_assigns[:custom_main].present? ? 'div' : 'main', { class: 'site-wrap bg-primary-lighter', id: local_assigns[:custom_main].present? ? 'main-wrapper' : 'main-content' }) do %>
+      <div class="grid-container padding-y-4 tablet:padding-y-8 <%= local_assigns[:disable_card].present? ? '' : 'card' %>">
+        <%= yield(:pre_flash_content) if content_for?(:pre_flash_content) %>
+        <%= render FlashComponent.new(flash: flash) %>
+        <%= content_for?(:content) ? yield(:content) : yield %>
+      </div>
+  <% end %>


looks like this has an indent by 4 instead of by 2? also maybe let's put the content_tag stuff on separate lines so it's easier to follow?

Suggested change

<%= content_tag(local_assigns[:custom_main].present? ? 'div' : 'main', { class: 'site-wrap bg-primary-lighter', id: local_assigns[:custom_main].present? ? 'main-wrapper' : 'main-content' }) do %>

<div class="grid-container padding-y-4 tablet:padding-y-8 <%= local_assigns[:disable_card].present? ? '' : 'card' %>">

<%= yield(:pre_flash_content) if content_for?(:pre_flash_content) %>

<%= render FlashComponent.new(flash: flash) %>

<%= content_for?(:content) ? yield(:content) : yield %>

</div>

<% end %>

<%= content_tag(

local_assigns[:custom_main].present? ? 'div' : 'main',

class: 'site-wrap bg-primary-lighter',

id: local_assigns[:custom_main].present? ? 'main-wrapper' : 'main-content'

) do %>

<div class="grid-container padding-y-4 tablet:padding-y-8 <%= local_assigns[:disable_card].present? ? '' : 'card' %>">

<%= yield(:pre_flash_content) if content_for?(:pre_flash_content) %>

<%= render FlashComponent.new(flash: flash) %>

<%= content_for?(:content) ? yield(:content) : yield %>

</div>

<% end %>

zachmargolis · 2022-11-07T23:31:48Z

app/views/layouts/account_side_nav.html.erb


 <%= javascript_packs_tag_once('navigation') %>
-<%= render template: 'layouts/base', locals: { disable_card: true } %>
+<%= render template: 'layouts/base', locals: { disable_card: true, custom_main: true } %>


what if we named the argument use_main to indicate that the template should not have a <main> tag? (and flip the corresponding logic)

Suggested change

<%= render template: 'layouts/base', locals: { disable_card: true, custom_main: true } %>

<%= render template: 'layouts/base', locals: { disable_card: true, use_main: false } %>

another idea: user_main_tag: false ?

I've just realized a different approach or more changes are necessary, as the rule https://dequeuniversity.com/rules/axe/4.3/region?application=axeAPI is violated.

A simple change would be to change how the line <%= link_to t('shared.skip_link'), '#main-content', class: 'usa-skipnav' %> works in the base layout. The id of #main-content could be changed by the layout file identity-idp/app/views/layouts/account_side_nav.html.erb.

Otherwise a bigger restructure of the layouts and accounts page seems to be necessary. The partial identity-idp/app/views/accounts/_side_nav.html.erb functions as a second nav. But this partial is nested [edit] outside of landmark tags in the current change.

For the accessibility build failure, it looks like the only content which is not within a landmark is the top "Access your government benefits ..." content. It seems like it would be appropriate to mark this up as an <aside>, since it's content which complements the main content?

i.e. changing this element from <div> to <aside>:

identity-idp/app/views/layouts/account_side_nav.html.erb

Line 10 in 92c73f1

<div class="display-none tablet:display-flex grid-row grid-gap margin-bottom-4">

@zachmargolis I have made your suggested changes. But I find the name user_main_tag: false to be confusing, and I find the boolean check for false more difficult to read.

aduth · 2022-11-08T14:27:05Z

app/views/layouts/base.html.erb

-    </div>
-  </main>
+
+  <%= content_tag(local_assigns[:custom_main].present? ? 'div' : 'main', { class: 'site-wrap bg-primary-lighter', id: local_assigns[:custom_main].present? ? 'main-wrapper' : 'main-content' }) do %>


Since we're not using the main-wrapper ID for anything, could we avoid assigning the id for the custom main case?

Suggested change

<%= content_tag(local_assigns[:custom_main].present? ? 'div' : 'main', { class: 'site-wrap bg-primary-lighter', id: local_assigns[:custom_main].present? ? 'main-wrapper' : 'main-content' }) do %>

<%= content_tag(local_assigns[:custom_main] ? 'div' : 'main', { class: 'site-wrap bg-primary-lighter', id: local_assigns[:custom_main] ? nil : 'main-content' }) do %>

jmdembe · 2022-11-08T16:59:11Z

As it stands, the skip link is only going to the main account content. We need to have it so that the user has the option to either skip to the main navigation, or the account preferences

aduth · 2022-11-08T17:43:35Z

As it stands, the skip link is only going to the main account content. We need to have it so that the user has the option to either skip to the main navigation, or the account preferences

My understanding is that the ticket being addressed here is to resolve a bug with how "Skip to main content" operates, which currently does not work as expected, as it should bypass the repeated navigation elements. We could explore additional skip links, but this seems like a separate task, and may require more exploration to whether it helps more than it hurts.

jmdembe · 2022-11-08T19:30:31Z

As it stands, the skip link is only going to the main account content. We need to have it so that the user has the option to either skip to the main navigation, or the account preferences

My understanding is that the ticket being addressed here is to resolve a bug with how "Skip to main content" operates, which currently does not work as expected, as it should bypass the repeated navigation elements. We could explore additional skip links, but this seems like a separate task, and may require more exploration to whether it helps more than it hurts.

I can see the help/hurt concern. I am thinking of a case where someone would want to navigate to that menu and the only way to get to it at this point is to back tab through the first element, which is not a good experience.

I would feel okay with this fix as it stands, but I would like a further discussion with IRS/other accessibility folks on the team to assess if my concern is valid.

jc-gsa · 2022-11-08T20:35:49Z

@jmdembe I raised your concerns when I started the ticket and again in the group meeting. As I understand it, others want the accessibility link to point to the "right hand side" of the main screen. If these changes are not desired, then the PR can be dropped.

aduth

This LGTM as addressing the bug described in the ticket 👍

Separately, it seems like it'd be good to sync as a team on whether there's any action regarding navigating to page elements other than the main content. Can we discuss after standup tomorrow?

jmdembe · 2022-11-08T21:02:54Z

@jmdembe I raised your concerns when I started the ticket and again in the group meeting. As I understand it, others want the accessibility link to point to the "right hand side" of the main screen. If these changes are not desired, then the PR can be dropped.

I am going to seek further clarification so that everyone is on the same page

jc-gsa added 2 commits November 4, 2022 09:11

Add additional skip link (WIP)

07e9eeb

Revert "Add additional skip link (WIP)"

a4ad9cd

This reverts commit 07e9eeb.

jc-gsa added the status - do not merge label Nov 7, 2022

jc-gsa requested a review from a team November 7, 2022 22:23

Add override of main tag in base layout from override layout

3fb5bc8

changelog: Improvements, Accessibility, Improve accessibility link (LG-7949)

jc-gsa force-pushed the LG-7949-skip-to-content branch from d6a77b3 to 3fb5bc8 Compare November 7, 2022 23:07

zachmargolis reviewed Nov 7, 2022

View reviewed changes

aduth reviewed Nov 8, 2022

View reviewed changes

jc-gsa added 2 commits November 8, 2022 13:55

Add suggestions

f627315

Merge branch 'main' into LG-7949-skip-to-content

46008d6

aduth approved these changes Nov 8, 2022

View reviewed changes

jc-gsa removed the status - do not merge label Nov 9, 2022

jc-gsa merged commit 58ef8e2 into main Nov 9, 2022

jc-gsa deleted the LG-7949-skip-to-content branch November 9, 2022 22:20

mitchellhenke mentioned this pull request Nov 10, 2022

Deploy RC 226 to Production #7328

Merged

	<%= render template: 'layouts/base', locals: { disable_card: true, custom_main: true } %>
	<%= render template: 'layouts/base', locals: { disable_card: true, use_main: false } %>

	<%= content_tag(local_assigns[:custom_main].present? ? 'div' : 'main', { class: 'site-wrap bg-primary-lighter', id: local_assigns[:custom_main].present? ? 'main-wrapper' : 'main-content' }) do %>
	<%= content_tag(local_assigns[:custom_main] ? 'div' : 'main', { class: 'site-wrap bg-primary-lighter', id: local_assigns[:custom_main] ? nil : 'main-content' }) do %>

Conversation

jc-gsa commented Nov 7, 2022

🎫 Ticket

🛠 Summary of changes

Uh oh!

zachmargolis Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

jc-gsa Nov 7, 2022 • 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!

aduth Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

jc-gsa Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

aduth Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

jmdembe commented Nov 8, 2022

Uh oh!

aduth commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmdembe commented Nov 8, 2022

Uh oh!

jc-gsa commented Nov 8, 2022

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

jmdembe commented Nov 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jc-gsa Nov 7, 2022 •

edited

Loading

aduth commented Nov 8, 2022 •

edited

Loading