fix(modal): prevent focus change when current focus is within modal

[dancormier]

https://stackoverflow.atlassian.net/browse/STACKS-412

This PR attempts to fix the issue of the modal controller attempting to set focus when focus is already set on an element within the modal by checking to see if the currently focused element is contained within the controller and only setting focus if it's not.

Note
This is a work in progress since tests are currently failing (and would probably benefit from a little cleanup since there's some redundancy among tests).

Current state of tests

I decided to timebox this fix and I couldn't figure out why these tests are failing only in Chrome and Firefox but not Webkit (with one exception). It looks like the modal is not getting the aria-hidden value changed from true to false when show is triggered. This explains why the Chrome/Firefox tests are failing, but it doesn't explain why Webkit is (mostly) passing 😕

@giamir I was hoping you could take a look to see if there's anything obvious to explain why 1) the aria-hidden value is not set as expected and 2) why tests would pass in Webkit. If you don't see anything obvious, please don't stress over it too much. I figure I'll return to this soon with fresh eyes, but I figured I'd ask for your help since I might just be too close to this.

Update Thu Nov 2, 2023

When running the tests now, all tests fail in Chromium and Firefox only with this error:

Error: Error invoking action "click->s-modal#toggle" (:0)

Two of the five tests also fail in Webkit. Both of these tests check focus on specific elements but it seems that the focus is always left on the button that triggers showing the modal.

❌ modal > should focus the element with `data-s-modal-target"initialFocus"`
      AssertionError: expected <button class="s-btn s-btn__outlined" data-action="s-modal#toggle" data-target="#launch-modal-base" data-testid="trigger">
                  Launch example modal
              </button> to equal <input type="text" data-testid="input-2" data-s-modal-target="initialFocus">
        at lib/components/modal/modal.test.ts:100:54

 ❌ modal > should focus the first focusable element when modal is shown
      AssertionError: expected <button class="s-btn s-btn__outlined" data-action="s-modal#toggle" data-target="#launch-modal-base" data-testid="trigger">
                  Launch example modal
              </button> to equal <input type="text" data-testid="input-1">
        at lib/components/modal/modal.test.ts:117:54

Webkit: |██████████████████████████████| 1/1 test files |
 3 passed, 2 failed

[netlify]

✅ Deploy Preview for stacks ready!

Name	Link
🔨 Latest commit	d6c832b
🔍 Latest deploy log	https://app.netlify.com/sites/stacks/deploys/6549210b89b6a300083772ac
😎 Deploy Preview	https://deploy-preview-1549--stacks.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[dancormier]

@giamir thank you for debugging the test failures for me and taking a moment to perform some clean up. I'm going to merge this in but I'd like to make sure I understand what resolved the failures.

The causes of failures (as I understand)

• the private member definitions needed to be changed to declare readonly so the property can be typed properly without being overridden
• we need to waitFor the element before trying to set focus

Questions

• I noticed we now check .to.have.focus instead of document.activeElement. This is just has no material impact but is just a syntactical change, correct?
• I noticed // for the unit tests we need to keep animations enabled and I'm curious to hear a little more on why we need animations. I'm not opposed to it but just interested in the reason.

[giamir]

I noticed we now check .to.have.focus instead of document.activeElement. This is just has no material impact but is just a syntactical change, correct?

Yes, syntactic sugar.

I noticed // for the unit tests we need to keep animations enabled and I'm curious to hear a little more on why we need animations. I'm not opposed to it but just interested in the reason.

If we remove the animations the modalTarget transitionend event never gets triggered which in turns never triggers the s-modal:shown event which in turns never execute the initial focus logic (see here and here).

I thought that setting the animation back for the unit tests might be a good move given that it puts our test environment even closer to how a regular user would experience it.
That said for the modal specifically I am not convinced about this check for the support of the transitionend event. A browser can support transitionend, but the user can override animation manually (like we did) and end up in a situation where s-modal:shown is never triggered. I expect this to be a very small percentage of use cases though, that's why I haven't spent more time on it or created a follow up ticket.