fix: shadow DOM scroll events — addGlobalScrollListener utility + close-on-scroll fix by pzaczkiewicz-athenahealth · Pull Request #10188 · adobe/react-spectrum

pzaczkiewicz-athenahealth · 2026-06-11T16:50:45Z

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:

Virtualizer / ScrollView — `document.addEventListener('scroll', onScroll, true)` never fires for elements inside a shadow root, so the virtualizer never updates which items are visible during scroll.
`useCloseOnScroll` (overlays) — `window.addEventListener('scroll', onScroll, true)` never fires for scrollable ancestors inside a shadow root, so combobox/popover overlays do not close when their ancestor scrolls.

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:

`packages/react-aria/src/virtualizer/ScrollView.tsx` (replaced manual shadow-root-walking code from an earlier partial fix)
`packages/react-aria/src/overlays/useCloseOnScroll.ts`
`packages/@adobe/react-spectrum/src/menu/useCloseOnScroll.ts`

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

Identified root cause: `scroll` events are `composed: false` and do not propagate out of shadow roots, even in the capture phase on `document`/`window`
Audited all `addEventListener('scroll'/'scrollend')` usages in library code; confirmed which rely on global propagation vs. direct element attachment
Created `addGlobalScrollListener` in `DOMFunctions.ts`, gated on `shadowDOM()` flag — light-DOM behaviour is unchanged when the flag is off
Updated `ScrollView.tsx` to use the new utility (removed manual ancestor-walking code)
Updated both copies of `useCloseOnScroll.ts` to use the new utility
Types compile cleanly (`yarn check-types:tsc`)
New browser tests written, confirmed to fail before the fix and pass after (see Test Instructions)
296 existing unit tests pass across Tree, VirtualizedMenu, and all overlay suites (1 pre-existing skip)

Test Instructions

Automated browser tests (Chromium)

`packages/react-aria-components/test/Tree.browser.test.tsx` — Virtualizer / ScrollView:

yarn vitest --config vitest.browser.config.ts run packages/react-aria-components/test/Tree.browser.test.tsx --project=chromium-desktop

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:

yarn vitest --config vitest.browser.config.ts run packages/react-aria-components/test/Select.browser.test.tsx --project=chromium-desktop

Light DOM test: Opens a ComboBox inside a scrollable div, fires a scroll event, confirms the popover closes — regression guard, passes before and after.
Shadow DOM test: Same setup inside a shadow root — fails before the fix, passes after.

Existing unit tests

yarn jest packages/react-aria-components/test/Tree.test.tsx packages/react-aria-components/test/VirtualizedMenu.test.tsx
yarn jest packages/react-aria/test/overlays/

🤖 Generated with Claude Code

nwidynski · 2026-06-22T10:52:10Z

+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);
+    }
+  };
+}
+


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);

Clearly I'm on the bleeding edge given that I need to merge two in-progress PRs into this branch.

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.

In that case, I'll ignore this comment until/unless #10102 gets merged first.

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.

I don't think it should do it automatically, addEvent could be for any element, not necessarily global ones. It's an internal utility function, we can always update it in the future if we need to. So i think it's fine as it is, thanks!

pzaczkiewicz-athenahealth · 2026-06-23T01:49:59Z

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?

nwidynski · 2026-06-23T06:39:27Z

@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!

nwidynski · 2026-06-23T13:06:10Z

+ * 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(


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.

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.

It's an internal function with a jsdoc and examples of when to use. I'm fine with this name.

snowystinger · 2026-07-02T04:31:40Z

-    document.addEventListener('scroll', onScroll, true);
-    return () => document.removeEventListener('scroll', onScroll, true);
-  }, [onScroll]);
+    return addEvent(getPropagationTargets(document, ref.current), 'scroll', onScroll, true);


This is going to trip a lint rule eventually (whenever we turn it on), it should be

Suggested change

return addEvent(getPropagationTargets(document, ref.current), 'scroll', onScroll, true);

