Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new scrollIntoView tests call root.renderer.scrollIntoView(...), but the JS TestRenderer wrapper does not implement that method, which will fail at runtime unless the wrapper is updated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends GPUIX’s native rendering pipeline to support browser-like scrolling affordances: it paints scrollbars for scroll boxes (overflow: scroll|auto) and adds a scrollIntoView(elementId, block, inline) API that scrolls ancestor scroll containers while honoring scroll-margin and scroll-padding.
Changes:
- Add native scrollbar rendering (overlay/classic modes) with support for
scrollbar-width,scrollbar-color, andscrollbar-gutter. - Implement
scrollIntoViewend-to-end (native renderer + test renderer), includingscroll-margin/scroll-paddingresolution. - Add React-side type updates and a dedicated scrollbar/scrollIntoView native test suite, plus a changeset.
File summaries
| File | Description |
|---|---|
| packages/react/src/types/host.ts | Extends StyleDesc and NativeRenderer typings for scrolling + scrollIntoView. |
| packages/react/src/tests/scrollbars.test.tsx | Adds native tests covering classic/overlay scrollbars and scrollIntoView behavior. |
| packages/native/src/test_renderer.rs | Exposes scrollIntoView in the native test renderer; refactors scroll offset conversion. |
| packages/native/src/style/resolve.rs | Updates overflow resolution to treat clip like hidden and apply used-overflow coercion. |
| packages/native/src/style.rs | Adds style fields for scrollbar and scroll-into-view related properties. |
| packages/native/src/renderer/scrollbar.rs | New native scrollbar implementation (rendering + input handling + CSS-like parsing). |
| packages/native/src/renderer/scroll_into_view.rs | New scroll-into-view implementation honoring margins/padding and alignment modes. |
| packages/native/src/renderer/frame.rs | Integrates scrollbars into div build, including gutter reservation and element insertion. |
| packages/native/src/renderer.rs | Wires scrollIntoView through NAPI and UI command routing; adds offset_to_js. |
| packages/native/index.d.ts | Regenerated declarations reflecting the new scrollIntoView API. |
| .changeset/scrollbars.md | Changeset documenting scrollbar rendering and scrollIntoView additions. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
# Conflicts: # examples/demo.test.tsx # examples/demo/app.tsx # packages/native/src/renderer/frame.rs
# Conflicts: # packages/native/src/custom_elements/code.rs # packages/native/src/renderer/frame.rs # packages/native/src/style/resolve.rs # packages/react/src/testing.ts # packages/react/src/types/host.ts
Owner
Author
|
Moved to remorses#53. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Contains #3.
What this adds
A box with
overflow: scrollorautogets a scrollbar on each axis it scrolls. The OS picks overlay or classic, as a browser does. An overlay bar shows for a second after a scroll and fades. A classic bar sits in a gutter the layout reserves. A drag of the thumb scrolls, a click in the track moves one page.scrollbar-width,scrollbar-colorandscrollbar-gutter(withstable both-edges) work.scrollbar-colortakesautoor exactly two colours, as css defines it, and one colour drops the declaration.GPUIX_SCROLLBARS=overlay|classicoverrides the OS choice for tests. A bar paints after the whole frame, above sibling effects.scrollIntoView(elementId, block, inline)scrolls every scroll box around an element, withscroll-marginandscroll-paddinghonoured, and the TestRenderer wraps it for the react tests.What I ran and saw
cargo testat this branch: 301 passed, 0 failed. Unit tests cover the two-colour rule and the overscroll shorthand split.scrollIntoViewthrough the TestRenderer.bun run buildat this commit, not from a hand edit.What I did not verify