Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GridSplitter with '1*'-<sized>-<sized> columns does not resize as expected #339

Closed
4 of 24 tasks
abdes opened this issue Feb 19, 2024 · 4 comments · Fixed by #342
Closed
4 of 24 tasks

GridSplitter with '1*'-<sized>-<sized> columns does not resize as expected #339

abdes opened this issue Feb 19, 2024 · 4 comments · Fixed by #342
Assignees
@abdes

Comments

@abdes
Copy link
Contributor

abdes commented Feb 19, 2024 •
edited
Loading

Describe the bug

"CommunityToolkit.WinUI.Controls.Sizers" Version="8.0.240109"

Consider a grid with 3 columns and 2 splitters as following:

    <Grid
        VerticalAlignment="Stretch"
        BorderBrush="{ThemeResource SystemControlHighlightChromeHighBrush}"
        BorderThickness="0,0,1,1">
        <Grid.ColumnDefinitions>
            <ColumnDefinition Width="300*" MinWidth="100" />
            <ColumnDefinition Width="Auto" MinWidth="6" />
            <ColumnDefinition Width="300" MinWidth="32" />
            <ColumnDefinition Width="Auto" MinWidth="6" />
            <ColumnDefinition Width="300" MinWidth="32" />
        </Grid.ColumnDefinitions>

        <Border
            Grid.Row="0"
            Grid.Column="0"
            Background="MidnightBlue">
            <TextBlock Text="This text is in a column with 1* Width - This is what causes the second splitter not to work." TextWrapping="Wrap" />
        </Border>
        <Border Grid.Row="0" Grid.Column="2">
            <TextBlock Text="This column resizes properly, and if it's the only one after the 1* column, it also resizes properly." TextWrapping="Wrap" />
        </Border>
        <Border
            Grid.Row="0"
            Grid.Column="4"
            Background="DarkRed">
            <TextBlock Text="This text Is not resizing - Grid.Column.Width = 300 - ResizeBehavior=PreviousAndNext" TextWrapping="Wrap" />
        </Border>

        <!--  Column Grid Splitter  -->
        <controls:GridSplitter
            Grid.Column="1"
            Width="16"
            ResizeBehavior="PreviousAndNext"
            ResizeDirection="Auto" />

        <!--  Row Grid Splitter  -->
        <controls:GridSplitter
            Grid.Column="3"
            Width="16"
            ResizeBehavior="PreviousAndNext"
            ResizeDirection="Auto" />
    </Grid>

Splitter at Column 1 works fine, but splitter at Column 3 does not resize Column 4 as expected.

Steps to reproduce

Run the attached sample.

GridSplitterBug.zip

Expected behavior

Given that the splitter is configured to resize PreviousAndNext, it is supposed to resize both columns.

Screenshots

GridSplitter.Bug.2024-02-19.222346.mp4

Code Platform

  • UWP
  • WinAppSDK / WinUI 3
  • Web Assembly (WASM)
  • Android
  • iOS
  • MacOS
  • Linux / GTK

Windows Build Number

  • Windows 10 1809 (Build 17763)
  • Windows 10 1903 (Build 18362)
  • Windows 10 1909 (Build 18363)
  • Windows 10 2004 (Build 19041)
  • Windows 10 20H2 (Build 19042)
  • Windows 10 21H1 (Build 19043)
  • Windows 10 21H2 (Build 19044)
  • Windows 10 22H2 (Build 19045)
  • Windows 11 21H2 (Build 22000)
  • Other (specify)

Other Windows Build number

Any version of windows

App minimum and target SDK version

  • Windows 10, version 1809 (Build 17763)
  • Windows 10, version 1903 (Build 18362)
  • Windows 10, version 1909 (Build 18363)
  • Windows 10, version 2004 (Build 19041)
  • Windows 10, version 2104 (Build 20348)
  • Windows 11, version 22H2 (Build 22000)
  • Other (specify)

Other SDK version

No response

Visual Studio Version

2022

Visual Studio Build Number

latest

Device form factor

Desktop

Additional context

<ItemGroup>
	<PackageReference Include="CommunityToolkit.WinUI.Controls.Sizers" Version="8.0.240109" />
	<PackageReference Include="Microsoft.WindowsAppSDK" Version="1.4.230913002" />
	<PackageReference Include="Microsoft.Windows.SDK.BuildTools" Version="10.0.22621.2428" />
</ItemGroup>

Help us help you

Yes, I'd like to be assigned to work on this item.
@michael-hawker
Copy link
Member

