-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(virtualizer): preserve subpixel precision in ScrollView to preven… #10489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
devo-id
wants to merge
7
commits into
adobe:main
Choose a base branch
from
devo-id:fix/virtualizer-fractional-width
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+147
−5
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4b5c598
fix(virtualizer): preserve subpixel precision in ScrollView to preven…
devo-id 23826ca
test: remove artificial fractional sizing tests
devo-id 46c6428
fix: preserve fractional virtualizer dimensions
devo-id 750f688
fix: handle virtualizer test environment measurements
devo-id c4a97ac
add chromatic and remove story now it's automated
snowystinger 6636228
revert chromatic and fix storybook
snowystinger 4e2acc4
fix: handle viewport dimensions in ScrollView
devo-id File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| /* | ||
| * Copyright 2026 Adobe. All rights reserved. | ||
| * This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. You may obtain a copy | ||
| * of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under | ||
| * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS | ||
| * OF ANY KIND, either express or implied. See the License for the specific language | ||
| * governing permissions and limitations under the License. | ||
| */ | ||
|
|
||
| import {act, render} from '@react-spectrum/test-utils-internal'; | ||
| import React, {useRef} from 'react'; | ||
| import {Size} from 'react-stately/useVirtualizerState'; | ||
| import {useScrollView} from '../../src/virtualizer/ScrollView'; | ||
|
|
||
| function RootScrollView( | ||
| props: Partial<Parameters<typeof useScrollView>[0]> & {target: HTMLElement} | ||
| ) { | ||
| let {target, ...otherProps} = props; | ||
| let ref = useRef(target); | ||
| let {contentProps} = useScrollView( | ||
| { | ||
| contentSize: new Size(1200, 2000), | ||
| onVisibleRectChange: jest.fn(), | ||
| allowsWindowScrolling: true, | ||
| ...otherProps | ||
| }, | ||
| ref | ||
| ); | ||
| return <div {...contentProps} />; | ||
| } | ||
|
|
||
| describe('ScrollView', () => { | ||
| beforeAll(() => { | ||
| jest.useFakeTimers(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| act(() => { | ||
| jest.runAllTimers(); | ||
| }); | ||
| }); | ||
|
|
||
| it('preserves viewport client dimensions when attached to documentElement', () => { | ||
| let origNodeEnv = process.env.NODE_ENV; | ||
| process.env.NODE_ENV = 'production'; | ||
| try { | ||
| Object.defineProperty(document.documentElement, 'clientWidth', { | ||
| configurable: true, | ||
| value: 1200 | ||
| }); | ||
| Object.defineProperty(document.documentElement, 'clientHeight', { | ||
| configurable: true, | ||
| value: 800 | ||
| }); | ||
| Object.defineProperty(document.documentElement, 'offsetHeight', { | ||
| configurable: true, | ||
| value: 50 | ||
| }); | ||
| let rectSpy = jest.spyOn(document.documentElement, 'getBoundingClientRect').mockReturnValue({ | ||
| width: 1200, | ||
| height: 50, | ||
| top: 0, | ||
| left: 0, | ||
| bottom: 50, | ||
| right: 1200, | ||
| x: 0, | ||
| y: 0, | ||
| toJSON: () => {} | ||
| }); | ||
|
|
||
| let onSizeChange = jest.fn(); | ||
| render(<RootScrollView target={document.documentElement} onSizeChange={onSizeChange} />); | ||
|
|
||
| expect(onSizeChange).toHaveBeenCalledWith(new Size(1200, 800)); | ||
|
|
||
| delete (document.documentElement as any).clientWidth; | ||
| delete (document.documentElement as any).clientHeight; | ||
| delete (document.documentElement as any).offsetHeight; | ||
| rectSpy.mockRestore(); | ||
| } finally { | ||
| process.env.NODE_ENV = origNodeEnv; | ||
| } | ||
| }); | ||
| }); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct, because
getBoundingClientRect()returns theborder-boxof an element, which is fine for normal elements but can easily break window scrolling, because neitherborder-boxnor offset sizes work for the viewport. We should be using something similar toModal.tsxinstead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nwidynski, thanks to pointing this out.
I traced the viewport case and confirmed the previous
getClientSize()logic could use the document content height instead of the actual viewport height for root scrolling elements.I updated it to keep the native
clientWidth/clientHeightfordocument.documentElement,body, anddocument.scrollingElement, while keeping the fractional measurement for normal elements unchanged. I also added a regression test covering the document root case.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not sure of how pedantic we want to be about root elements but there is a lot more complexity that goes into which element is responsible for viewport overflow. For example, the body element can be an independent scroll container, so bailing out unconditionally may not be right. I will let @snowystinger weigh in there.
I also noticed that this is potentially introducing a breaking change to test environments, since the NODE_ENV flag is now required. This may be okay, but
clientWidthandclientHeightare documented public API for this use case as far as i know, so I wanted to bring it up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. I hadn't considered that body can also act as an independent scroll container, so I agree the root element handling needs to be more careful. The NODE_ENV change is also worth looking at since it changes which measurement path tests use. I'll look into both before making another change.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC RAC's Virtualizer doesn't support window scrolling, so this may not be a big issue.The biggest risk in mixinggetBoundingClientRectwithclientWidth/offsetWidthis CSS transforms, though I'm not sure whether RAC cares about that case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lixiaoyan https://react-aria.adobe.com/releases/v1-17-0
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha I missed it. Anyhow the CSS transform is still a real issue.