Fix scrollbar padding accumulation + DimAuto content size drift (#4522)#4864
Closed
duskfold wants to merge 7 commits intogui-cs:v2_developfrom
Closed
Fix scrollbar padding accumulation + DimAuto content size drift (#4522)#4864duskfold wants to merge 7 commits intogui-cs:v2_developfrom
duskfold wants to merge 7 commits intogui-cs:v2_developfrom
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add 7 tests in StaleContentSizeCaptureTests that exercise the stale content size capture bug in LayoutSubViews: - Test 1: Core stale capture via OnSubViewLayout (regression) - Test 2: Dialog resize scenario (regression) - Test 3: Dialog OnSubViewLayout divergence (FAILS - proves bug) - Test 4: NeedsLayout direct set propagation bypass (regression) - Test 5: SubViewsLaidOut SetContentSize too late (regression) - Test 6: ListView OnViewportChanged content size (regression) - Test 7: TableView RefreshContentSize (regression) Test 3 demonstrates the core divergence: LayoutSubViews captures contentSize before OnSubViewLayout fires, so Dialog.UpdateSizes() changes content size from (57,16) to (80,35) but children were laid out with the stale (57,16) value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Re-read content size after OnSubViewLayout to ensure updates from SetContentSize are reflected before laying out children. Dialog content area now always floors at Viewport size, preventing it from shrinking below visible bounds. Added documentation of the issue and verification steps.
Refactored the Thickness struct for modern C# style and maintainability. Converted methods and properties to expression-bodied members, collapsed multi-line property definitions, and removed unnecessary null-conditional operators. Updated the Draw method for early returns and clearer logic. Ensured code style consistency with project conventions. No functional changes.
…cs#4522) Two related bugs causing Dialog content to shrink by 2px per maximize/restore cycle: 1. ScrollBar.VisibleChanged handlers in View.ScrollBars.cs used relative Padding.Thickness +1/-1 adjustments. During layout re-entry, ShowHide fires multiple times per visibility change, compounding the adjustment. Each cycle added +1 to Padding.Right (vertical scrollbar) and +1 to Padding.Bottom (horizontal scrollbar), permanently inflating adornment thickness. Fix: Track whether scrollbar padding has been applied via boolean flags and make VisibleChanged handlers idempotent. The +1 is only applied when transitioning from not-applied to applied, and -1 only when transitioning back. 2. Dialog.UpdateSizes used _minimumSubViewsSize as a floor for ContentSize regardless of whether the dialog uses DimAuto. For fixed-size dialogs, this floor captures the high-water mark from the maximized state and never shrinks, inflating ContentSize beyond the actual Viewport on restore. Fix: Only use _minimumSubViewsSize as a floor for DimAuto dialogs (which need to grow to fit content). Fixed-size dialogs use Viewport size directly. Traced and verified with instrumented builds: Padding.Bottom now oscillates cleanly (3 -> 4 -> 3) on every cycle instead of ratcheting (3 -> 6 -> 8 -> 10 -> ...). Tests: Added ScrollBarPaddingAccumulationTests with two tests covering the resize-cycle and idempotency scenarios.
BDisp
approved these changes
Mar 28, 2026
Member
|
Thanks. I fixed this in #4862. |
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
4 participants
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.
Builds on top of @tig's #4863 (stale ContentSize re-read). That fix is correct and necessary, but a second bug remained: repeated maximize/restore cycles caused Dialog content to shrink by 2px per cycle until the content area collapsed.
Root cause (traced with instrumented builds)
Two bugs working together:
1. ScrollBar padding accumulation (View.ScrollBars.cs)
The VisibleChanged handlers use relative Padding.Thickness +1/-1 adjustments. During layout re-entry (which happens on every resize), ShowHide fires multiple times per visibility change. Each fire adds another +1, and the -1 on hide doesn't fully compensate because the baseline has already shifted.
Trace data showing Padding.Bottom ratcheting before fix:
Fix: Track whether scrollbar padding has been applied via boolean flags (_verticalScrollBarPaddingApplied / _horizontalScrollBarPaddingApplied). The handlers become idempotent: +1 only fires when transitioning from not-applied to applied, -1 only when transitioning back.
Trace data after fix:
2. _minimumSubViewsSize floor in non-DimAuto dialogs (DialogTResult.cs)
UpdateSizes unconditionally floors ContentSize at _minimumSubViewsSize. For fixed-size dialogs (non-DimAuto), this captures the high-water mark from the maximized state and never shrinks back, inflating ContentSize beyond the actual Viewport on restore.
Fix: Only use _minimumSubViewsSize as a floor for DimAuto dialogs (which need to grow to fit content). Fixed-size dialogs use Viewport size directly.
Tests
Added ScrollBarPaddingAccumulationTests.cs with two tests:
All 7 existing StaleContentSizeCaptureTests continue to pass.
How we found it
Instrumented DialogTResult.cs and AdornmentImpl.cs with file-based tracing (Padding.Thickness setter logged every change with full stack traces). The stack traces showed ScrollBar.ShowHide -> ConfigureVerticalScrollBarEvents/ConfigureHorizontalScrollBarEvents -> Padding.Thickness += 1 firing during PaddingView.OnParentFrameChanged on every layout re-entry cycle.