Fix negative-width header clip Rect in TableViewHeaderRow.ArrangeOverride (#437) - #438
Open
SolidRockProgrammer wants to merge 1 commit into
Open
SolidRockProgrammer wants to merge 1 commit into
SolidRockProgrammer wants to merge 1 commit into
Conversation
TableViewHeaderRow.ArrangeOverride clips the scrollable headers panel to
Rect(xClip, 0, panel.ActualWidth - xClip, finalSize.Height)
where xClip = (headersOffset * -1) + frozenOffset reduces algebraically to
TableView.HorizontalOffset. The enclosing guard only checks that the panel
has been measured (ActualWidth > 0); nothing keeps ActualWidth - xClip
positive, and a non-positive width is not a valid Rect.
It is reachable as a steady state whenever the frozen columns are wider than
the viewport: the scroll viewer's extent covers every column, so scrolling
to the end leaves HorizontalOffset past the scrollable headers panel's own
(much smaller) extent. On WinUI that throws straight out of the arrange
pass:
System.ArgumentOutOfRangeException: Non-negative number required.
(Parameter 'width')
at Windows.Foundation.Rect..ctor(Single x, Single y, Single width, ...)
at WinUI.TableView.TableViewHeaderRow.ArrangeOverride(Size finalSize)
On Uno the same construction is accepted verbatim (its Rect allows negative
width by default), and the resulting clip lands outside the panel, so every
header is hidden at the band's full height instead.
Clamp xClip into [0, ActualWidth] - the same Math.Clamp treatment
TableView.OnScrollContentPresenterPointerWheelChanged already gives the
offset - so the geometry is always well formed. Behaviour is unchanged
whenever the offset is inside the panel's extent, which is every ordinary
scroll position.
Adds TableViewHeaderRowTests: one test drives the offset past the panel's
extent and asserts the clip stays a valid rectangle (it throws the exception
above without this change), one pins the ordinary in-extent clip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new UI tests load content into the test window but don’t unload it, which can cause cross-test interference/flakiness in the test suite.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes a layout/clip crash (WinUI) and rendering failure (Uno) in TableViewHeaderRow.ArrangeOverride by ensuring the scrollable header clip rectangle never has a negative width when the horizontal offset exceeds the scrollable headers panel extent.
Changes:
- Clamp the computed clip X into
[0, _scrollableHeadersPanel.ActualWidth]to guarantee a well-formedRect. - Add UI tests that validate the clip stays valid when offset exceeds extent, and remains unchanged for normal in-range scrolling.
| File | Description |
|---|---|
| src/TableViewHeaderRow.cs | Clamps header clip geometry inputs to prevent negative-width Rect during arrange. |
| tests/TableViewHeaderRowTests.cs | Adds regression tests covering out-of-range and in-range header clipping behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+15
to
+35
| [UITestMethod] | ||
| public async Task HeaderClip_StaysAValidRect_WhenOffsetExceedsTheHeadersPanelExtent() | ||
| { | ||
| // Frozen columns wider than the viewport let HorizontalOffset run past the scrollable | ||
| // headers panel's own extent, which is what drives the clip width below zero. | ||
| var tableView = await CreateTableViewAsync(frozenColumnCount: 2); | ||
| var panel = GetScrollableHeadersPanel(tableView); | ||
|
|
||
| tableView.SetValue(TableView.HorizontalOffsetProperty, 5000d); | ||
| tableView.UpdateLayout(); | ||
|
|
||
| Assert.IsTrue(tableView.HorizontalOffset > panel.ActualWidth, | ||
| $"Precondition: the offset ({tableView.HorizontalOffset}) is past the panel's extent ({panel.ActualWidth})"); | ||
|
|
||
| var clip = panel.Clip; | ||
| Assert.IsNotNull(clip, "A clip is applied once the headers are scrolled"); | ||
| Assert.IsTrue(clip!.Rect.Width >= 0, | ||
| $"Header clip width must never be negative (was {clip.Rect.Width})"); | ||
| Assert.IsTrue(clip.Rect.X >= 0 && clip.Rect.X <= panel.ActualWidth, | ||
| $"Header clip X must stay inside the panel's extent (was {clip.Rect.X} for a panel {panel.ActualWidth} wide)"); | ||
| } |
This branch has not been deployed
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.
Description
TableViewHeaderRow.ArrangeOverrideclips the scrollable headers panel towhere
xClip = (headersOffset * -1) + frozenOffsetreduces algebraically toTableView.HorizontalOffset. The guard on the enclosingifonly checks that the panel has been measured (ActualWidth > 0); nothing keepsActualWidth - xClippositive, and a non-positive width is not a validRect.The two inputs come from different places —
HorizontalOffsetis read live off the scroll viewer,ActualWidthis the scrollable headers panel's own extent — so there is no invariant tying one to the other. With frozen columns wider than the viewport it is a steady state: the scroll viewer's extent covers every column, frozen ones included, so scrolling to the end leaves the offset past the scrollable panel's much smaller extent. It is also reachable transiently by narrowing a column while the view is scrolled right.What happens then differs by platform, and neither outcome is good:
Windows.Foundation.Rect's constructor validates, so the arrange pass throws:Rectallows negative width by default, so the invalid rectangle is stored verbatim, the clip resolves to a region outside the panel, and every column header disappears while the header band keeps its full height. No exception, no error — just a blank band over rows that render normally.The change
Clamp
xClipinto[0, ActualWidth]— the sameMath.ClamptreatmentTableView.OnScrollContentPresenterPointerWheelChangedalready gives the offset — so the geometry is always well formed. Behaviour is unchanged for every offset inside the panel's extent, which is every ordinary scroll position.Tests
New
tests/TableViewHeaderRowTests.cs:HeaderClip_StaysAValidRect_WhenOffsetExceedsTheHeadersPanelExtent— two frozen columns wider than the viewport, scroll to the end, assert the clip is still a valid rectangle inside the panel's extent. Without the change this test fails with theArgumentOutOfRangeExceptionabove (verified by reverting only the clamp, confirming the reverted IL in the built assembly, and re-running).HeaderClip_IsUnchanged_WhenScrolledWithinTheHeadersPanelExtent— pins the ordinary case: clipXis the offset, clip width is the unscrolled remainder.Full suite on this branch: 168/168 passed (
vstest.consoleagainstWinUI.TableView.Tests.build.appxrecipe, x64 Debug,net9.0-windows10.0.19041.0).Related Issue
Closes #437
Type of Change
Checklist
mainbranchAdditional Notes
On the Uno checkbox, precisely:
src/WinUI.TableView.csprojbuilds clean fornet10.0(the Uno target) with this change, and the Uno-side behaviour described above was measured by reading the shippedUno.Foundation.dll—Windows.Foundation.Rect's constructor only validates whenUno.FoundationFeatureConfiguration.Rect.AllowNegativeWidthHeightisfalse, and it defaults totrue. The test project targetsnet9.0-windows10.0.19041.0only, so the suite itself has not been run on an Uno head.The branch is cut from the
v1.4.1tag rather thanmain— that is the build these measurements were taken against, andsrc/TableViewHeaderRow.cs'sArrangeOverrideis byte-identical onmain, so it applies unchanged.On the Uno side the clamp makes the geometry valid and stops the silent invalid-rect path; it does not by itself repaint headers for an offset that is genuinely past the panel's extent (the clip is then degenerate rather than invalid). Deciding what should be shown in that state looks like a separate question, so this change is deliberately limited to keeping the rectangle well formed.
🤖 Generated with Claude Code