LG-8682 Bypass Blocks - aka make skip to main content work by JackRyan1989 · Pull Request #7769 · 18F/identity-idp

JackRyan1989 · 2023-02-03T21:04:43Z

🎫 Ticket

🛠 Summary of changes

One pages within the document upload/authorization flow, selecting the 'Skip to main content' link does not actually work. It interrupts the navigation as managed by the React flow state machine. This is a bit of a triage: we load some JS that prevents the default functionality of the link, and instead manually focuses the main content section and scrolls to it.

I did not see a spec file for the document-capture.tsx file, so I didn't create one? But am happy to either update an existing test spec or create a new one for this particular component.

📜 Testing Plan

Confirm that 'Skip to Main Content' links display on all pages
Confirm that functionality of all 'Skip to Main Content' links works (focuses main content section, scrolls section into view)
Confirm that no errors are logged to the console
Confirm that the accessibility of the component is not affected: still tabbable/reachable by other screen reader key combos, read by screen reader correctly
Confirm that the accessibility of the page is not otherwise affected

👀 No Screenshots for you!

…PAT concerns around skip to main content link.

aduth · 2023-02-03T21:10:20Z

Unaware this was being worked on, I was discussing this bug earlier with @eric-gade , and we came up with a similar conclusion that the key thing would be to prevent the default behavior of the skip link. I sorta feel this should be the default behavior across the board, even to the extent that I was surprised the base implementation of the skiplink doesn't already do this. I was suggesting we add this to the app-wide app/javascript/packs/application.ts:

document
  .querySelector('.usa-skipnav')
  ?.addEventListener('click', (event) => event.preventDefault());

Do you have thoughts on that approach?

The focusing and scrolling should already be happening as a result of the base implementation in the U.S. Web Design System: https://github.com/uswds/uswds/blob/v2.13.3/src/js/components/skipnav.js#L20

zachmargolis · 2023-02-03T21:12:13Z

app/javascript/packages/document-capture/context/hijack-skip-nav.tsx

+ * @param - null
+ *
+ * @return - null
+ */
+


I'd ditch the null documentation?

Suggested change

* @param - null

*

* @return - null

*/

*/

zachmargolis · 2023-02-03T21:12:48Z

