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

[regression/8.0.0-preview.3.8149] Regression in grid layouts in .NET 8 #14818

Closed
filipnavara opened this issue Apr 27, 2023 · 10 comments · Fixed by #14902
Closed

[regression/8.0.0-preview.3.8149] Regression in grid layouts in .NET 8 #14818

filipnavara opened this issue Apr 27, 2023 · 10 comments · Fixed by #14902
Assignees
Labels
area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter fixed-in-7.0.92 Look for this fix in 7.0.92! fixed-in-7.0.100 fixed-in-7.0.101 fixed-in-8.0.0-preview.5.8529 Look for this fix in 8.0.0-preview.5.8529! i/regression This issue described a confirmed regression on a currently supported version layout-grid platform/iOS 🍎 t/bug Something isn't working

Comments

@filipnavara
Copy link
Member

Description

We get many incorrect grid layout where some cells take up more space than they should. Notably, this is not the same problem as fixed recently in PR 14648, and it is not fixed as of commit e55fdb8.

Repro project running under net7.0-ios:
Simulator Screenshot - iPhone 14 - 2023-04-28 at 00 22 48

Repro project running under net8.0-ios:
Simulator Screenshot - iPhone 14 - 2023-04-28 at 00 23 31

Steps to Reproduce

  1. Download the repro app linked below
  2. Observe the layout is correct when compiled as net7.0-ios
  3. Change the target framework in .csproj to net8.0-ios, the layout of flyout becomes incorrect

Link to public reproduction project repository

https://github.com/filipnavara/grid-layout-in-flyout-repro

Version with bug

8.0 previews

Last version that worked well

7.0 (current)

Affected platforms

iOS

Affected platform versions

iOS 16

Did you find any workaround?

No response

Relevant log output

No response

@filipnavara filipnavara added the t/bug Something isn't working label Apr 27, 2023
@Eilon Eilon added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Apr 27, 2023
@filipnavara
Copy link
Member Author

Worked fine in <MauiVersion>8.0.0-preview.2.7871</MauiVersion>, so the regression happened between .NET 8 Preview 2 and Preview 3.

@filipnavara
Copy link
Member Author

cc @hartez ... This is not fixed even by the latest changes in main, as verified with the CI builds. It's likely to be caused by one of your changes. I can try to narrow it down further if necessary but the lack of CI builds from March makes that a bit tricky.

@filipnavara
Copy link
Member Author

filipnavara commented Apr 28, 2023

I successfully narrowed it down to changes in GridLayoutManager.cs in PR #14114. Specifically, the added check in DecompressStars (targetSize.Height > MeasuredGridHeight()) breaks it.

@hartez
Copy link
Contributor

hartez commented Apr 28, 2023

I suspect this is the same issue as #14694.

@filipnavara
Copy link
Member Author

filipnavara commented Apr 28, 2023

I suspect this is the same issue as #14694.

I think it's related but not the same. What happens here is that the CollectionView (or in another case in our app, Label with a long text) generates a huge size when calculated as "Auto". The row(/column) is declared as * though and in fact there is an external constraint on the size (page size, or in the other case a single cell inside a SwipeView inside a CollectionView). We expect the overall grid size not to exceed the container size which means that the * columns are supposed to shrink. The change in PR #14114, however, causes it to skip the decompression step and it does exceed the container size.

@filipnavara
Copy link
Member Author

Or, to put it into the code terms:

  • _explicitGridHeight == NaN
  • _gridHeightConstraint == 763
  • MeasuredGridHeight() == 803.66666...
  • targetSize.Height == 763
  • _grid.VerticalLayoutAlignment == Fill

...and with these values the condition for decompressVertical produces false, even though it produced a layout that's >803 points high, which is more than the constraint of 763.

@samhouts samhouts added the i/regression This issue described a confirmed regression on a currently supported version label Apr 28, 2023
@samhouts samhouts changed the title Regression in grid layouts in .NET 8 [regression/8.0.0-preview.3.8149] Regression in grid layouts in .NET 8 Apr 28, 2023
@samhouts samhouts added this to the .NET 8 milestone Apr 28, 2023
@JohnHDev

This comment was marked as off-topic.

@filipnavara

This comment was marked as off-topic.

@JohnHDev

This comment was marked as off-topic.

@filipnavara

This comment was marked as off-topic.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 15, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.5.8529 Look for this fix in 8.0.0-preview.5.8529! label Jun 21, 2023
@samhouts samhouts modified the milestones: .NET 8, .NET 7 + Servicing Jul 10, 2023
@samhouts samhouts added the fixed-in-7.0.92 Look for this fix in 7.0.92! label Jul 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter fixed-in-7.0.92 Look for this fix in 7.0.92! fixed-in-7.0.100 fixed-in-7.0.101 fixed-in-8.0.0-preview.5.8529 Look for this fix in 8.0.0-preview.5.8529! i/regression This issue described a confirmed regression on a currently supported version layout-grid platform/iOS 🍎 t/bug Something isn't working
Projects
None yet
6 participants