test: adds ShadowDOM / UNSAFE_PortalProvider tests #8806
Conversation
These tests specifically target issue adobe#8675 where menu items in popovers close immediately instead when using ShadowDOM with UNSAFE_PortalProvider. New test suites added: - FocusScope: Shadow DOM boundary containment issues - usePopover: Shadow DOM popover interactions and focus management - useFocusWithin: Focus within detection across shadow boundaries - useInteractOutside: Interact outside detection with portals I generated these tests with AI then reviewed / updated them.
After applying the patches I mentioned in my issue I've noticed that many of the AI generated tests were either broken or just not testing anything interesting. Luckily there are some that are. I'll have to update the rest of the tests as they aren't passing at the moment.
I have a conflicting default prettier config (I can't seem to find this projects.) I'm reverting a number of my autochanges.
There are a bunch of redundant tests. Getting rid of them for now.
packages/@react-aria/interactions/test/useInteractOutside.test.js
Outdated
Show resolved
Hide resolved
packages/@react-aria/interactions/test/useInteractOutside.test.js
Outdated
Show resolved
Hide resolved
packages/@react-aria/interactions/test/useInteractOutside.test.js
Outdated
Show resolved
Hide resolved
| */ | ||
| export function getEventTarget<T extends Event>(event: T): Element { | ||
| if (shadowDOM() && (event.target as HTMLElement).shadowRoot) { | ||
| if (shadowDOM() && (event.target as HTMLElement)?.shadowRoot) { |
There was a problem hiding this comment.
interesting, when was this not defined?
There was a problem hiding this comment.
It was a while ago I did this, I'll try and reproduce it and see exactly when this happened.
Co-authored-by: Robert Snow <[email protected]>
Co-authored-by: Robert Snow <[email protected]>
I added a popover test but it seems that when creating my replication in test code I noticed that the popover is never displayed. Seems that there is an issue with showing popovers in the tests when everything is in the shadow dom...
I had to add another dependency to get the test to actually find the parts of the shadow dom I was interested in. Seems like the default testing libraries don't cover this case unfortunately.
|
Okay so I've updated this to have a much better test which in my opinion tests the functionality. I unfortunately had to add another dependency to do so: https://github.com/konnorrogers/shadow-dom-testing-library I wasn't able to find the shadow dom elements without it 🤷 The bad news is that I reverted the code changes and the test still passes so I'm not sure what's going on. Either:
Either way this is a more complete PR at least. |
| ] | ||
| ], | ||
| "dependencies": { | ||
| "shadow-dom-testing-library": "^1.13.1" |
There was a problem hiding this comment.
issue: All dependencies within this package.json are devDependencies. I see no reason why shadow-dom-testing-library would be any different.
There was a problem hiding this comment.
This was just a mistake, I'll move it to dev deps.
| let onBlur = useCallback((e: FocusEvent) => { | ||
| // Ignore events bubbling through portals. | ||
| if (!e.currentTarget.contains(e.target)) { | ||
| if (!nodeContains(e.currentTarget as Element, e.target as Element)) { |
There was a problem hiding this comment.
issue (repeated): The fact that you have to add type assert highlights that nodeContains does not accept the optimal set of types. Requiring type assertions in order to use a function erodes trust that the function is being used correctly.
suggestion: While Adobe hasn't gotten back to either of us since we restarted this work last week on whether they'd prefer a targeted approach or a universal approach, I can say that the changes I made to DomFunctions.ts are an improvement.
| ); | ||
| } | ||
| render( | ||
| <UNSAFE_PortalProvider getContainer={() => portal}> |
There was a problem hiding this comment.
question for Adobe: While I like the concept of having a PortalProvider so that each individual Popover shouldn't need to burden itself with knowing where to mount, there's a subtlety to juggling refs that is difficult to solve for the initial-render case. In React, we can't have access to the DOM node until the ref function has a chance to fire. This means that every React component in the render tree goes through one render cycle with no refs present. This is problematic for ReactDOM.createPortal, as getContainer() is forced to return document in the absence of resolved refs. This can cause the portalled content to briefly be rendered at document.body before re-rendering at shadowRoot in the next render cycle. Given that getContainer is a function, it may not even update once shadowRoot is resolved, keeping the Popover mounted at the wrong DOM node until the user interacts with it or something, causing getContainer to be reevaluated at its next render.
This test obfuscates this complexity, as shadowRoot is created outside of React. The most synchronous way to create a shadow root within React is do do so within a ref callback function, and store that DOM node in state rather than as a ref. This forces a re-render, and allows ReactDOM.createPortal to use the resolved DOM node. In order to prevent the initial ReactDOM.creaetPortal from mounting at document.body, I have to delay rendering until the mount point has a chance to resolve. None of this is done in react-aria's Popover, as I can see delaying rendering being problematic in server side rendering.
My question is this: Does react-aria have a plan for how to delay rendering for Popovers, or is it up to the application code to identify and correct this race condition?
|
My team is watching this PR and would like to see this in as soon as possible as well! We have the patches running locally but would prefer them in a release before releasing to production. As a heads up, I did try and test all of S2 portal components with these changes and the ComboBox did exhibit a number of odd behaviors. All other component behavior seemed normal. Great job on this fix, thus far! |
4 participants
Closes #8675
✅ Pull Request Checklist:
📝 Test Instructions:
Not sure what the test instructions would be, I have this reproduction repo: https://github.com/johnpangalos/rac-shadow-dom-bug-report, but some of the React Spectrum team couldn't run it. I can try and set it up so that you can link the repo to a build of RAC.
I have one non passing test, but I thought it was best to put up a PR anyway with the changes I've had time to finish so far.
The tests were originally created by AI, but I've had to alter them to get them to test what I'd actually like them to test.
🧢 Your Project:
Thomson Reuters (Pagero)