fix: shadow DOM scroll events — addGlobalScrollListener utility + close-on-scroll fix#10188
Conversation
| define: { | ||
| // Enable real size measurement in browser tests so virtualizer components | ||
| // use actual clientWidth/clientHeight instead of Infinity. | ||
| 'process.env.VIRT_ON': JSON.stringify('1') |
There was a problem hiding this comment.
I'll rebase off of #10190 so that whatever strategy you choose for your own PR gets automatically applied to mine.
There was a problem hiding this comment.
Also, that JSON.stringify is an embarrassing Claude silliness that I missed on my review. I admittedly paid more attention to the implementation than the config.
There was a problem hiding this comment.
Opted not to rebase given that conflated the diff for this PR with #10190. Instead, I adopted the same vitest.browser.config.ts changes so that git would be able to auto-resolve the merge conflict.
| export function addGlobalScrollListener( | ||
| globalTarget: Window | Document, | ||
| refElement: Element | null, | ||
| listener: EventListener, | ||
| options?: boolean | AddEventListenerOptions | ||
| ): () => void { | ||
| globalTarget.addEventListener('scroll', listener, options); | ||
|
|
||
| let shadowRoots: ShadowRoot[] = []; | ||
| if (shadowDOM() && refElement) { | ||
| let node: Node | null = refElement; | ||
| while (node) { | ||
| if (isShadowRoot(node)) { | ||
| shadowRoots.push(node as ShadowRoot); | ||
| node = (node as ShadowRoot).host; | ||
| } else { | ||
| node = node.parentNode; | ||
| } | ||
| } | ||
| for (let root of shadowRoots) { | ||
| root.addEventListener('scroll', listener, options); | ||
| } | ||
| } | ||
|
|
||
| return () => { | ||
| globalTarget.removeEventListener('scroll', listener, options); | ||
| for (let root of shadowRoots) { | ||
| root.removeEventListener('scroll', listener, options); | ||
| } | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we should aim for this to be more generic, since it applies to all events which don't compose. #10102 introduces an addEvent utility, wich accepts an array of event targets. We could use that in combination with a helper like this:
/**
* Collects the enclosing ShadowRoots between a node and the document.
*/
export function getShadowRoots(node: Node | null | undefined): ShadowRoot[] {
if (!shadowDOM()) {
return [];
}
let roots: ShadowRoot[] = [];
let current: Node | undefined = node?.getRootNode();
while (isShadowRoot(current)) {
roots.push(current);
current = current.host.getRootNode();
}
return roots;
}addEvent(window, 'scroll', listener, true);
addEvent(getShadowRoots(scrollRef.current), 'scroll', listener, true);There was a problem hiding this comment.
Clearly I'm on the bleeding edge given that I need to merge two in-progress PRs into this branch.
There was a problem hiding this comment.
Just FYI, it's not certain that #10102 will be merged. It's still awaiting review from the maintainer team. If it isn't you can just extract the addEvent utility.
There was a problem hiding this comment.
In that case, I'll ignore this comment until/unless #10102 gets merged first.
There was a problem hiding this comment.
I decided to take addEvent on anyway. Doing my best to reduce future merge conflicts. My biggest concern is that it would be nice if addEvent did getShadowRoots itself so that future developers didn't have to know about this shadow DOM limitation, but then addEvent would need an optional targetRef argument. That may make this limitation more discoverable. I'd be interested in the opinions of the maintainers on this matter.
eb278c5 to
a2dabe0
Compare
| return {scrollable, mountPoint}; | ||
| } | ||
|
|
||
| async function openComboBox(container: Element) { |
There was a problem hiding this comment.
FYI, there are also combobox testing utilities available in @react-aria/test-utils. See packages/react-aria-components/test/ComboBox.test.js for examples of how to use these.
cdde876 to
eaa70f9
Compare
|
Thanks for the review @nwidynski! I know you're not one of the maintainers, but it's best to get this PR in the best shape that it can be so that it's more likely to be approved. How did you even find this PR and realize that it touched on similar concepts as your in-progress work? |
|
@pzaczkiewicz-athenahealth Yep, that was the whole idea. To answer your question - it is pretty simple - I've made it a habit to skip over just about every other PR in this repo and have been doing so for a while. The ones I find interesting I comment on and most of the time try to pre-review, in the spirit of hopefully making time for the core team and also to stay up to date with changes in the hook layer, which we use at work. I've also found it to just be an amazing learning tool that works for me, haha! |
| * don't compose (e.g. scroll, scrollend), which do not propagate out of shadow | ||
| * roots even in the capture phase. Pass the result straight to `addEvent`. | ||
| */ | ||
| export function getEventTargets( |
There was a problem hiding this comment.
I'm happy with an API like this as well. Maybe we should differentiate from the getEventTarget above though? Something like getComposedTargets or getComposedEventTargets?
This also isn't constrained to global event targets, so I'm not sure if naming should put that much emphasis on that specific use case - even though it is arguably the most common one.
There was a problem hiding this comment.
Naming things is the hardest problem of computer science after all.
The biggest problem with the API you suggested was that every (current) invocation of it would also need to accumulate 2 cleanup functions. That makes things awkward. A common theme among DOMFunctions to date they've been 100% swappable with their light-dom equivalents. They're applicable in all cases and the coder doesn't need to think or worry about light vs shadow DOM so long as they use the wrapper function. The problem about scroll events is they need an additional parameter compared to the light-dom equivalent, so it can't be a drop-in replacement. You only need that additional parameter in events that don't compose, so I can't do a global find/replace to establish a safe pattern given that other call sites don't even have a "target" DOM node available.
I was a bit distracted yesterday, so didn't notice the near-duplicate name right above getEventTargets. Even so, I asked Claude yesterday to come up with better names, and got these:
- getEventTargets(global, node) — generic, matches addEvent's EventTarget[] param name. Clearly a bad choice now given context.
- getPropagationTargets(global, node) — emphasizes "everything the event needs to be heard on."
- getShadowRootsWithGlobal — clunky.
- getCapturingTargets — ties to capture-phase use.
- getNonComposedEventTargets(global, node) — most precise about why, but verbose.
- getGlobalEventTargets(global, node) — reads well: "the global-ish targets to listen on."
Given that getEventTargets is clearly wrong at this point, I'm liking getPropagationTargets. Seems to be a good compromise between precision and brevity. It's interesting to contrast getComposedEventTargets vs getNonComposedEventTargets. Claude is accurate in that these are specifically the DOM nodes we need given that the event isn't composing.
I'll push getPropagationTargets, but I'd be interested to hear your take on these, or other suggestions.
28fad85 to
bfa5e59
Compare
Summary
Fixes #10093. The `scroll` DOM event has `composed: false`, meaning it does not propagate out of shadow roots — not even in the capturing phase. This silently broke two behaviours when `enableShadowDOM()` is in use:
Approach
Added `addGlobalScrollListener` to `DOMFunctions.ts` — the established home for shadow-DOM-safe DOM wrappers. When `shadowDOM()` is off the function attaches only to the global target, identical to the original code. When `shadowDOM()` is on it additionally walks the ancestor chain from a reference element, collects every `ShadowRoot` found, and attaches a capturing `scroll` listener to each.
Updated all affected callers to use this utility:
Callers that attach scroll listeners directly to a local element (`TabPanelCarousel.tsx`, `Pagination.tsx`) and the `visualViewport` listener in `useOverlayPosition.ts` are unaffected — they don't rely on event propagation crossing shadow boundaries.
`vitest.browser.config.ts` has a new `define: { 'process.env.VIRT_ON': '1' }` entry required for the Tree browser test: without it, the virtualizer treats `NODE_ENV=test` as a signal to use `Infinity` for viewport dimensions and skips virtualization, making the scroll test meaningless.
Pull Request Checklist
Test Instructions
Automated browser tests (Chromium)
`packages/react-aria-components/test/Tree.browser.test.tsx` — Virtualizer / ScrollView:
Mounts a 50-item virtualized Tree inside a shadow root, scrolls the treegrid, and asserts Item 0 leaves the DOM while Item 20 appears.
`packages/react-aria-components/test/Select.browser.test.tsx` — useCloseOnScroll:
Existing unit tests
🤖 Generated with Claude Code