Use design system component for USA banner

[aduth]

Why: As a user of assistive technology, I expect that collapsed content is ignored, so that I can navigate to skip content which is expected to be hidden by default, and so that toggle controls (aria-expanded buttons) control content visibility.

Why: As a developer, I want to reuse existing implementations of the design system (USWDS, Login.gov Design System), so that I can take advantage of upstream bug fixes, and avoid the overhead involved with maintaining an ad hoc reimplementation.

Screen Recording, Before, https://secure.login.gov:

Notes:

• The underlying issue is that the .hide class behaves as a visually hidden styling, meaning that it is still navigable by screen readers. The USWDS implementation uses the hidden attribute instead, replicated here.
• Ideally this would use component code from identity-style-guide, rather than reaching to uswds directly. Unfortunately, the distributable of the identity-style-guide package does not (currently) support this sort of opt-in usage, and instead applies its component behaviors globally.

Alternatives or follow-up tasks could include:

• Pushing updates to identity-style-guide to allow it to be used directly
• Keep the custom toggle-banner.js implementation, but update it to operate using the hidden attribute instead of the hide class

[zachmargolis]

A few questions

[zachmargolis]

😂 so much indirection for one behavior! Looking forward to more in the future

[aduth]

😂 so much indirection for one behavior! Looking forward to more in the future

Yep, definitely implemented it this way under the assumption of an opt-in approach to progressively adopting USWDS native component implementations.

It's also the less risky approach, though I do have to wonder what exactly might break if we would instead just load the default script and let it apply to all markup of the page 🤔 Accordions are the likely candidate here, since we use a custom implementation that would likely result in some conflicting / duplicated event handling. This has its own separate problems (LG-3194).

Ideally this code could be dropped altogether, where instead we just load the identity-style-guide script.

[zachmargolis]

querySelector returns only a single instance... do we want to look for all .usa-banner on a page? Or should we call behavior.on(document) instead?

From https://designsystem.digital.gov/documentation/developers/#js-customization:

on: Initialize a component’s JavaScript behavior by passing the root element, such as window.document.

[aduth]

Or should we call behavior.on(document) instead?

This ties into my comment at #3957 (comment) . Yes, ideally we don't have to cherry-pick elements from the page, but instead initialize all components in the entire document. All things considered, maybe it'd be less effort and less error-prone to do this from the start.

[zachmargolis]

Re: that other comment, I don't have strong opinions on cherry-picking or doing the whole page, both seem acceptable to me.

My main worry here is that if we happen to have two .usa-banner on a page, this will ignore the second one, that's the only thing I'd like to see addressed. Otherwise if we want to be clear it's only one, should we throw on an id? #usa-banner etc?

[aduth]

It's a good point, and while it might not be quite as problematic for the banner specifically, once we apply this to other components, it's much more likely to anticipate there being multiple elements matching the selector. So if we decide to keep this, it should definitely support multiple results.

[zachmargolis]

Ok so should we just change to .querySelectorAll(...).forEach()?

[zachmargolis]

LGTM

[zachmargolis]

I just re-read the original description:

Keep the custom toggle-banner.js implementation, but update it to operate using the hidden attribute instead of the hide class

I think this approach would be preferable, if I'm reading the code correctly, USDWS only uses aria-hidden but we got bug reports from a partner doing accessibility testing that IE11 + JAWS doesn't have good support for it, so we switched to using hidden attribute instead. Previous PRs: #3967, #3773

So I think that we can avoid some future a11y bug reports if we make sure to use hidden attribute like that.

[aduth]

if I'm reading the code correctly, USDWS only uses aria-hidden but we got bug reports

From what I can find, this was previously the case, but USWDS does now use the hidden attribute.

See:

• Use hidden attribute and expand by default with no js uswds/uswds#2406
• uswds/uswds@2f4e574#diff-6449513e1e61be79d366e960d8578ffb

This looks to have been included in the 1.6.11 release.

Just to be sure, I also verified in this branch:

[zachmargolis]

From what I can find, this was previously the case, but USWDS does now use the hidden attribute.

Oh perfect, thanks for confirming!

[aduth]

On closer look, I think we can go ahead and bypass this selective opt-in and use the component library script directly. I was concerned that there would be existing use of component classes which would interfere with bringing in the design system code. I specifically recall the accordion being a concern of mine, in addition to this banner code. However, the accordion markup we use doesn't apply the same class names that would be picked up by the design system library, which expects .usa-accordion (we use just .accordion).

For good measure, I did a quick check of all selectors in the design system code, and cross-referenced to our codebase:

$ grep -r -f <(grep -r src/js/components -e '${PREFIX}-' | perl -n -e '/\$\{PREFIX\}-(.+?)[,`]/ && print "usa-" . $1 . "\n"') ../identity-idp/app
../identity-idp/app/views/shared/_banner.html.erb:    <div class="usa-accordion">
../identity-idp/app/views/shared/_banner.html.erb:      <header class="usa-banner__header">
../identity-idp/app/views/shared/_banner.html.erb:            <p class="usa-banner__header-text"><%= FeatureManagement.fake_banner_mode? ?
../identity-idp/app/views/shared/_banner.html.erb:            <p class="usa-banner__header-action" aria-hidden="true"><%= t('shared.banner.how') %></p>
../identity-idp/app/views/shared/_banner.html.erb:          <button class="usa-accordion__button usa-banner__button" aria-expanded="false" aria-controls="gov-banner">
../identity-idp/app/views/shared/_banner.html.erb:      <div class="usa-banner__content usa-accordion__content hide" id="gov-banner">

As expected, the only results are the banner component being updated here, which is exactly what we'd want.

For action items then:

• Remove app/javascript/app/utils/uswds.js
• Switch from uswds to identity-style-guide
• Import identity-style-guide in app/javascript/app/utils/index.js

[aduth]

• Switch from uswds to identity-style-guide

Since identity-style-guide includes its own component implementations, it was important to ensure those too do not conflict. These are duplicate implementations of what exist in this application (accordion close, input strength, etc). However, the style guide implementations also use different classes than what is used in this application (typically classes prefixed with lg-). Thus, while it's not ideal to load multiple implementations for the same components, they shouldn't conflict. Future effort should seek to remove more of the custom implementations in this application, updating to use class names which could be picked up by identity-style-guide.

The updates in bbd1cc1 bring the overall revisions down significantly 🎉 And even better, it means we're now loading the Login.gov Design System component scripts.

[zachmargolis]

This looks way cleaner! LGTM