fix: ScrollableContainerNode ignores bounds.Y, overwriting layout above it#301
Merged
Conversation
Fixes #300 — ScrollableContainerNode was passing the raw outer context directly to ScrolledRenderContext, causing content to always render at absolute terminal row 0. This overwrote any layout content above the scrollable node (headers, labels, etc.). The fix calls context.CreateSubContext(bounds) first, consistent with the pattern used by every other node (PanelNode, StreamingTextNode, SelectionListNode, etc.), so inner writes at row 0 actually land at bounds.Y on screen. Also includes 6 regression tests verifying: - Content renders at correct row when scrollable is below a header - Scrolling still works correctly with the bounds-relative fix - The PanelNode workaround from #300 still works - Scrollbar draws in the correct bounds - Single-node-at-row-0 still renders correctly
…uard
The previous version called ScrollDown() before Measure(), so MaxScroll was 0
and CanScrollDown was false — the three ScrollDown() calls were no-ops and
_scrollOffset stayed 0. The assertion Contains("Line4") then passed trivially
(Line4 is already visible at offset 0) and would have passed against the old
buggy code too.
Now: Measure() first so the scroll takes effect, pin AutoScroll to None so the
offset comes from ScrollDown() (not the auto-tail policy), assert ScrollOffset
== 3, and assert the viewport actually shifted (Line8 visible, Line1/Line2
scrolled out). Verified this fails against the pre-fix behavior (header row 0 is
overwritten by scrolled content).
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.
Problem
ScrollableContainerNode.Render()passes the raw outer context directly toScrolledRenderContext, causing content to always render at absolute terminal row 0 regardless of the node's actual position in the layout tree. This overwrites any content above it (headers, labels, etc.).Root cause: Every other node calls
context.CreateSubContext(bounds)first, which offsets the write origin tobounds.{X,Y}.ScrollableContainerNodewas the outlier that skipped this step.Fix
Call
context.CreateSubContext(bounds)before constructingScrolledRenderContext, consistent with the pattern used by every other node:Also updated the scrollbar drawing to use
boundsContextinstead of the rawcontext.Testing
ScrollableContainerNodeBoundsTests.cs:Render_AtOffset_PutsContentAtCorrectRow— core fix testRender_InVerticalLayout_PreservesHeaderAboveScrollable— full layout treeRender_ScrollShiftsContentWithinBounds— scrolling still worksRender_WrappedInPanelNode_WorkaroundStillWorks— the workaround still worksRender_AtRow0_NoHeaderAbove_RendersContent— edge case: no content aboveRender_WithScrollbar_DrawsScrollbarInBounds— scrollbar position correctCloses #300