Skip to content

Fix: OnIdiom Default Value Handling for ValueType Properties - #28257

Closed
bhavanesh2001 wants to merge 4 commits into
dotnet:mainfrom
bhavanesh2001:onidiom_default_value
Closed

Fix: OnIdiom Default Value Handling for ValueType Properties#28257
bhavanesh2001 wants to merge 4 commits into
dotnet:mainfrom
bhavanesh2001:onidiom_default_value

Conversation

@bhavanesh2001

Copy link
Copy Markdown
Contributor

Description of Change

This PR fixes an issue, where OnIdiom without a default value incorrectly assigns 0 instead of the expected default.

Root Cause

  • OnIdiom relies on Activator.CreateInstance(propertyType) when value is null.
  • For value types like double Activator.CreateInstance() returns 0.0 , instead of the expected default value (-1) from BindableProperty.GetDefaultValue().

Issues Fixed

Fixes #9978

@bhavanesh2001
bhavanesh2001 requested a review from a team as a code owner March 9, 2025 10:59
@dotnet-policy-service dotnet-policy-service Bot added the community ✨ Community Contribution label Mar 9, 2025
@jsuarezruiz

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz jsuarezruiz added the area-xaml XAML, CSS, Triggers, Behaviors label Mar 10, 2025
@jsuarezruiz

Copy link
Copy Markdown
Contributor

/rebase

@github-actions
github-actions Bot force-pushed the onidiom_default_value branch from a6b928b to 4c196fa Compare March 28, 2025 09:24
@StephaneDelcroix

Copy link
Copy Markdown
Contributor

I wonder if OnIdiom without a default value shouldn't just crash, or at least warn...


[Issue(IssueTracker.Github, 9978, "VisualElement.HeightRequest defaults to 0 instead of -1 when using OnIdiom default value",
PlatformAffected.All)]
public partial class Issue9978 : ContentPage

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd need this test in Xaml.UnitTest so we test for all xaml inflators

@StephaneDelcroix StephaneDelcroix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fix test

@bhavanesh2001

Copy link
Copy Markdown
Contributor Author

I wonder if OnIdiom without a default value shouldn't just crash, or at least warn...

I don't think it's crashing. But it assigns 0 instead of -1, (for example HeightRequest) . Which makes the view not appear since height is 0

@StephaneDelcroix

Copy link
Copy Markdown
Contributor

I don't think it's crashing. But it assigns 0 instead of -1, (for example HeightRequest) . Which makes the view not appear since height is 0

y, it's not crashing. but I wonder if it shouldn't instead of behaving in an unexpected way

@bhavanesh2001

Copy link
Copy Markdown
Contributor Author

y, it's not crashing. but I wonder if it shouldn't instead of behaving in an unexpected way

This suggests that defining one platform without a default is allowed — same behavior as OnPlatform.

But yeah, I agree with you — probably shouldn’t behave this way.

{
if (Default == null
&& Phone == null
&& Tablet == null
&& Desktop == null
&& TV == null
&& Watch == null)
throw new XamlParseException("OnIdiomExtension requires a non-null value to be specified for at least one idiom or Default.", serviceProvider);

@bhavanesh2001

Copy link
Copy Markdown
Contributor Author

/rebase

@github-actions
github-actions Bot force-pushed the onidiom_default_value branch from 4c196fa to 94fa331 Compare May 8, 2025 17:38
@bhavanesh2001

Copy link
Copy Markdown
Contributor Author

@StephaneDelcroix I added a unit test. Please take a look. I kept the UItest, let me know if you want it removed.

}

[Test]
public void OnIdiomReturnsBindablePropertyDefaultWhenNotSet()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

look at what other tests are doing, in the Issues folder. it needs a xaml and a xaml.cs file. this doesn't test XamlC inflation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry about that — I hadn’t opened Xaml.UnitTests until a few hours ago. :)

@StephaneDelcroix StephaneDelcroix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks. quite there, but not yet. see comment

@kubaflo kubaflo added s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) labels Mar 15, 2026

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please apply ai's suggestions?

@MauiBot MauiBot added s/agent-review-incomplete and removed s/agent-changes-requested AI agent recommends changes - found a better alternative or issues labels May 11, 2026
@dotnet dotnet deleted a comment from MauiBot May 12, 2026
@kubaflo

kubaflo commented May 24, 2026

Copy link
Copy Markdown
Contributor

/review -b feature/refactor-copilot-yml

@kubaflo

kubaflo commented May 26, 2026

Copy link
Copy Markdown
Contributor

/review -b feature/refactor-copilot-yml

@kubaflo

kubaflo commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

/review -b feature/enhanced-reviewer -p ios

@MauiBot

MauiBot commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Merge Conflict Detected — This PR has merge conflicts with its target branch. Please rebase onto the target branch and resolve the conflicts.

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please resolve conflicts?

@kubaflo

kubaflo commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Thanks for this contribution, @bhavanesh2001! 🙏

Unfortunately this fix is no longer needed — the same root cause was already resolved on main by #29028 (commit 830b8de), which fixed the duplicate issue #13243 ("[Android] FlyoutWidth with OnIdiom shows no flyout").

Both issues share the identical root cause: OnIdiomExtension returned Activator.CreateInstance(propertyType) (i.e. 0) instead of the bindable property's real default (e.g. -1) when no value was supplied for the current idiom. The merged fix applies the same bp.GetDefaultValue(...) approach used here, and is actually slightly broader (it also covers reference-type properties, not just value types). That overlap is what's now producing the merge conflict in OnIdiomExtension.cs.

You can confirm on main:
https://github.com/dotnet/maui/blob/main/src/Controls/src/Xaml/MarkupExtensions/OnIdiomExtension.cs

Because the source fix is already in place, I'd suggest closing this PR. The one piece not covered on main is a XAML-level unit test (#29028 only added UI tests), so if you'd like to keep contributing here, a focused Xaml.UnitTests test asserting the bindable default for unspecified idioms would be a welcome follow-up.

Thanks again for digging into this! 🚀

@kubaflo kubaflo closed this Jun 14, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 15, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-xaml XAML, CSS, Triggers, Behaviors community ✨ Community Contribution s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VisualElement.HeightRequest defaults to 0 instead of -1 when using OnIdiom default value

5 participants