app/javascript/packages/document-capture/components/document-capture.tsx

  const { inPersonURL, arcgisSearchEnabled } = useContext(InPersonContext);
  useDidUpdateEffect(onStepChange, [stepName]);
  useEffect(() => {
+    hijackSkipNav();


does this need any guard to make sure it's not called twice? or should the function itself check to see that there's not already a listener set?

To make sure I understand, is this what your concerned about? Memory Issues

If it is, I think it would solve the issue to create a function like:

function setFocus(event) { event.preventDefault(); mainContent?.focus(); mainContent?.scrollIntoView(); }

And then have the addEventListener work like skipNavLink?.addEventListener('click', setFocus);

Does that seem like a reasonable solution?

@JackRyan1989 The return value to the useEffect callback should be another callback that removes the event listener. Additionally, you may want hijackSkipNav to track the times it's been called/cleaned up to avoid redundant attachment (and premature detachment) of event listeners.

yeah either solution works but I definitely think the unmount solution would be the cleanest

JackRyan1989 · 2023-02-03T21:20:12Z

Unaware this was being worked on, I was discussing this bug earlier with @eric-gade , and we came up with a similar conclusion that the key thing would be to prevent the default behavior of the skip link. I sorta feel this should be the default behavior across the board, even to the extent that I was surprised the base implementation of the skiplink doesn't already do this. I was suggesting we add this to the app-wide app/javascript/packs/application.ts:
document
  .querySelector('.usa-skipnav')
  ?.addEventListener('click', (event) => event.preventDefault());
Do you have thoughts on that approach?

The focusing and scrolling should already be happening as a result of the base implementation in the U.S. Web Design System: https://github.com/uswds/uswds/blob/v2.13.3/src/js/components/skipnav.js#L20

Personally I don't have an opinion about implementing this site wide. The motivation for this particular change is that it's affecting keyboard accessibility, is the functionality of this link causing issues elsewhere in the application?

And interestingly (and at least in Firefox) the autoscroll aspect of .focus() was not happening, even with setting preventScroll: false, hence line 20.

aduth · 2023-02-03T21:42:11Z

I think my main concern is if we have the link working different ways on different pages, it's hard to track, and likely to cause some confusion down the line. It feels like this ought to be the default behavior of the skip link, or at least if the USWDS implementation is already managing most of the programmatic focus shifting, the native behavior of updating the page location isn't necessary. I guess if we were also relying on the native behavior to move the viewport due to inconsistencies with the browser, we could pull that into the global event handler as well?

JackRyan1989 · 2023-02-03T21:46:35Z

I think my main concern is if we have the link working different ways on different pages, it's hard to track, and likely to cause some confusion down the line. It feels like this ought to be the default behavior of the skip link, or at least if the USWDS implementation is already managing most of the programmatic focus shifting, the native behavior of updating the page location isn't necessary. I guess if we were also relying on the native behavior to move the viewport due to inconsistencies with the browser, we could pull that into the global event handler as well?

Oh yeah gotcha. Yeah that seems like a good idea to me.

NavaTim · 2023-02-03T22:37:38Z

app/javascript/packages/document-capture/context/hijack-skip-nav.tsx

+export default function hijackSkipNav(): void {
+  const skipNavLink = document.querySelector<HTMLFormElement>('.usa-skipnav');
+  const mainContent = document.querySelector<HTMLFormElement>('#main-content');
+
+  skipNavLink?.addEventListener('click', (e) => {
+    e.preventDefault();
+    mainContent?.focus();
+    mainContent?.scrollIntoView();
+  });
+}


Here's an example of the kind of logic I mentioned.

Suggested change

export default function hijackSkipNav(): void {

const skipNavLink = document.querySelector<HTMLFormElement>('.usa-skipnav');

const mainContent = document.querySelector<HTMLFormElement>('#main-content');

skipNavLink?.addEventListener('click', (e) => {

e.preventDefault();

mainContent?.focus();

mainContent?.scrollIntoView();

});

}

// Function has same identity between calls, so can't be duplicated

function hijackListener(e: Event) {

e.preventDefault();

// Get the main content on click so we don't need to worry about it being removed;

// this is an infrequent operation, so a repeated DOM search is acceptable

const mainContent = document.querySelector<HTMLFormElement>('#main-content');

mainContent?.focus();

mainContent?.scrollIntoView();

}

// Keep track of the listener calls so we don't cleanup the listener until it's appropriate

// Listeners are no longer relevant after there's no reference to the element, so WeakMap is appropriate

const listenerCounts = new WeakMap<HTMLFormElement,number>();

export default function hijackSkipNav(): () => void {

const skipNavLink = document.querySelector<HTMLFormElement>('.usa-skipnav');

skipNavLink?.addEventListener('click', hijackListener);

const prevCnt = listenerCounts.get(skipNavLink) || 0;

listenerCounts.set(skipNavLink, prevCnt + 1);

// Return a function that can be passed as the useEffect return value for listener cleanup

return () => {

// Only cleanup if this is the last usage; otherwise decrement the listener counter

const cnt = listenerCounts.get(skipNavLink);

if (cnt === 1) {

listenerCounts.delete(skipNavLink);

skipNavLink?.removeEventListener('click', hijackListener);

} else if (cnt > 1) {

listenerCounts.set(skipNavLink, cnt - 1);

}

};

}

Could be overkill since this should only be happening once per page.

Ok, this helps clarify some things! I think first things first we should decide if we're following @aduth's advice of implementing this as the default behavior, if so then no need to wrangle with the useEffect block. @NavaTim do you have an opinion about making this the default behavior?

If @aduth said to make this the default behavior, then I'm good with that.

eric-gade · 2023-02-06T14:49:08Z

Hi @JackRyan1989 @NavaTim -- as he mentioned, @aduth and I were discussing this Friday morning, and it also relates to an accessibility ticket I've picked up (LG-8786).

If you are already going to implement the application-wide behavior then I do not want to duplicate efforts -- I've haven't had a chance to work on this issue any more until this morning, but will hold off until I hear from you all.

In the meantime, @aduth suggested the possibility of posting to the uswds repo about a feature request for the skip-to-content module and seeing whether or not they can implement something similar. I'll take charge of making that post.

EDIT: I've posted to uswds here.

JackRyan1989 · 2023-02-06T17:17:55Z

Hi @JackRyan1989 @NavaTim -- as he mentioned, @aduth and I were discussing this Friday morning, and it also relates to an accessibility ticket I've picked up (LG-8786).

If you are already going to implement the application-wide behavior then I do not want to duplicate efforts -- I've haven't had a chance to work on this issue any more until this morning, but will hold off until I hear from you all.

In the meantime, @aduth suggested the possibility of posting to the uswds repo about a feature request for the skip-to-content module and seeing whether or not they can implement something similar. I'll take charge of making that post.

EDIT: I've posted to uswds here.

Hey Eric,

Sounds good. Seems like the most straightforward option is to make this change the default behavior. I'll go ahead and handle that with this PR and thanks for reaching out to USWDS.

JackRyan1989 · 2023-02-06T18:16:17Z

I think my main concern is if we have the link working different ways on different pages, it's hard to track, and likely to cause some confusion down the line. It feels like this ought to be the default behavior of the skip link, or at least if the USWDS implementation is already managing most of the programmatic focus shifting, the native behavior of updating the page location isn't necessary. I guess if we were also relying on the native behavior to move the viewport due to inconsistencies with the browser, we could pull that into the global event handler as well?

@aduth Thanks for the guidance here. Changes have been made. I confirmed that the scrollIntoView and focus method calls are not necessary on Chrome, but I'm still experiencing the issue where the main-content section doesn't scroll into view on Firefox. Happy to remove if you think it would be a better idea.

aduth

LGTM 👍

aduth · 2023-02-07T17:52:36Z

app/javascript/packs/application.ts

+const mainContent = document.getElementById('main-content');
+document.querySelector('.usa-skipnav')?.addEventListener('click', (event) => {
+  event.preventDefault();
+  mainContent?.focus();


Is the focus necessary, if this is already happening in the base USWDS implementation?

https://github.com/uswds/uswds/blob/e75f3f347170ebfe472bba9719935a4f5ba38b69/packages/usa-skipnav/src/index.js#L20

Suggested change

mainContent?.focus();

No it isn't. Good call! Thanks @aduth

changelog: User-facing improvements, In-person proofing, Addressing V…

15a98dc

…PAT concerns around skip to main content link.

JackRyan1989 requested review from a team and racingspider February 3, 2023 21:04

zachmargolis reviewed Feb 3, 2023

View reviewed changes

NavaTim reviewed Feb 3, 2023

View reviewed changes

eric-gade mentioned this pull request Feb 6, 2023

USWDS - Feature: Use JS to "scroll to content" in skipnav.js uswds/uswds#5133

Open

2 tasks

Removing doc-auth specific instead making global.

7b88bc3

aduth approved these changes Feb 7, 2023

View reviewed changes

Remove unneccessary focus()

483649e

JackRyan1989 merged commit 327f67d into main Feb 7, 2023

JackRyan1989 deleted the jryan/lg-8682-fix-skip-to-content-links branch February 7, 2023 18:30

aduth mentioned this pull request Feb 9, 2023

Deploy RC 251 to Prod #7799

Merged

aduth mentioned this pull request Jan 30, 2024

Silently ignore invalid params for FormSteps #10005

Merged

-export default function hijackSkipNav(): void {
-  const skipNavLink = document.querySelector<HTMLFormElement>('.usa-skipnav');
-  const mainContent = document.querySelector<HTMLFormElement>('#main-content');
-  skipNavLink?.addEventListener('click', (e) => {
-    e.preventDefault();
-    mainContent?.focus();
-    mainContent?.scrollIntoView();
-  });
-}
+// Function has same identity between calls, so can't be duplicated
+function hijackListener(e: Event) {
+  e.preventDefault();
+  // Get the main content on click so we don't need to worry about it being removed;
+  // this is an infrequent operation, so a repeated DOM search is acceptable
+  const mainContent = document.querySelector<HTMLFormElement>('#main-content');
+  mainContent?.focus();
+  mainContent?.scrollIntoView();
+}
+// Keep track of the listener calls so we don't cleanup the listener until it's appropriate
+// Listeners are no longer relevant after there's no reference to the element, so WeakMap is appropriate
+const listenerCounts = new WeakMap<HTMLFormElement,number>();
+export default function hijackSkipNav(): () => void {
+  const skipNavLink = document.querySelector<HTMLFormElement>('.usa-skipnav');
+  skipNavLink?.addEventListener('click', hijackListener);
+  const prevCnt = listenerCounts.get(skipNavLink) || 0;
+  listenerCounts.set(skipNavLink, prevCnt + 1);
+  // Return a function that can be passed as the useEffect return value for listener cleanup
+  return () => {
+    // Only cleanup if this is the last usage; otherwise decrement the listener counter
+    const cnt = listenerCounts.get(skipNavLink);
+    if (cnt === 1) {
+      listenerCounts.delete(skipNavLink);
+      skipNavLink?.removeEventListener('click', hijackListener);
+    } else if (cnt > 1) {
+      listenerCounts.set(skipNavLink, cnt - 1);
+    }
+  };
+}

Conversation

JackRyan1989 commented Feb 3, 2023

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 No Screenshots for you!

Uh oh!

aduth commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NavaTim Feb 3, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

JackRyan1989 commented Feb 3, 2023

Uh oh!

aduth commented Feb 3, 2023

Uh oh!

JackRyan1989 commented Feb 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-gade commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackRyan1989 commented Feb 6, 2023

Uh oh!

JackRyan1989 commented Feb 6, 2023

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aduth commented Feb 3, 2023 •

edited

Loading

NavaTim Feb 3, 2023 •

edited

Loading

eric-gade commented Feb 6, 2023 •

edited

Loading