Fix implicit styles with BasedOn chains breaking in 10.0.20#33218
Fix implicit styles with BasedOn chains breaking in 10.0.20#33218StephaneDelcroix wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in .NET MAUI 10.0.20 where implicit styles using BasedOn chains with keyed styles stopped working. The issue was introduced by PR #32711 which fixed ApplyToDerivedTypes functionality.
Key Changes
- Changed merging strategy: Instead of creating wrapper styles that broke
BasedOnchains, the fix now stores multiple applicable implicit styles in a list and applies them sequentially - Preserved BasedOn chains: Each style's
BasedOnchain remains intact, ensuring property values from keyed styles are properly resolved - Maintained ApplyToDerivedTypes support: The fix continues to support multiple implicit styles from the type hierarchy while fixing the regression
| // Apply implicit styles from most base to most derived | ||
| // This ensures derived styles override base styles for conflicting properties | ||
| if (_applicableImplicitStyles != null && _applicableImplicitStyles.Count > 0) | ||
| { | ||
| // Apply in reverse order (most base first, most derived last) | ||
| // so derived styles override base styles | ||
| for (int i = _applicableImplicitStyles.Count - 1; i >= 0; i--) | ||
| { | ||
| //NOTE specificity could be more fine grained (using distance) | ||
| ((IStyle)_applicableImplicitStyles[i]).Apply(bindable, new SetterSpecificity(SetterSpecificity.StyleImplicit, 0, 0, 0)); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| //NOTE specificity could be more fine grained (using distance) | ||
| ImplicitStyle?.Apply(bindable, new SetterSpecificity(SetterSpecificity.StyleImplicit, 0, 0, 0)); | ||
| } |
There was a problem hiding this comment.
The logic for applying and unapplying multiple implicit styles is duplicated across multiple methods (Apply() at lines 113-127, UnApply() at lines 145-155, and SetStyle() at lines 254-264 and 275-286). Consider extracting this logic into private helper methods like ApplyImplicitStyles(BindableObject target) and UnApplyImplicitStyles(BindableObject target) to eliminate duplication and ensure consistency. This would improve maintainability and reduce the risk of introducing bugs when updating the style application logic in the future.
| // Set ImplicitStyle to the most derived style for compatibility | ||
| // (some code may check if ImplicitStyle is set) | ||
| ImplicitStyle = applicableStyles[0]; |
There was a problem hiding this comment.
Setting ImplicitStyle to the most derived style (applicableStyles[0]) when multiple applicable styles exist may lead to unexpected behavior or confusion. Code that checks if ImplicitStyle is set may not realize there are multiple styles being applied. Consider either:
- Setting ImplicitStyle to null when multiple styles are present to make it clear that the _applicableImplicitStyles list should be used instead
- Adding a property or method to indicate when multiple implicit styles are active
This would make the multi-style state more explicit and prevent potential misuse of the ImplicitStyle property.
| // Set ImplicitStyle to the most derived style for compatibility | |
| // (some code may check if ImplicitStyle is set) | |
| ImplicitStyle = applicableStyles[0]; | |
| // When multiple implicit styles are active, leave ImplicitStyle null | |
| // so callers know to consult _applicableImplicitStyles instead. | |
| ImplicitStyle = null; |
|
/rebase |
cfba365 to
6ac9db6
Compare
|
/rebase |
This fixes issue #33203 where implicit styles using BasedOn with keyed styles stopped working in 10.0.20. The regression was introduced by PR #32711 which fixed #9648 (ApplyToDerivedTypes not working for implicit styles). The problem was that when merging multiple applicable implicit styles, the code created wrapper styles that replaced the original BasedOn chain, losing the connection to keyed styles that defined the actual property values. The fix changes the approach: instead of creating wrapper styles that break BasedOn chains, we now store the list of applicable implicit styles and apply them sequentially during Apply(). This preserves each style's BasedOn chain intact while still supporting the ApplyToDerivedTypes functionality from #9648. Fixes #33203
Tests verify that: 1. Implicit styles with BasedOn to keyed styles work correctly 2. Derived implicit styles properly override base styles' properties
When a base style uses AppThemeBinding for a property and a derived style overrides that property with a static value, the AppThemeBinding would re-apply when theme changes or resources propagate, overwriting the derived style's value. Changes: - AppThemeBinding.Apply(fromTarget): Skip re-apply when value is set from target - AppThemeBinding.ApplyCore(): Check if current value was set by this binding before re-applying (using IsValueFromThisBinding helper) - MergedStyle: Apply graduated specificities for multiple implicit styles Added unit test Maui33203AppTheme to verify derived style setters override base style's AppThemeBinding in both Light and Dark modes.
6ac9db6 to
46d0b16
Compare
PureWeen
left a comment
There was a problem hiding this comment.
I'm pretty sure the failures on this one are legit UI test failures.
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…" (#33397) - [x] Revert `MergedStyle.cs` to original state (restore simple `OnImplicitStyleChanged()` method) - [x] Delete test file `Maui9648.xaml` - [x] Delete test file `Maui9648.xaml.cs` - [x] Delete benchmark file `ImplicitStylesBenchmark.cs` - [x] Add passing tests from PR #33218 for issue #33203 - [x] Remove failing AppTheme tests ### Description of Change Reverts the changes from #32711 which modified how implicit styles with `ApplyToDerivedTypes=true` are applied. The original behavior returns early with the first applicable implicit style instead of merging multiple styles from the type hierarchy. ### Issues Fixed Fixes #33203 ### Test Results **Maui33203 Tests (6/6 PASS):** - `ImplicitStyleWithBasedOnKeyedStyleWorks` - All 3 inflators PASSED ✅ - `DerivedImplicitStyleOverridesBaseStyle` - All 3 inflators PASSED ✅ <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > Revert this PR #32711 </details> <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/dotnet/maui/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
This fixes issue #33203 where implicit styles using
BasedOnwith keyed styles stopped working in 10.0.20.Root Cause
The regression was introduced by PR #32711 which fixed #9648 (
ApplyToDerivedTypesnot working for implicit styles). That fix created wrapper styles to merge multiple applicable implicit styles, but in doing so it replaced the originalBasedOnchain with a new one, losing the connection to keyed styles that defined the actual property values.The Fix
Instead of creating wrapper styles that break
BasedOnchains, we now store the list of applicable implicit styles and apply them sequentially duringApply(). This preserves each style'sBasedOnchain intact while still supporting theApplyToDerivedTypesfunctionality from #9648.Test Results
ApplyToDerivedTypesfix) still passBasedOn) now works correctlyIssues Fixed
Fixes #33203