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

fix(netstd): late template application is ignored #18944

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

Xiaoy312
Copy link
Contributor

GitHub Issue (If applicable): closes unoplatform/add-private#18

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Setting a value to Control.Template has no effect once the template materialization had occured.

What is the new behavior?

^ no more.

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

This regression was introduced with the life cycle refactor(ac1b54c) that was bundled into templated-parent refactor (#17645).

@jeromelaban
Copy link
Member

@Mergifyio backport release/stable/5.5

Copy link
Contributor

mergify bot commented Nov 27, 2024

backport release/stable/5.5

✅ Backports have been created

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18944/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18944/index.html

@Xiaoy312 Xiaoy312 force-pushed the dev/xygu/20241127/template-reapplication branch from 669a584 to 2293898 Compare November 29, 2024 03:33
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18944/index.html

@@ -207,7 +207,30 @@ public ControlTemplate Template

private protected virtual void OnTemplateChanged(DependencyPropertyChangedEventArgs e)
{
#if !UNO_HAS_ENHANCED_LIFECYCLE
#if UNO_HAS_ENHANCED_LIFECYCLE
Copy link
Member

Choose a reason for hiding this comment

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

IF we are making template application closer and closer to WinUI on Skia and Wasm, then I think TemplateProperty should have AffectsMeasure flag.

We are not 100% matching WinUI though, so I'm not sure if the flag can have a bad effect currently.

But basically after the child removal, the way the template should re-apply is through the template application that happens during measure, which is why WinUI has AffectsMeasure.

Copy link
Member

Choose a reason for hiding this comment

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

In theory, adding a template will automatically incur a measure invalidation, since it's adding a frameworkelement to the tree that will itself request a measure.

Copy link
Member

@Youssef1313 Youssef1313 Nov 29, 2024

Choose a reason for hiding this comment

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

@jeromelaban I don't think that's correct.

Changing the template doesn't directly add a FrameworkElement to the visual tree. The template is applied (or should be applied) during measure and this is when the child is added to the tree (we do it during measure for Skia/Wasm already).

At least, this is how it works in WinUI. And as we are starting to align it with WinUI, we should do the same.

And yes, we are now removing a child which will likely invalidate measure anyways, but there can be really very weird corner cases where we still need to invalidate measure (e.g, first child is null already somehow but measure was done already), I think. It's safer to align with WinUI code anyways. Note that this can never be an extra invalidation. If the new code added here already removed a child, then invalidate measure will happen anyway. If it didn't remove a child in some edge case, then the only way for the template to be applied is to invalidate the measure. So it's safe and more correct to have AffectsMeasure on TemplateProperty (at least on Skia and Wasm. The other platforms do very weird things)

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18944/index.html

@jeromelaban jeromelaban merged commit 2f3ac18 into master Dec 4, 2024
112 of 114 checks passed
@jeromelaban jeromelaban deleted the dev/xygu/20241127/template-reapplication branch December 4, 2024 15:50
jeromelaban added a commit that referenced this pull request Dec 11, 2024
…5.5/pr-18944

fix(netstd): late template application is ignored (backport #18944)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants