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

ScrollView content needs layout when ScrollView.InvalidateMeasure is called #17639

Merged
merged 5 commits into from Nov 27, 2023

Conversation

spadapet
Copy link
Contributor

Description of Change

When ScrollView.Content changes its alignment (like HorizontalOptions change from Start to End), then the ScrollView's layout needs to properly update. It worked on Android and Mac/iOS, but Windows would not update layout until the window was resized.

After HorizontalOptions changed on the content, Windows would call InvalidateMeasure on the WinUI ScrollView, but the child content didn't get updated. This change makes sure that the content also gets measured.

This fix is helpful for XAML Hot Reload, so the layout will update as HorizontalOptions changes.

Issues Fixed

Fixes #14377

@spadapet spadapet added the partner/hot-reload-xaml Issues impacting XAML Hot Reload experiences label Sep 26, 2023
@spadapet spadapet requested a review from a team as a code owner September 26, 2023 00:23
@spadapet spadapet self-assigned this Sep 26, 2023
@spadapet spadapet closed this Sep 26, 2023
@spadapet spadapet deleted the dev/peterspa/ScrollViewLayoutInvalidateProperly branch September 26, 2023 16:32
@spadapet spadapet restored the dev/peterspa/ScrollViewLayoutInvalidateProperly branch September 26, 2023 16:40
@spadapet spadapet reopened this Sep 26, 2023
@spadapet
Copy link
Contributor Author

Ah! Deleted the wrong branch by accident. Good thing I could restore it and reopen this PR.

@Eilon Eilon added the area/layout 🔲 StackLayout, GridLayout, ScrollView, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Sep 26, 2023
@spadapet spadapet force-pushed the dev/peterspa/ScrollViewLayoutInvalidateProperly branch from 6688c8b to d443403 Compare September 26, 2023 21:19
@samhouts samhouts added this to the Under Consideration milestone Oct 5, 2023
@spadapet spadapet force-pushed the dev/peterspa/ScrollViewLayoutInvalidateProperly branch from d443403 to 1c05ee4 Compare October 9, 2023 17:05
@spadapet
Copy link
Contributor Author

Ping @PureWeen, @hartez for review

@hartez
Copy link
Contributor

hartez commented Nov 1, 2023

@spadapet I think you're on the right track, but the re-mapping of the InvalidateMeasure command could cause problems for users who've modified the base mapping. @PureWeen and @mattleibow and I are working out how we can handle this sort of Core-level remapping to avoid that problem (#17918).

In the meantime, I dropped the TestContentHorizontalOptionsChanged test into the main branch and ran it on Windows without the other code changes, and it passes. The bug seems to be real - if I put the same structure in an app, the horizontal options definitely aren't updating. Maybe the test is giving a false positive?

@spadapet
Copy link
Contributor Author

spadapet commented Nov 2, 2023

@spadapet I think you're on the right track, but the re-mapping of the InvalidateMeasure command could cause problems for users who've modified the base mapping. @PureWeen and @mattleibow and I are working out how we can handle this sort of Core-level remapping to avoid that problem (#17918).

In the meantime, I dropped the TestContentHorizontalOptionsChanged test into the main branch and ran it on Windows without the other code changes, and it passes. The bug seems to be real - if I put the same structure in an app, the horizontal options definitely aren't updating. Maybe the test is giving a false positive?

@hartez thanks for looking, I'll re-check the scenario with and without the fix. I'm pretty sure the test used to fail on Windows without the fix.

And this fix can wait for #17918 if necessary.

@spadapet spadapet force-pushed the dev/peterspa/ScrollViewLayoutInvalidateProperly branch from 1c05ee4 to 21bca04 Compare November 8, 2023 18:06
@spadapet
Copy link
Contributor Author

spadapet commented Nov 8, 2023

In the meantime, I dropped the TestContentHorizontalOptionsChanged test into the main branch and ran it on Windows without the other code changes, and it passes. The bug seems to be real - if I put the same structure in an app, the horizontal options definitely aren't updating. Maybe the test is giving a false positive?

@hartez I found out that if I added a delay in the test, then the bug does repro. Without the delay, it's too fast for the UI to render and the bug doesn't repro. Since that doesn't quite make sense to me yet, I'll push new changes once I understand that a little more.

@spadapet
Copy link
Contributor Author

spadapet commented Nov 8, 2023

@hartez, @PureWeen, my latest pushed changes are ready for review. Although I know the pending draft PR #17918 might affect my fix. If I have to wait for that other PR, let me know.

I feel weird adding an await Task.Delay into the test, but without that, the UI doesn't render and the bug doesn't repro. I'm probably missing some other better method call that allows rendering to happen. Suggestions?

@spadapet spadapet force-pushed the dev/peterspa/ScrollViewLayoutInvalidateProperly branch from bfc80f6 to 56264fb Compare November 15, 2023 19:58
@spadapet
Copy link
Contributor Author

@hartez, @PureWeen, any more comments?

@hartez
Copy link
Contributor

hartez commented Nov 17, 2023

@hartez, @PureWeen, any more comments?

Sorry, this week was overwhelmed by launch stuff, travel, and upcoming holidays.

@mattleibow and @PureWeen - any reason we can't take this as-is, and then update it ourselves pending the outcome of #17918?

@PureWeen
Copy link
Member

/rebase

@github-actions github-actions bot force-pushed the dev/peterspa/ScrollViewLayoutInvalidateProperly branch from 56264fb to 0f7f57f Compare November 21, 2023 19:16
@spadapet spadapet force-pushed the dev/peterspa/ScrollViewLayoutInvalidateProperly branch from 0f7f57f to 663277e Compare November 22, 2023 18:32
@PureWeen PureWeen enabled auto-merge (squash) November 22, 2023 18:38
@spadapet spadapet force-pushed the dev/peterspa/ScrollViewLayoutInvalidateProperly branch from 663277e to 4f6200f Compare November 27, 2023 19:09
auto-merge was automatically disabled November 27, 2023 20:17

Pull Request is not mergeable

@spadapet spadapet merged commit 34d73c8 into main Nov 27, 2023
47 checks passed
@spadapet spadapet deleted the dev/peterspa/ScrollViewLayoutInvalidateProperly branch November 27, 2023 21:50
@github-actions github-actions bot locked and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/layout 🔲 StackLayout, GridLayout, ScrollView, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter partner/hot-reload-xaml Issues impacting XAML Hot Reload experiences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[WinUI] ScrollView does not layout contents if they don't change in size.
5 participants