return addEvent(getPropagationTargets(getOwnerDocument(ref.current), ref.current), 'scroll', onScroll, true);

In which case, we may as well push it into getPropagationTargets with a default of document and make the ref required. People can always pass document as the ref.current if they truly only want the global.

Good call on adding getOwnerDocument. I have two observations:

All calls currently add events to either document or window. While I can't think of a practical difference between the two as it concerns event bubbling, I took a conservative approach to not change window to document in case there was a difference I was not aware of.

It's possible there may be a future use-case where someone wants to add an event listener to a non-ownerDocument node that nonetheless has one or more shadow roots between it and from. This is admittedly an unlikely event, so a simpler API makes things better. If that use-case were come to pass, this function can always be amended with more parameters.

snowystinger · 2026-07-02T04:33:00Z

+ */
+export function getPropagationTargets(
+  global: EventTarget,
+  refNode: Node | null | undefined


Suggested change

refNode: Node | null | undefined

from: Node | null | undefined

It's not required to be a node from a ref, it can be from anywhere

snowystinger · 2026-07-02T04:39:13Z

+ * - `selectstart`
+ * - `slotchange`
+ */
+export function addEvent<T extends EventTarget, K extends keyof EventMapType<Exclude<T, null>>>(


This function already exists in usePreventScroll

Interestingly, your PR and https://github.com/adobe/react-spectrum/pull/10102/changes#diff-97811109fb85d90f4e601e5585ebdee17e8cdd3c2ccbc4f2a5d2d654f880d893 both have the exact same new addEvent. I am fine with this, but it should be removed from the other location if it's going to be here

@snowystinger #10188 (comment)

@nwidynski was very helpful in giving my PR an initial once-over. I deliberately made the implementation identical, with the exception of additional js-doc explaining when to use in conjunction with getPropagationTargets. They're in the same file at the same location, so git will force merge conflict resolution one way or another.

snowystinger · 2026-07-02T04:40:40Z

+/**
+ * Attaches an event listener on target(s) and returns a cleanup function.
+ *
+ * Some events are `composed: false` and do not propagate out of shadow roots,
+ * even in the capture phase. For those, a listener on `window`/`document` alone
+ * will miss events that fire inside a shadow root. Pass the array from
+ * `getPropagationTargets(global, refNode)`
+ * (react-aria/private/utils/shadowdom/DOMFunctions) as `target` so the listener
+ * is attached to the global target *and* every enclosing shadow root.
+ *
+ * Known `composed: false` events (non-exhaustive):
+ *
+ * - `scroll`
+ * - `scrollend`
+ * - `change`
+ * - `submit`
+ * - `reset`
+ * - `select`
+ * - `selectstart`
+ * - `slotchange`
+ */


Suggested change

/**

* Attaches an event listener on target(s) and returns a cleanup function.

*

* Some events are `composed: false` and do not propagate out of shadow roots,

* even in the capture phase. For those, a listener on `window`/`document` alone

* will miss events that fire inside a shadow root. Pass the array from

* `getPropagationTargets(global, refNode)`

* (react-aria/private/utils/shadowdom/DOMFunctions) as `target` so the listener

* is attached to the global target *and* every enclosing shadow root.

*

* Known `composed: false` events (non-exhaustive):

*

* - `scroll`

* - `scrollend`

* - `change`

* - `submit`

* - `reset`

* - `select`

* - `selectstart`

* - `slotchange`

*/

/**

* Attaches an event listener on target(s) and returns a cleanup function.

*/

None of that has anything to do with this function, it is related to the other place this comment appears in the file above, DOMFunctions.ts

Fair enough. My concern is that since not all events compose, new addEvent calls may not add getPropagationTargets, creating new feature gaps for apps in the shadow DOM. Cross-referencing the two functions increases the chances that new bugs don't get created, especially if an LLM is used to add the new addEvent calls.

I think we should cover the case with a lint rule, then we don't need to consult this each time, we'll just get an error that tells us

…adobe#10093)

snowystinger

Opening Level 1 Item 3 then Level 2 Item 3 results in blank space at the end of the Tree. It was missing the rowHeight, so I've fixed that.

I also added a control to the top of the storybook to enable the shadow dom flag since once set, it cannot be unset and it would have leaked into every story you visit after that point with no indication that was what had happened.

I think this looks right. Thanks!

snowystinger · 2026-07-03T04:44:11Z

  });

