Fixing flaky navigation handling

[olemartinorg]

Description

In cypress (specifically frontend-test/validation.ts -> "Clicking the error report should focus the correct field") a field is unfocused and navigated to again right afterwards. The render runs every time, but the useEffect() did not - those are batched up to run at the end. This caused the field to not always get focus, making the test flaky. I found this problem when the test failed every ~3-4 runs in #3552, and I suspect it might have been introduced in #3565.

The rest of this PR became a whack-a-mole to try to get tests stable again. As it stands now, the above mentioned test seems very stable to me, but I had to rework the solution here multiple times to get every test green.

My findings:

• Relying on useEffect() alone doesn't always cut it. We want to schedule focusing on the correct element for after the component has rendered, but in practice this was difficult. As mentioned above, when deps don't change, the effect will not run again.
• When calling setSearchParams() (from useSearchParams() from react-router) it seems to also navigate you back to the previous page if you called it after a navigation but before your render call got that new state.
• React router location (from useLocation() etc) and the window.location.hash are often not in sync during navigation. Because it will take some time before react-router gives you the new state, the code could end up trying to focus a component when it shouldn't, or clean up search params (calling setSearchParams()) when that would do more harm than good.
• If you set the search param to focus a component and that component doesn't exist, the search param will just linger. If you then navigated to another page where that component was present, you'd end up getting focus (although way too late). For this reason I added code to clean up those search params on page navigation.
• Using a setTimeout() to defer the focusing until after render was much more stable, but caused some new problems. For instance, when a component unmounts we want to clean up that ongoing process and stop the focusing effort.

Related Issue(s)

• closes #{issue number}

Verification/QA

• Manual functionality testing
  • I have tested these changes manually
  • Creator of the original issue (or service owner) has been contacted for manual testing (or will be contacted when released in alpha)
  • No testing done/necessary
• Automated tests
  • Unit test(s) have been added/updated
  • Cypress E2E test(s) have been added/updated
  • No automatic tests are needed here (no functional changes/additions)
  • I want someone to help me make some tests
• UU/WCAG (follow these guidelines until we have our own)
  • I have tested with a screen reader/keyboard navigation/automated wcag validator
  • No testing done/necessary (no DOM/visual changes)
  • I want someone to help me perform accessibility testing
• User documentation @ altinn-studio-docs
  • Has been added/updated
  • No functionality has been changed/added, so no documentation is needed
  • I will do that later/have created an issue
• Support in Altinn Studio
  • Issue(s) created for support in Studio
  • This change/feature does not require any changes to Altinn Studio
• Sprint board
  • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board
  • I don't have permissions to do that, please help me out
• Labels
  • I have added a kind/* and backport* label to this PR for proper release notes grouping
  • I don't have permissions to add labels, please help me out

[cammiida]

🙌

[sonarqubecloud]

Quality Gate failed

Failed conditions
38.3% Coverage on New Code (required ≥ 45%)
40.87% Condition Coverage on New Code (required ≥ 45%)

See analysis details on SonarQube Cloud

[cammiida]

Veldig bra jobba! 👏 tror kanskje ikke alt her er helt nødvendig, så har prøvd å forenkle bittelitt. Approver igjen med forbehold om at i hvert fall kommentaren for GenericComponent og sletting av search params i useNavigatePage blir tatt hensyn til 😄

[cammiida]

Tror ikke det skal være nødvendig å synce forrige search params med de som blir sendt inn i funksjonen siden navigate ikke tar dem med? Med mindre man spesifiserer dem ofc. Burde man ikke bare bruke de som blir sendt inn gjennom options? Altså fjerne 282-286 og sette 289-291 til

if (isStatelessApp) {
  const url = `/${page}`
  navigate(url, options, { replace }, { targetLocation: url, callback: () => focusMainContent(options) });

Samme under da, I guess.

[cammiida]

Nice! Lurer på om noe lignende også burde gjøres i NavigationBarComponent?

[cammiida]

Fjerner litt duplisering av logikk?

const newSearchParams = new URLSearchParams(searchParams);
newSearchParams.set(SearchParams.FocusComponentId, indexedId);
const errorBindingKey = options?.error?.['bindingKey'];
if (errorBindingKey) {
  newSearchParams.set(SearchParams.FocusErrorBinding, errorBindingKey);
}

if (targetPage && targetPage !== currentPageId) {
  await navigateToPage(targetPage, {
    ...options?.pageNavOptions,
    shouldFocusComponent: true,
    focusComponentId: indexedId,
    searchParams: newSearchParams,
    replace:
      !!newSearchParams.get(SearchParams.FocusComponentId) || !!newSearchParams.get(SearchParams.ExitSubform),
  });
} else {
  setSearchParams(newSearchParams);
}

[cammiida]

navigateTo sier ikke så mye om hva det er og høres ut som en funskjon 🙈 er det en funksjon, en konst, eller noe helt annet? Hva men noe ala componentToNavigateTo. Bittelitt langt, men eksplisitt i hvert fall, med mindre du har andre forslag 😄

[cammiida]

setTimeout her tror jeg ikke trengs, og tror resten her burde inn i en useEffect? Kanskje, men er åpen for at det ikke må det 😄 Det her funker i hvert fall over 100 runs for meg

function useHandleFocusComponent(nodeId: string, containerDivRef: React.RefObject<HTMLDivElement | null>) {
  const [searchParams, setSearchParams] = useSearchParams();
  const indexedId = searchParams.get(SearchParams.FocusComponentId);
  const errorBinding = searchParams.get(SearchParams.FocusErrorBinding);
  const isNavigating = useNavigation().state !== 'idle';

  const location = useLocation();
  const abortController = useRef(new AbortController());

  const hashWas = window.location.hash;
  const locationIsUpdated = hashWas.endsWith(location.search);
  const shouldFocus = indexedId && indexedId == nodeId && !isNavigating && locationIsUpdated;

  useEffect(() => {
    async function focusComponent() {
      try {
        const div = await waitForRef(containerDivRef, 2000, abortController.current.signal);
        requestAnimationFrame(() => {
          !abortController.current.signal.aborted && div.scrollIntoView({ behavior: 'instant' });
        });

        const field = findElementToFocus(div, errorBinding);
        if (field && !abortController.current.signal.aborted) {
          field.focus();
        }
      } catch (error) {
        if (!abortController.current.signal.aborted) {
          console.error('Failed to focus component', nodeId, error);
        }
      } finally {
        if (!abortController.current.signal.aborted && hashWas === window.location.hash) {
          // Only cleanup when hash is the same as what it was during render. Navigation might have occurred, especially
          // in Cypress tests where state changes will happen rapidly. These search params are cleaned up in
          // useNavigatePage() automatically, so it shouldn't be a problem if the page has been changed. If something
          // else happens, we'll re-render and get a new chance to clean up later.
          cleanupQuery(searchParams, setSearchParams);
        }
      }
    }
    shouldFocus && focusComponent();
  }, [containerDivRef, errorBinding, hashWas, nodeId, searchParams, setSearchParams, shouldFocus]);

  useEffect(
    () => () => {
      // Abort on unmount so that we do not keep trying to focus this component
      abortController.current.abort();
    },
    [abortController],
  );
}

[cammiida]

Suggested change

	const exitSubform = options?.searchParams?.has(SearchParams.ExitSubform, 'true') ?? false;
	const shouldExitSubform = options?.searchParams?.has(SearchParams.ExitSubform, 'true') ?? false;