Bug 53561 - GridCalc is calculating incorrect height on items with column spanning.
Summary: GridCalc is calculating incorrect height on items with column spanning.
Status: RESOLVED ANSWERED
Alias: None
Product: Forms
Classification: Xamarin
Component: Forms ()
Version: unspecified
Hardware: PC Windows
: --- normal
Target Milestone: ---
Assignee: Jimmy [MSFT]
URL:
Depends on:
Blocks:
 
Reported: 2017-03-20 15:52 UTC by Brad Chase
Modified: 2018-03-20 17:41 UTC (History)
5 users (show)

Tags: fr
Is this bug a regression?: ---
Last known good build:


Attachments
sample project (27.98 KB, application/zip)
2017-03-21 00:00 UTC, Jimmy [MSFT]
Details
Changed sample project (47.97 KB, application/x-rar)
2017-03-21 00:53 UTC, Brad Chase
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED ANSWERED

Description Brad Chase 2017-03-20 15:52:16 UTC
Create a grid with two columns, Auto and a Star.
Place a label in the second column.
Place a label in the first column with a span of two.

The grid calculates incorrectly on every row with an item that spans causing a really tall grid that gets confused easily especially with repeating items.

I have the fix that Ill put up a PR with, I wanted a Link here to discuss the issue.
Comment 1 Brad Chase 2017-03-20 20:01:37 UTC
I forgot to mention the second label needs to be longer than the first.
Comment 2 Jimmy [MSFT] 2017-03-21 00:00:06 UTC
Created attachment 20477 [details]
sample project

Thanks for filing this! I'm attaching a sample project, but I don't believe it's reproducing the issue you are describing. In the sample app, the Labels overlap each other but there's no tall Grid like you mentioned. 

Can you attach a sample project or modify this one so it reproduces the issue? Thanks again!
Comment 3 Brad Chase 2017-03-21 00:39:51 UTC
Yea I have a very easy test case that contains a grid and two labels.  I can attach it.  Lemme grab the xaml and I'll post it here.  I looked at it all day and had it working until I put it into our larger project and it had other rendering issues.  The problem is it is measuring it first and taking the immediate size of the first element then ignoring the spanning one.  Once it measures again on the star columns it then recalcs the second label correctly but the return height of the grid stays with the first pass causing an extra tall grid.

Before I look at it any longer, is the grid's rendering code up for rework?  It seems like there can be some improvements there.
Comment 4 Brad Chase 2017-03-21 00:53:11 UTC
Created attachment 20480 [details]
Changed sample project

Here is the changed sample project.
Comment 5 Brad Chase 2017-03-21 01:22:46 UTC
I have a fix for it.  I wanted to do some more testing.  If all goes well I will put up a PR tomorrow.  But things are rendering much better.  The problem is there seems to be many more problems in the rendering code than just this one.  There are other issues with rendering after leaving the app and coming back that are not tied to this.  BUT that said, after looking I see alot of similarities with the other functions that could cause issues.


So if you want to see how it should render with that code, change these lines in the meantime:

in  double MeasuredStarredColumns():

comment out these two lines-
						//if (col.ActualWidth >= 0) // if Actual is already set (by a smaller span), skip
						//	continue;

and change-

if (actualWidth >= 0)
							col.ActualWidth = actualWidth;

to-

if (actualWidth >= col.ActualWidth)
							col.ActualWidth = actualWidth;



in double MeasureStarredRows():

comment out these two lines-
//if (row.ActualHeight >= 0) // if Actual is already set (by a smaller span), skip till pass 3
						//	continue;

and change-
if (actualHeight >= 0)
							row.ActualHeight = actualHeight;

to-
if (actualHeight >= row.ActualHeight)
							row.ActualHeight = actualHeight;
Comment 6 Brad Chase 2017-03-22 16:46:44 UTC
Noting this here:

https://github.com/xamarin/Xamarin.Forms/pull/830
Comment 7 Samantha Houts [MSFT] 2017-03-22 17:20:38 UTC
Marking in progress due to the open PR. Thanks!
Comment 8 Brad Chase 2017-03-23 00:50:43 UTC
OK, I did some tests on the auto calculations and found that they are definitely causing problems as well.  To further that I replaced them with the new code and found that our biggest problems with the Grid were also fixed!  The biggest issue for us which was one of the reasons I started chasing this one was if you left the app and came back all the elements inside of grids were cut off to their minimum sizes.  So it was literally view upon view of just one character texts.  Good news is, replacing the auto code with the new code makes the entire app render correctly, just as it was before you left it!  Fantastic :).  Thats been bugging me for a very long time.  Ill check in the code in the morning.
Comment 9 Brad Chase 2017-06-22 19:40:41 UTC
I saw the status changed and before this is tackled I wanted to make sure I reiterated that this needs a very very in depth look at.  Maybe even a rewrite to solve these problems and add some performance gains in.
Comment 10 Paul Brenner 2018-03-19 20:45:43 UTC
See https://github.com/xamarin/Xamarin.Forms/issues/2137
Comment 11 Samantha Houts [MSFT] 2018-03-20 17:41:42 UTC
Please follow the GitHub issue for updates. Thank you!