-  it('should close the overlay when target is window in a scroll event', function () {
+  it('should close the overlay when the page scrolls', function () {


I think your intuition was correct for this one, we should allow it to attach to the window object. I'd go so far as to say that global scroll should always be attached to window, not document, and the other instances are wrong.

If you think otherwise, then this test is now a duplicate of the one before it.

For now I've updated it to default to window and added tests so that we know what the contract is

nwidynski reviewed Jun 22, 2026

View reviewed changes

pzaczkiewicz-athenahealth mentioned this pull request Jun 22, 2026

fix: make sure virtualized grid layouts dont disappear when hidden via display: none #10190

Merged

5 tasks

pzaczkiewicz-athenahealth force-pushed the Issue-10093-virtualizer-shadow-dom branch 2 times, most recently from eb278c5 to a2dabe0 Compare June 22, 2026 15:15

nwidynski reviewed Jun 22, 2026

View reviewed changes

Comment thread packages/react-aria-components/test/Select.browser.test.tsx Outdated

pzaczkiewicz-athenahealth force-pushed the Issue-10093-virtualizer-shadow-dom branch from cdde876 to eaa70f9 Compare June 23, 2026 01:46

nwidynski reviewed Jun 23, 2026

View reviewed changes

pzaczkiewicz-athenahealth force-pushed the Issue-10093-virtualizer-shadow-dom branch from 28fad85 to bfa5e59 Compare June 23, 2026 15:57

snowystinger reviewed Jul 2, 2026

View reviewed changes

Comment thread vitest.browser.config.ts

snowystinger reviewed Jul 2, 2026

View reviewed changes

pzaczkiewicz-athenahealth force-pushed the Issue-10093-virtualizer-shadow-dom branch from bfa5e59 to c14a355 Compare July 2, 2026 18:00

fix: Virtualizer, useCloseOnScroll are able to work in the shadow DOM (…

91c3d89

…adobe#10093)

pzaczkiewicz-athenahealth force-pushed the Issue-10093-virtualizer-shadow-dom branch from 47ae921 to 91c3d89 Compare July 2, 2026 18:38

snowystinger added 2 commits July 3, 2026 15:03

reduce scope of testing, add tests for helper, fix storybook behaviours

5df106d

fix ts

2ac85d6

snowystinger approved these changes Jul 3, 2026

View reviewed changes

snowystinger added the ready for review label Jul 3, 2026

	return addEvent(getPropagationTargets(document, ref.current), 'scroll', onScroll, true);
	return addEvent(getPropagationTargets(getOwnerDocument(ref.current), ref.current), 'scroll', onScroll, true);

	refNode: Node \| null \| undefined
	from: Node \| null \| undefined

Uh oh!

Conversation

pzaczkiewicz-athenahealth commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Approach

Pull Request Checklist

Test Instructions

Automated browser tests (Chromium)

Existing unit tests

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwidynski Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pzaczkiewicz-athenahealth commented Jun 23, 2026

Uh oh!

nwidynski commented Jun 23, 2026

Uh oh!

nwidynski Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

snowystinger Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwidynski Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pzaczkiewicz-athenahealth commented Jun 11, 2026 •

edited

Loading

nwidynski Jun 22, 2026 •

edited

Loading

nwidynski Jun 23, 2026 •

edited

Loading

snowystinger Jul 2, 2026 •

edited

Loading

nwidynski Jul 2, 2026 •

edited

Loading