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.

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

ContentSizer sets Width to 0 if layout hasn't completed #85

Closed
michael-hawker opened this issue Jun 12, 2023 · 2 comments · Fixed by #86
Closed

ContentSizer sets Width to 0 if layout hasn't completed #85

michael-hawker opened this issue Jun 12, 2023 · 2 comments · Fixed by #86
Labels
bug Something isn't working

Comments

@michael-hawker
Copy link
Member

Related to #26

Discovered as part of work on DataTable experiment in labs in this branch/commit here: CommunityToolkit/Labs-Windows@47f46df#diff-b874aea5366fbeacbd08eec0606a72bf3b458be31ff250be121b9c9f5ec7b49d

Basically, when the target control was being set in this scenario, the layout hadn't occurred yet on the, so the DesiredSize.Width is still '0'; which means the Width of the component is set to 0 and is never updated or changed making it invisible.

Instead, if the DesiredSize is not set yet, then we shouldn't manipulate the Width of the control, as the patch above did.

We set the Width property is that's modified by the sizing behavior:

protected override bool OnDragHorizontal(double horizontalChange)
{
if (TargetControl == null)
{
return true;
}
horizontalChange = IsDragInverted ? -horizontalChange : horizontalChange;
if (!IsValidWidth(TargetControl, _currentSize + horizontalChange, ActualWidth))
{
return false;
}
TargetControl.Width = _currentSize + horizontalChange;

However, when we start to drag we measure the ActualWidth (or Height) here:

protected override void OnDragStarting()
{
if (TargetControl != null)
{
_currentSize =
Orientation == Orientation.Vertical ?
TargetControl.ActualWidth :
TargetControl.ActualHeight;
}
}

So, in theory we may just be able to remove this initial code all together?

@michael-hawker michael-hawker added the bug Something isn't working label Jun 12, 2023
@michael-hawker
Copy link
Member Author

michael-hawker commented Jun 12, 2023

Was able to write a test which I think demonstrates this issue:

image

Ah, and I found the key comment too:

// as if it's NaN we won't be able to manipulate it.

So that's why we need to set the Width somewhere... I think I'll do this check in the OnDragStarting though instead? As then layout should be complete as user manipulation is occuring.

@michael-hawker
Copy link
Member Author

I wonder if the original code re-used the Width field over the ActualWidth calculated from layout? As testing without setting it initially seems to be working fine... And the tests...pass

image

Basically, this should be better behavior, as now the ContentSizer doesn't change any initial behavior until it is manipulated by the user.

Opening a PR... :)

michael-hawker added a commit that referenced this issue Jun 12, 2023
ContentSizer was overwriting Width to Zero causing initial layout problems of a component, instead this code is removed as it is not needed to maintain the same behavior. Added a test to validate the old behavior was incorrect and the new desired behavior should work.
michael-hawker added a commit that referenced this issue Jun 12, 2023
ContentSizer was overwriting Width to Zero causing initial layout problems of a component, instead this code is removed as it is not needed to maintain the same behavior. Added a test to validate the old behavior was incorrect and the new desired behavior should work.
michael-hawker added a commit that referenced this issue Jun 13, 2023
ContentSizer was overwriting Width to Zero causing initial layout problems of a component, instead this code is removed as it is not needed to maintain the same behavior. Added a test to validate the old behavior was incorrect and the new desired behavior should work.
@github-project-automation github-project-automation bot moved this to ✅ Done in Toolkit 8.x Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant