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

ContentView inherits from the wrong base Layout class #7613

Open
ivan-todorov-progress opened this issue May 30, 2022 · 20 comments
Open

ContentView inherits from the wrong base Layout class #7613

ivan-todorov-progress opened this issue May 30, 2022 · 20 comments
Labels
area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter migration-compatibility Xamarin.Forms to .NET MAUI Migration, Upgrade Assistant, Try-Convert partner Issue or Request from a partner team platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Milestone

Comments

@ivan-todorov-progress
Copy link

ivan-todorov-progress commented May 30, 2022

Description

There are two parallel layout implementations in .NET MAUI:

  1. The new layout implementation with the base class of Microsoft.Maui.Controls.Layout
  2. The legacy layout implementation from Xamarin.Forms with the base class of Microsoft.Maui.Controls.Compatibility.Layout

The new layout implementation is compatible with the new MAUI handlers. The old layout implementation from Xamarin.Forms is not compatible with the handlers and should be avoided.

The problem with the ContentView is that it has a handler, but is still inherits from the legacy layout Microsoft.Maui.Controls.Compatibility.Layout. This is a major source of bugs, as the usual layout logic might not be executed correctly or might not get executed at all on some platforms.

Steps to Reproduce

Download and run the attached sample project on Windows.

TestApp.zip

The sample project contains a single CustomView inheriting from ContentView. The sole purpose of that CustomView is to override all the virtual Measure, Arrange, Layout etc. methods of the base class and dump some debug output when they are called. The sample applications has two additional buttons to call the InvalidateMeasure method on the CustomView from the base VisualElement class and from the IView interface.

Observe the debug output of the application. You can also put breakpoints in the overridden methods to be sure. Notice that nothing happens: nothing is called at all. All the layout logic of the CustomView is completely discarded.

Version with bug

6.0 (current)

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

N/A

Did you find any workaround?

Unless Microsoft changes the implementation of the ContentView to inherit from the new Microsoft.Maui.Controls.Layut and implements the necessary glue code, I cannot suggest any viable workaround, except not using ContentView or any templated controls in the .NET MAUI applications.

Relevant log output

No response

@ivan-todorov-progress ivan-todorov-progress added s/needs-verification Indicates that this issue needs initial verification before further triage will happen t/bug Something isn't working labels May 30, 2022
@drasticactions
Copy link
Contributor

Wouldn't this be an issue with the Telerik controls you're using? I'm not sure how this is a MAUI issue.

@ivan-todorov-progress
Copy link
Author

ivan-todorov-progress commented May 31, 2022

Wouldn't this be an issue with the Telerik controls you're using? I'm not sure how this is a MAUI issue.

Please, excuse me for the confusion. The correct types are Microsoft.Maui.Controls.Layout and Microsoft.Maui.Controls.Compatibility.Layout. I have corrected the description of the bug.

The sample application does not use any Telerik controls at all. It just inherits from Microsoft.Maui.Controls.ContentView.

@Eilon Eilon added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label May 31, 2022
@AswinPG
Copy link

AswinPG commented Jun 2, 2022

#7668 Could this be related ?

@ivan-todorov-progress
Copy link
Author

ivan-todorov-progress commented Jun 6, 2022

@AswinPG I have not debugged the MAUI source code to that extent TBH, but the LayoutChildren method in ContentPage seems like an old paradigm from the legacy Xamarin.Forms layout implementation. The new MAUI layouts have something called ILayoutManager, with two methods: Measure and ArrangeChildren: https://github.com/dotnet/maui/blob/main/src/Core/src/Layouts/ILayoutManager.cs.

It is a total mess really, as the new and the legacy layout implementations define different paradigms and sometimes combining the two in the same visual tree does not work quite well, i.e.: InvalidateMeasure not invalidating anything, the Measure and Layout/Arrange methods not always called when they should etc. Migrating everything to the new MAUI layouts would resolve many such problems and this should be top priority for the MAUI team IMHO, as a broken layout system would mean broken apps and many disappointed developers migrating to other UI frameworks. ;-)

@Xyncgas
Copy link

Xyncgas commented Jun 6, 2022

