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

Setting Background to null will update rendering on Android and iOS #24726

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

spadapet
Copy link
Contributor

@spadapet spadapet commented Sep 11, 2024

Description of Change

When Background was set to null for some ViewElements, the Background of the platform element wasn't updated at all for Android and iOS. I just removed the "if" check that skipped setting Background to null.

Issues Fixed

Fixes #24725
Fixes #22914

(maybe more if you look at similar bugs linked in those bugs)

@spadapet spadapet self-assigned this Sep 11, 2024
@spadapet spadapet requested a review from a team as a code owner September 11, 2024 22:40
Copy link
Member

@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.

Yea, this is somewhat intentional, but we didn't really stay consistent here with WinUI unfortunately which makes this confusing.

The one problem with null is that it's somewhat undefined. Like, when you set the xplat background to null what does that mean?

For WinUI when you set it to null it basically reverts the style back to the templated style. This choice is a bit random because in WinUI land setting a background to "null" actually has a different meaning then reverting it or setting it to transparent. I wish we never would have done this on WinUI because now it's just confusing.

So, in theory for Android and iOS if we were to stay consistent,setting to null would mean revert to platform defaults (I haven't quite looked here if that's what you're doing)

It becomes really, really hard to define what "null" means across the spectrum of all properties you can possibly set and then trying to extract those from the platform.

Realistically if you want to "Clear" a property what you should do is call "ClearValue" on the property at which point it will revert back to whatever your style is for that property.

We haven't really seen a lot of movement on this being a problem for users that "null" is essentially a noop, so we haven't really made an effort to make "null" do anything.

@spadapet
Copy link
Contributor Author

spadapet commented Sep 11, 2024

Yea, this is somewhat intentional, but we didn't really stay consistent here with WinUI unfortunately which makes this confusing.

@PureWeen thanks for looking, I understand more now.

I think in the code I'm changing, the final computed Background brush is being used. So it's not just what the Label has directly set on it, but it takes applied styles into account.

My change is just expanding a previous PR from @rmarinho :

That PR limited the fix to Layout, I made it include everything.

I started looking into this based on customer feedback. A customer showed me a bug where he was using XAML Hot Reload, typed Background="Yellow" on a Label, then deleted it. But the Label stayed yellow. The same bug applies to ContentPage, Border, etc.

Plus my bug is a dupe of another customer's bug:

So there are some customers hitting the bug. I just don't know all of the little details of background=null. If this PR doesn't get in, then I could add a workaround in XAML Hot Reload, but that is not ideal since it involves rebuilding the element from scratch.

@mattleibow mattleibow added the area-theme Themes, theming label Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-theme Themes, theming
Projects
None yet
3 participants