💥 No Changeset

Latest commit: 226ef75

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Since we would prefer to make this change in focus-trap (see #86), then this PR is no longer needed.

However, I would like to get some feedback from @stefcameron et al. about the testing plan (see Considerations > Tests). I could open another PR with those specific changes.

Since we would prefer to make this change in focus-trap (see #86), then this PR is no longer needed.

This PR is still needed, but now only for its update to the prop-types of the allowOutsideClick option. So the rest of the changes should be reverted, but that line's change should be kept, and a Changeset added for a patch update. Of course, we'll have to make the change over on focus-trap first, but we should keep this PR open until then.

However, I would like to get some feedback from @stefcameron et al. about the testing plan (see Considerations > Tests). I could open another PR with those specific changes.

I suggest we pull the transformation of specifiedFocusTrapOptions into tailoredFocusTrapOptions into a separate function that we can test in isolation. That way we can have confidence that we are tailoring the options correctly for use by focus-trap.

I think you're talking about this block of code, is that right? If so, then yes, I say that's a good idea to pull out into a separate class method so that it could be tested in isolation.

But there's currently no testing framework setup for this repo. I think David saw it as simple enough to test manually with the test server. I think another reason is that js-dom, what's typically used to run tests by most popular JS test frameworks like Enzyme or TestingLibrary, does not fully support focus events. I've been working a lot with focus lately at work, with focus-trap-react and React TestingLibrary, and I've found that while js-dom will work with some direct focus methods like HTMLElement#focus(), it generally doesn't work well with anything else. So you can't, for example (AFAIK in my own attempts), actually test the tabbing sequence works.

So while I think some unit tests should be good, I think we also need to be conscious that manual testing will have to remain essential for certain parts of the code.

For unit tests, I'd personally like to see jest and @testing-library/react (RTL). Jest is the best test runner I've used so far (I've made projects that used Jasmine and Mocha as well), and RTL is the best testing library I've used so far (after using numerous others, including Enzyme). RTL has this philosophy of testing right in the DOM, as close to what the user would interact with in their browser as possible, and I think that sets it a mark above Enzyme.

@maraisr Any thoughts from you on testing setup?

But that can be another PR (and doesn't have to be from you either). I'm just giving my thoughts here since you brought-up testing.

For what it's worth, I second the proposal of using jest and @testing-library/react. They are both popular solutions and might be a workable option for testing focus.

Given that testing frameworks based on js-dom seem to have occasional problem with the focus, have you considered
cypress that runs in real browser and the example page can already be a very good testing target? It can be run both in head and headless mode and I feel it guarantees the most realistic user experience in the real environment.

@liunate Cypress is a great idea! Even with jest-dom's toHaveFocus() assertion, I know from experience that not all focus events work in js-dom. But Cypress in headless mode could work nicely in our CI workflow. Do you have experience with Cypress? Could you help set that up?

Happy to help though there's some open source processes things I need to go thru with my employer first. Will let you know soon.

@SeanMcP FYI, I haven't forgotten about this. I'm just waiting for a review on focus-trap/focus-trap#125, then I'll publish focus-trap@6.0.0 (hopefully I've gotten publish rights by then, still not sure if I have that yet), and then we can update this PR with an update to the focus-trap dependency, and finally merge it!

Update: So focus-trap is in a state where it could be published now, but with its changes, it should be major. Looking further down the line to tabbable, it also has some breaking changes pending that should result in a major. A major in tabbable would force a major in focus-trap, would force a major in focus-trap-react.

So rather than do a bunch of successive majors in this repo and focus-trap, I'm going to update the tabbable repo (which would also, just to be sure, since I'm new and the dependencies are very old, warrant a major), and then do a major release of each one in succession. I'll include this PR in that major that will pull-in the updated focus-trap dependency.

@liunate Cypress is a great idea! Even with jest-dom's toHaveFocus() assertion, I know from experience that not all focus events work in js-dom. But Cypress in headless mode could work nicely in our CI workflow. Do you have experience with Cypress? Could you help set that up?

I just created #95 for this.

Update: Getting closer! I just updated all the deps on tabbable and we're nearly set to publish a major there, which will roll into a major on focus-trap, grabbing the code this PR depends on, and then we'll have a major of this package, and then it'll finally be published!

@SeanMcP Finally, #110 gives us focus-trap@6.0.0 which adds the feature related to this change! 🎉

Published in 8.0.0 just now.