-
Notifications
You must be signed in to change notification settings - Fork 742
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
Conversation
@Mergifyio backport release/stable/5.5 |
✅ Backports have been created
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18944/index.html |
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18944/index.html |
669a584
to
2293898
Compare
🤖 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18944/index.html |
…5.5/pr-18944 fix(netstd): late template application is ignored (backport #18944)
GitHub Issue (If applicable): closes unoplatform/add-private#18
PR Type
What kind of change does this PR introduce?
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:
Screenshots Compare Test Run
results.Other information
This regression was introduced with the life cycle refactor(ac1b54c) that was bundled into templated-parent refactor (#17645).