I've been finding using webview to draw layout can already be satisfying enough, for my app to run on multiple device while having native platform capabilities.
If creating layout in MAUI is easy enough then I would use it, however you are gonna have to top all the dom events they have in the webview (onclick -- I supposed there's button in MAUI, onmouseover, onkeyup...)
but also is MAUI supporting everything well but linux

@jfversluis jfversluis added partner Issue or Request from a partner team and removed s/needs-verification Indicates that this issue needs initial verification before further triage will happen labels Jun 13, 2022
@philipag
Copy link

I agree with @ivan-todorov-progress in that this is the type of bug that is making it next to impossible to migrate complex XF apps to Maui. This should be top priority to get fixed.

@rachelkang rachelkang added this to the 6.0-servicing milestone Jun 30, 2022
@hartez hartez removed their assignment Aug 2, 2022
@Redth Redth modified the milestones: 6.0-servicing, Backlog Aug 30, 2022
@ghost
Copy link

ghost commented Aug 30, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@Zhanglirong-Winnie Zhanglirong-Winnie added s/verified Verified / Reproducible Issue ready for Engineering Triage s/triaged Issue has been reviewed labels Sep 13, 2023
@Zhanglirong-Winnie
Copy link

Verified this issue with Visual Studio Enterprise 17.8.0 Preview 1.0. Can repro on windows platform with sample project.
TestApp.zip

@MitchBomcanhao
Copy link

not being able to use contentviews with control templates makes migrating our xamarin forms app nearly impossible.

@Alex-111
Copy link

The later this will be fixed the more apps will break. Any reasons why this is delayed?

@MitchBomcanhao
Copy link

bump!

@GalaxiaGuy
Copy link
Contributor

GalaxiaGuy commented Jan 25, 2024

@hartez In #19794 you say this isn't exactly a bug.

Does that mean it is intentional that ContentView (and TemplatedView) inherits from Compatibility.Layout?

@hartez
Copy link
Contributor

hartez commented Jan 25, 2024

@hartez In #19794 you say this isn't exactly a bug.

Does that mean it is intentional that ContentView (and TemplatedView) inherits from Compatibility.Layout?

Yes, insofar as they inherited from that class in Forms, and that base class relationship was left intact for backward compatibility reasons in the first release of .NET MAUI.

Starting from scratch, and with no backward-compatibility goals, I doubt we'd have kept that inheritance chain. Which is why I say it isn't exactly a bug; given the current capabilities and design, it certainly looks like one, but it was an intentional design choice at one point. And "fixing" it would require a breaking change.

@MitchBomcanhao
Copy link

Considering you have to remove the compatibility mode in order to get content view to even render on windows, seems like it is already broken. Might as well just fix it.

@Xyncgas
Copy link

Xyncgas commented Jan 25, 2024

You are losing new customer for your new framework by remaining compatibility

@jump32
Copy link

jump32 commented Jan 26, 2024

@hartez So why not have a version of ContentView with this inheritance chain under the Compatibility namespace? In the same way as you do for other controls, such as Stack, etc.

That way you can have a version in the Controls namespace that has the correct inheritance. That isn't fundamentally broken. And that, you know, actually works.

@hartez
Copy link
Contributor

hartez commented Jan 27, 2024

@hartez So why not have a version of ContentView with this inheritance chain under the Compatibility namespace? In the same way as you do for other controls, such as Stack, etc.

That way you can have a version in the Controls namespace that has the correct inheritance. That isn't fundamentally broken. And that, you know, actually works.

I'm not clear on what you all mean by "ContentView not working", but if I take the repro project attached to this issue and update it to .NET 8, the CustomView class displays just fine.

Now, if you actually put some custom measurement logic in CustomView and aren't just calling base.MeasureOverride, at the moment you're required to explicitly set the value of DesiredSize; if you don't, the layout won't work correctly.

So this won't work:

protected override Size MeasureOverride(double widthConstraint, double heightConstraint)
{
	var size = new Size(200,200);
	return size;
}

But this will:

protected override Size MeasureOverride(double widthConstraint, double heightConstraint)
{
	var size = new Size(200,200);
	DesiredSize = size;
	return size;
}

This was a design/documentation error (it wasn't documented that you needed to set DesiredSize, and really the root classes should just be doing that for you anyway). So after #19794, you won't have to do that anymore - the first version of MeasureOverride above will work.

But that's all orthogonal to the "wrong base class" claim. The original error reported (1.5 years ago) has been fixed, and the fix didn't have much to do with the base class. Changing the base class now (even if you could without breaking any existing code) is unlikely to address any problems you're having. If I could I would just close this issue.

@MitchBomcanhao
Copy link

@hartez what I mean by it not working is that if you have the maui compatibility mode enabled in mauiprogram.cs, then windows will not render contentview stuff. I don't know if the two problems are linked, but contentview is completely broken on windows when using compatibility mode, so this idea of not changing it because it becomes a breaking change is quite funny for everyone that is actually trying to convert existing applications and finding them to just not work at all.

@hartez
Copy link
Contributor

hartez commented Jan 28, 2024

This issue isn't about compatibility mode, though. And I'm not sure I see how changing the base class of ContentView would fix something that's broken with compatibility mode, especially if it's only broken in compatibility mode on one platform. That it's only broken on one platform suggests a bug in the compatibility renderer for that platform, not an issue with the hierarchy in the cross-platform code.

It looks like there are other issues open for the actual problem (i.e., that ContentView doesn't work properly in compatibility mode); I would suggest the focus be on those issues (e.g., #19779), not this one.

Unless someone has opened a PR that fixes the problem (without an API break) by changing the base class?

@rachelkang rachelkang added the migration-compatibility Xamarin.Forms to .NET MAUI Migration, Upgrade Assistant, Try-Convert label Feb 1, 2024
@PureWeen
Copy link
Member

First step for this one
#23710

Now in .NET 10 we will probably just remove that middle layer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter migration-compatibility Xamarin.Forms to .NET MAUI Migration, Upgrade Assistant, Try-Convert partner Issue or Request from a partner team platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Projects
None yet
Development

No branches or pull requests