Thanks for the detailed report @abdes and offering to dig in more, a good place to start debugging is probably in these methods here: https://github.com/CommunityToolkit/Windows/blob/main/components/Sizers/src/GridSplitter/GridSplitter.Events.cs

Could be something with getting the right column and bounds checks, or could be something with the logic of determining if the column is resizable? Would have suspected for us to see this before, but I think normally there's usually a single grid-splitter in a two-column setup, so probably why we haven't seen it before.

Thanks, I've assigned this to you for now, let us know if you need any help!

@abdes
Copy link
Contributor Author

abdes commented Feb 23, 2024 •
edited
Loading

The problem is indeed in the way the GridSplitter decides how to resize the current and sibling columns. When both are not Star items, and the vector contains another Star item (i.e. my scenario), the GridSplitter only resizes the current item, which makes the delta get applied to the Star item and not to the sibling.

In a scenario with only 2 items and a splitter, this does not show, because the delta gets automatically applied by the grid layout to the sibling.

If you look at the code at

Windows/components/Sizers/src/GridSplitter/GridSplitter.Events.cs

Lines 53 to 74 in f9ce94a

// if current row has fixed height then resize it
if (!IsStarRow(CurrentRow))
{
// No need to check for the row Min height because it is automatically respected
return SetRowHeight(CurrentRow, currentChange, GridUnitType.Pixel);
}
// if sibling row has fixed width then resize it
else if (!IsStarRow(SiblingRow))
{
// Would adding to this column make the current column violate the MinWidth?
if (IsValidRowHeight(CurrentRow, currentChange) == false)
{
return false;
}
return SetRowHeight(SiblingRow, siblingChange, GridUnitType.Pixel);
}
// if both row haven't fixed height (auto *)
else
{

We can see that the method bails out immediately after changing the first row with fixed size (same applies to OnDragHorizontal for columns). It should not do that. It should also check if the other row is also fixed size and apply the change to it in order to ensure that the delta does not get applied to another Star item in the vector.

@michael-hawker I submitted a PR with a fix, please check it.

abdes added a commit to abdes/Windows that referenced this issue Feb 23, 2024
@abdes
fix: resize both current and sibling when they are both fixed size
1ad3d47 
Fixes CommunityToolkit#339
If the vector contains another item with Star sizing, it's not enough
to just change current. The change will flow to the Star sized item
and not to the sibling if the sibling is fixed-size. We need to explicitly
apply the change to the sibling.
abdes added a commit to abdes/Windows that referenced this issue Feb 23, 2024
@abdes
fix: resize both current and sibling when they are both fixed size
8dd6176 
Fixes CommunityToolkit#339
If the vector contains another item with Star sizing, it's not enough
to just change current. The change will flow to the Star sized item
and not to the sibling if the sibling is fixed-size. We need to explicitly
apply the change to the sibling.
@abdes abdes mentioned this issue Feb 23, 2024
Merged
11 tasks
abdes added a commit to abdes/Windows that referenced this issue Feb 24, 2024
@abdes
fix: check constraints before allowing the resize to proceed
c8c7cae 
Fixes CommunityToolkit#339
If the vector contains another item with Star sizing, it's not enough
to just change current. The change will flow to the Star sized item
and not to the sibling if the sibling is fixed-size. We need to explicitly
apply the change to the sibling.
Now also checks the constraints before allowing the resize.
@Jay-o-Way
Copy link

But, you expect two columns to resize, while they are both fixed sizes. That's a contradictory design.

@abdes
Copy link
Contributor Author

abdes commented Mar 8, 2024 via email •
edited
Loading

Arlodotexe pushed a commit that referenced this issue Mar 28, 2024
@abdes @Arlodotexe
fix: resize both current and sibling when they are both fixed size
8406f07 
Fixes #339
If the vector contains another item with Star sizing, it's not enough
to just change current. The change will flow to the Star sized item
and not to the sibling if the sibling is fixed-size. We need to explicitly
apply the change to the sibling.
Arlodotexe pushed a commit that referenced this issue Mar 28, 2024
@abdes @Arlodotexe
fix: check constraints before allowing the resize to proceed
3087994 
Fixes #339
If the vector contains another item with Star sizing, it's not enough
to just change current. The change will flow to the Star sized item
and not to the sibling if the sibling is fixed-size. We need to explicitly
apply the change to the sibling.
Now also checks the constraints before allowing the resize.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@abdes abdes

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@abdes @michael-hawker @Jay-o-Way