Focus not trapped in form

[tyler-dane]

Prerequisites

• [x] Using an up-to-date main branch

Expected Behavior

Given that the event form is open When the user TABs through all fields Then the focus should restart at the beginning (the title field) And when the user pressing ESC Then the form should still close

Current Behavior

After cycling through all form fields, the focus goes back to the URL

Steps to Reproduce

1. Create event or draft

2. Hit TAB until focus goes to URL bar

Possible Solution (Not obligatory)

Trap focus, either natively or using a library

Context

Please include react-testing-library/jest tests to confirm your solution

[DiegoMutre]

Hey @tyler-dane 👋, I'd love to work on this one.

[tyler-dane]

Hey @tyler-dane 👋, I'd love to work on this one.

Sounds good!

[DiegoMutre]

Hey @tyler-dane, sorry for closing the PR (I struggle with git). I just added the commits to the branch for the PR but I have one last issue. I synced my branch with the latest commits of the upstream repository, but when trying to run yarn dev:backend, it throws this error:

Error: The running SuperTokens core version is not compatible with this NodeJS SDK. Please visit https://supertokens.io/docs/community/compatibility to find the right versions
...

So, I think that by updating supertokens in #123, now I need to upgrade my core, which is 7.0, to a compatible version. I just emailed the supertokens team for that.

Btw, the test commands work well but in linux (I'm using WSL) and not in Windows, for some reason.

So, once my supertokens-core is upgraded and I add the tests, I will test the new implementation in the browser and send the PR. Although, I can do it for a preview of the code.

Anyway, let me know anything new. 👋

[tyler-dane]

Hey @DiegoMutre sorry to hear about the SuperTokens issues. You're right about needing to upgrade your core as a result of #123. I should've communicated expectations and next steps better in that PR.

While you wait for an upgrade, you could update your package.json locally to use the old version that is compatible with your current ST core version.

That's interesting that the tests commands pass in Linux but not windows.

[DiegoMutre]

A quick update: I've been working on this issue this week. It works well, as I said before. I'm actually stuck making the tests work. I find a new problem every day when testing. Also, I had to install a linux distro alongside Windows because WSL was taking a lot of RAM and sometimes consumed a lot of disk when it was short of RAM (because the project is big). I tried to find a fix for tests not working in jest but found nothing yet.

I will keep working on making the tests work.

[tyler-dane]

Hey @DiegoMutre , glad to hear you're committed to seeing things through.

If the testing issues persist, we can merge without tests and add them in another issue.

Since the UI is so simple, it's easy to notice when bugs happen during manual testing. The way the UI currently manages state also makes UI testing tricky, because there are so many side effects. (This is why you'll see that act warning in the testing output).

Testing is a 'nice-to-have' for this issue. I'd rather us get this feature in and you pick up other work that more directly benefits users rather than stay stuck on testing.

[tyler-dane]

@DiegoMutre - Thanks for the PR! Works great. About to merge and deploy it.

One final note on testing: I saw that this testing section in the focus-trap docs mentioned that testing with jsdom is not supported. Wonder if that was the issue. If so, I think we made the right call by not forcing it with tests.

Testing this feature is better suited for a tool like Playwright, which supports full DOM interaction testing. This'll be a good discussion to revisit once we add that down the line.