fix: shadow DOM scroll events — addGlobalScrollListener utility + close-on-scroll fix#10188
Conversation
eb278c5 to
a2dabe0
Compare
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! |
28fad85 to
bfa5e59
Compare
bfa5e59 to
c14a355
Compare
47ae921 to
91c3d89
Compare
snowystinger
left a comment
There was a problem hiding this comment.
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!
| }); | ||
|
|
||
| it('should close the overlay when target is window in a scroll event', function () { | ||
| it('should close the overlay when the page scrolls', function () { |
There was a problem hiding this comment.
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
Add two rsp-rules ESLint rules to prevent shadow DOM regressions with
non-composing events (scroll, scrollend, change, submit, reset, select,
selectstart, slotchange), which do not cross shadow DOM boundaries:
- add-event-non-composing: flags addEvent(target, 'scroll', ...) unless the
target is getPropagationTargets(...).
- no-non-composing-event-listener: flags x.addEventListener('scroll', ...) for
non-composing events, exempting non-DOM-tree receivers (visualViewport,
MediaQueryList).
Both rules are enabled globally and disabled for non-shipping code (tests,
stories, docs, chromatic, dev). Existing legitimate element-scoped listeners in
useFormValidation, useFormReset, and Chat carry inline exemptions with
explanations.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a70e772 to
b24c38d
Compare
# Conflicts: # packages/react-aria/src/utils/domHelpers.ts # packages/react-aria/test/utils/DOMFunctions.test.js
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