Skip to content

Conversation

@GID0317
Copy link
Contributor

@GID0317 GID0317 commented Sep 16, 2025

The stricter condition caused scenarios (like in the #343 (comment)) where an icon visually sat beside text that updated its brush, but the icon didn’t because the Parent/VisualParent relationship didn’t meet the extra inequality test. Removing Parent != VisualParent lets normal visual-parent brush changes flow more consistently.

After this change the Icons more reliably pick up ancestor Foreground (especially dynamic theme resources) when their own Foreground wasn’t explicitly set. Explicit Foreground still opt‑out automatically. No public API change.

The impact of change so far was very low. But it can be investigated more if it breaks something

Refactor foreground inheritance logic in IconElement.
Updated foreground handling in FontIcon to use data binding instead of manual property assignments. Removed legacy foreground inheritance logic.
@GID0317 GID0317 changed the title Gallery Fix: font icons theming bug [Enchantment] font icons theming theme aware bug fix Sep 17, 2025
@NotYoojun NotYoojun self-requested a review October 5, 2025 06:09
Copy link
Member

@NotYoojun NotYoojun left a comment

Choose a reason for hiding this comment

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

Thanks for working on this fix! I noticed that the original implementation had a specific Foreground inheritance mechanism (ShouldInheritForegroundFromVisualParent + manual propagation). With the new binding-based approach, it looks like that inheritance logic might no longer behave the same way—for example, when the control’s own Foreground is explicitly set vs. when it should inherit from its visual parent.

My concern is that this change could break scenarios where the icon was expected to follow its parent’s Foreground automatically. Could you clarify whether this new binding approach still preserves the original inheritance behavior, or if there are cases where it now diverges?

@GID0317
Copy link
Contributor Author

GID0317 commented Oct 5, 2025

Thanks for the review. Yes, inheritance is preserved. If FontIcon.Foreground is unset, the glyph inherits/updates from its visual parent. if set, it uses that value. The only change is we no longer forcibly copy VisualParentForeground in rare edge cases where the control itself wouldn’t inherit.

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.

2 participants