Skip to content

Synchronize native scroll state with the UI tree - #5

Merged
dogukani merged 1 commit into
mainfrom
fix/native-scroll-state
Sep 24, 2026
Merged

dogukani merged 1 commit into
mainfrom
fix/native-scroll-state

Conversation

@dogukani

@dogukani dogukani commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Win32 scroll widgets update their own scrollbar and document position, while framework layout keeps a separate scroll offset. After native wheel or scrollbar input, rendered content and the Core tree can disagree.

Change

  • Mirror native vertical scroll changes into the UI tree and synchronize the native scrollbar when the tree changes.
  • Let framework layout own the vertical content offset for framework-backed widgets. Preserve the existing native offset path for Controls widgets and horizontal scrolling.

Verification

  • Windows smoke build and built-in verification passed.
  • notes-jsx built; its initial PNG matched the previous regression capture byte for byte.
  • Two page-downs in the Notes list produced 2172 device pixels and 1086 logical pixels at 2x scale, with later notes visible.
  • With the matching local Core and examples changes, 12 page-downs in virtual-list displayed rows 69–73 instead of a blank viewport.

Integration retest: the first image-based scroll check did not exercise JSX onScroll. Manual interaction exposed that the compiler delegated this non-bubbling event to body; compiler#1 binds it to the target. The rebuilt Windows app with all four changes appeared fixed in manual retest. Merge this PR with that compiler dependency in mind.

Summary by CodeRabbit

  • Bug Fixes
    • Improved Win32 scrolling accuracy by keeping scroll positions consistent between device pixels and layout units.
    • Kept scroll-container documents anchored at the top while preserving scrolling behavior for other content.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: a321c6b0-82da-40b5-af38-ba758a134cf4

📥 Commits

Reviewing files that changed from the base of the PR and between 67cb15f and 1911c6d.

📒 Files selected for processing (2)
  • packages/geastack-windows/targets/win32/main/win32_renderer.cpp
  • packages/geastack-windows/targets/win32/main/win32_widgets.cpp

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Win32 scroll handling now converts positions between native device pixels and tree layout units. Scroll-container documents with nonnegative node IDs remain vertically positioned at zero when scrollbars or scroll positions are updated.

Changes

Win32 scroll synchronization

Layer / File(s) Summary
Scroll coordinate conversion
packages/geastack-windows/targets/win32/main/win32_renderer.cpp
The renderer converts native scroll positions to layout units before updating the tree. It converts tree scroll positions to device pixels before updating the widget.
Scroll-container document offsets
packages/geastack-windows/targets/win32/main/win32_widgets.cpp
For widgets with nonnegative node IDs, document vertical positioning remains at zero when scrollbars or scroll positions are updated. Other widgets retain the negative scroll-top offset.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: dashersw

Merge Risk: ⚪ Minimal · up to 1911c

Framework scroll positions and native scrollbar positions are synchronized in their respective units while preserving Controls offsets. No concrete regression remains that should block merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: synchronizing native Win32 scroll state with the UI tree.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@dogukani
dogukani merged commit 0ef3782 into main Sep 24, 2026
3 checks passed
@dogukani
dogukani deleted the fix/native-scroll-state branch September 24, 2026 18:56
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant