Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Move layout change resolution calls up to VisualElementRenderer #13640

Merged
merged 6 commits into from
Feb 5, 2021

Conversation

hartez
Copy link
Contributor

@hartez hartez commented Feb 3, 2021

Description of Change

The change to on-demand layout resolution in 5.0.0-pre2 moved the layout resolution to the layout renderers. Unfortunately, this meant that some ContentView uses (13492) and any custom renderer which replaced the layout renderers (13418) would not update their layout changes properly.

These changes move the layout resolution call to the base VisualElementRenderer class on each platform; this way, they encompass content views and any custom layout renderers.

Issues Resolved

API Changes

None

Platforms Affected

  • iOS
  • Android
  • UWP

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Unfortunately, this one is hard to write a test for without breaking all of Control Gallery. Best we can do is load up the repro projects from #13418 and #13492, apply the NuGet package from this PR, and verify that the problems are fixed.

For #13551 there is an automated UI test.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@hartez hartez added this to To Fix in 5.0.0 SR 3 via automation Feb 3, 2021
@hartez hartez moved this from To Fix to PR Needs Review in 5.0.0 SR 3 Feb 3, 2021
@hartez hartez moved this from PR Needs Review to PR Build/Test Failures in 5.0.0 SR 3 Feb 3, 2021
@hartez hartez moved this from PR Build/Test Failures to PR Needs Review in 5.0.0 SR 3 Feb 3, 2021
@jsuarezruiz jsuarezruiz added a/layout i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often labels Feb 4, 2021
Copy link
Contributor

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PureWeen PureWeen moved this from PR Needs Review to PR Build/Test Failures in 5.0.0 SR 3 Feb 4, 2021
@hartez
Copy link
Contributor Author

hartez commented Feb 4, 2021

Same UITest across all iOS runs failed

https://dev.azure.com/xamarin/public/_releaseProgress?releaseId=2840&_a=release-environment-extension&environmentId=136487&extensionId=ms.vss-test-web.test-result-in-release-environment-editor-tab

Yeah, looks like this is somehow busting the default (text-only) cells. Looking into it.

@hartez hartez moved this from PR Build/Test Failures to PR Needs Review in 5.0.0 SR 3 Feb 5, 2021
@hartez
Copy link
Contributor Author

hartez commented Feb 5, 2021

Okay, all relevant tests are passing. iOS 12 lane just isn't running, the iOS 14 failures are the known ListView issues, and the Android stuff is just flaky tests.

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the nuget on the repos, and works as expected. I can see the controls now.

@PureWeen PureWeen merged commit 3d054b8 into 5.0.0 Feb 5, 2021
5.0.0 SR 3 automation moved this from PR Needs Review to Done Feb 5, 2021
@PureWeen PureWeen deleted the fix-13418 branch February 5, 2021 21:16
@samhouts samhouts added this to the 5.0.0 milestone Feb 12, 2021
@samhouts samhouts added a/collectionview i/regression t/bug 🐛 blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. 5.0.0 Regression on 5.0.0 labels Feb 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
5.0.0 Regression on 5.0.0 a/collectionview a/layout blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often i/regression t/bug 🐛
Projects
No open projects
5 participants