feat(react): remove focus-trap-react replacing with internally managed focus traps

[scurker]

closes #1703

Any existing components that were utilizing focus-trap-react should now be using the internal useFocusTrap hook. Some small refactors had to take place to accommodate the new hook, but otherwise the functionality should be identical.

[aws-amplify-us-east-1]

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1730.d15792l1n26ww3.amplifyapp.com

[schne324]

nit - this use of setTimeout could probably benefit from a comment (I assume this is to wait for the dialog to be rendered, if so there may be a way to avoid setTimeout use)

[scurker]

I agree, but I wish I remembered why as it's been like this since it's initial implementation:

cauldron/packages/react/src/components/Dialog/index.tsx

Lines 65 to 66 in eb5def1

	this.attachEventListeners();
	this.attachIsolator(() => setTimeout(this.focusHeading));

I'm taking an educated guess that it's queuing the focus to be on the next tick to prevent focus conflicts. We can add that as a comment if you feel that's useful?

[schne324]

Looks good to me. I left a couple of nits. I'm also wondering if we should document anything about not using iframes within things that need focus traps as it could mess with the focus trap code (if, for example, the first tab stop in a dialog is within an iframe)

[scurker]

Looks good to me. I left a couple of nits. I'm also wondering if we should document anything about not using iframes within things that need focus traps as it could mess with the focus trap code (if, for example, the first tab stop in a dialog is within an iframe)

I'm not following 100%.

I just tested an alert with an iframe inside of it and it works as expected (since we focus the header by default) and focus works exactly as you would expect. Is there a particular edge case scenario you would be looking to cover?

[schne324]

I'm not following 100%.

I just tested an alert with an iframe inside of it and it works as expected (since we focus the header by default) and focus works exactly as you would expect. Is there a particular edge case scenario you would be looking to cover?

I was making an assumption that funky things could happen because we have this code:

    const focusableElements = Array.from(
      targetElement?.querySelectorAll(focusable) || []
    ) as HTMLElement[];

and any focusable elements that are within an iframe would not be captured in that focusableElements array.

For example: https://github.com/dequelabs/cauldron/pull/1730/files#diff-8527069e35f9293283f4a8860691c088d4423c2addf12b5670303aceeff92c96R111-R117

   if (focusableElements.length && eventTarget === startGuard) {
      focusableElements.reverse()[0]?.focus();
      return;
    } else if (focusableElements.length && eventTarget === endGuard) {
      focusableElements[0]?.focus();
      return;
    }

the first and last elements in focusable could be, in reality, within an iframe but in this array they are not captured at all

[scurker]

I was making an assumption that funky things could happen because we have this code:

[...]

and any focusable elements that are within an iframe would not be captured in that focusableElements array.

[...]

the first and last elements in focusable could be, in reality, within an iframe but in this array they are not captured at all

Ah right. With that code there, the focusable elements would only really be applicable for the first or last element (to determine where the next focus needs to go). In my test case I still had surrounding elements within the iframe and it works as expected. So this would be a bit of an edge case where the last or first focusable element was within an iframe.

I feel that's an acceptable outcome for us, but I'm not sure how we want to take note of it. The majoritive use case would be working within a single document context.