Shadow dom event handling #8991
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.
Co-authored-by: Robert Snow <snowystinger@gmail.com>
Co-authored-by: Robert Snow <snowystinger@gmail.com>
# Conflicts: # package.json # packages/@react-aria/combobox/src/useComboBox.ts # packages/react-aria-components/src/Popover.tsx
|
I haven't touched tests yet, but wanted to open this PR as a draft given the scope of what I've touched. I feel like I should scale back, as the size of the delta is scary, but at the same time, react-spectrum won't have shadow DOM support unless all bubbling event.target or node.contains calls are shadow-dom safe. I didn't touch changeEvents because they don't bubble. |
| return event.composedPath()[0] as Element; | ||
| export function getEventTarget<E, SE extends SyntheticEvent<E>>(event: SE): SE extends EventTargetType<infer Target> ? Target : never; | ||
| export function getEventTarget(event: Event): EventTarget | null; | ||
| export function getEventTarget<E, SE extends SyntheticEvent<E>>(event: Event | SE): EventTarget | null { |
There was a problem hiding this comment.
I made two major enhancements to this function's Typescript definition:
- Use function overloading to give a more accurate return type when a SyntheticEvent is used. For example, the target for a
FocusEventisEventTarget & Target. The first overload preserves that type, resulting in no need for type assertions in those situations compared to usingevent.target. - If the event is a
Event, then the return type isEventTarget | null, which is consistent with the type definitions in @types/react. The existing implementation assumes that event.target isElement. While is this true in most circumstances, I worry that glossing over the edge cases in this central function could have unforeseen runtime consequences.
| ``` | ||
|
|
||
| It's likely that you are using a different version of Node.js. Please use Node.js 18. When changing the node version, delete `node_modules` and re-run `yarn install` | ||
| It's likely that you are using a different version of Node.js. Please use Node.js 22. When changing the node version, delete `node_modules` and re-run `yarn install` |
There was a problem hiding this comment.
package.json has:
"@types/node@npm:*": "^22",
"@types/node@npm:^18.0.0": "^22",
"@types/node@npm:>= 8": "^22",
This implies that the recommended Node.js version has changed. If true, documentation should update as well.
| "en-US" | ||
| ] | ||
| ], | ||
| "volta": { |
There was a problem hiding this comment.
Volta has been extremely helpful in my projects to standardize versions of node and yarn used by my developers. Hopefully this isn't an untoward addition to package.json.
|
Hi there @pzaczkiewicz-athenahealth I have been banging on the keyboard trying to find at the very least a temporary solution for shadow dom issues while waiting for the team to take on the initiative. I am curious if you are still perusing this as well, or have moved on to other solutions? Would love to get your insights if you don't mind! |
|
No there is no workaround short of forking react-spectrum and fixing it yourself. We've deprioritized our adoption of react-aria-components for now. |
|
Thank you! Seems you did decide on a systemic approach after all. Breaking into multiple PRs makes sense. |
4 participants
Closes #8675
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: