Fixed shell pages with top bar #20337

kubaflo · 2024-02-03T00:18:08Z

Description of Change

The solution is based on the Page Rendered from Xamarin Forms

Issues Fixed

Fixes #19159

Before	After
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-02-03.at.01.15.28.mp4	Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-02-02.at.16.41.21.mp4

ghost · 2024-02-03T00:18:17Z

Hey there @kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

jsuarezruiz · 2024-02-05T07:15:56Z

/azp run

azure-pipelines · 2024-02-05T07:16:19Z

Azure Pipelines successfully started running 3 pipeline(s).

jsuarezruiz · 2024-02-05T09:39:13Z

src/Controls/tests/UITests/Tests/Issues/Issue19159.cs

+			_ = App.WaitForElement("page1");
+
+			// The content should not be overlaid by top bar
+			App.Screenshot();


Could use here App.VerifyScreenshot?

Sure!. Should I change it in all my UI tests? I think I wrote App.Screenshot(); instead of VerifyScreenshot in all my PRs 😅

It depends. If there are checks validating properties such as text, sizes, etc. is a correct additional validation. But if we are going to validate something (colors, sizes, etc.) with a screenshot, VerifyScreenshot is the correct one.

jsuarezruiz · 2024-02-06T14:22:56Z

/azp run

azure-pipelines · 2024-02-06T14:23:20Z

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen

I don't know if we can do this.

We intentionally got rid of the padding path.
With NET8 we set the additional insets

maui/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs

Lines 553 to 613 in 0da0a61

    
           		void LayoutHeader() 
        
           		{ 
        
           			if (ShellSection == null) 
        
           				return; 
        
           			int tabThickness = 0; 
        
           			if (_header != null) 
        
           			{ 
        
           				tabThickness = HeaderHeight; 
        
           				var headerTop = (OperatingSystem.IsIOSVersionAtLeast(11) || OperatingSystem.IsMacCatalystVersionAtLeast(11) 
        
           #if TVOS 
        
           				|| OperatingSystem.IsTvOSVersionAtLeast(11) 
        
           #endif 
        
           					) ? View.SafeAreaInsets.Top : TopLayoutGuide.Length; 
        
           				CGRect frame = new CGRect(View.Bounds.X, headerTop, View.Bounds.Width, HeaderHeight); 
        
           				_blurView.Frame = frame; 
        
           				_header.ViewController.View.Frame = frame; 
        
           			} 
        
           			nfloat left; 
        
           			nfloat top; 
        
           			nfloat right; 
        
           			nfloat bottom; 
        
           			if (OperatingSystem.IsIOSVersionAtLeast(11) || OperatingSystem.IsMacCatalystVersionAtLeast(11) 
        
           #if TVOS 
        
           				|| OperatingSystem.IsTvOSVersionAtLeast(11) 
        
           #endif 
        
           			) 
        
           			{ 
        
           				left = View.SafeAreaInsets.Left; 
        
           				top = View.SafeAreaInsets.Top; 
        
           				right = View.SafeAreaInsets.Right; 
        
           				bottom = View.SafeAreaInsets.Bottom; 
        
           			} 
        
           			else 
        
           			{ 
        
           				left = 0; 
        
           				top = TopLayoutGuide.Length; 
        
           				right = 0; 
        
           				bottom = BottomLayoutGuide.Length; 
        
           			} 
        
           			if (tabThickness > 0) 
        
           				_additionalSafeArea = new UIEdgeInsets(tabThickness, 0, 0, 0); 
        
           			else 
        
           				_additionalSafeArea = UIEdgeInsets.Zero; 
        
           			if (_didLayoutSubviews) 
        
           			{ 
        
           				var newInset = new Thickness(left, top, right, bottom); 
        
           				if (newInset != _lastTabThickness || tabThickness != _lastTabThickness) 
        
           				{ 
        
           					_lastTabThickness = tabThickness; 
        
           					_lastInset = new Thickness(left, top, right, bottom); 
        
           					((IShellSectionController)ShellSection).SendInsetChanged(_lastInset, _lastTabThickness); 
        
           				} 
        
           			} 
        
           			UpdateAdditionalSafeAreaInsets(); 
        
           		}

So someone could read from the SafeAreaInsets property to get the value and then use that for padding if they want to.

kubaflo · 2024-02-08T01:45:27Z

I don't know if we can do this.

We intentionally got rid of the padding path. With NET8 we set the additional insets

maui/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs

Lines 553 to 613 in 0da0a61

void LayoutHeader()

{

if (ShellSection == null)

return;

int tabThickness = 0;

if (_header != null)

{

tabThickness = HeaderHeight;

var headerTop = (OperatingSystem.IsIOSVersionAtLeast(11) || OperatingSystem.IsMacCatalystVersionAtLeast(11)

#if TVOS

|| OperatingSystem.IsTvOSVersionAtLeast(11)

#endif

) ? View.SafeAreaInsets.Top : TopLayoutGuide.Length;

CGRect frame = new CGRect(View.Bounds.X, headerTop, View.Bounds.Width, HeaderHeight);

_blurView.Frame = frame;

_header.ViewController.View.Frame = frame;

}

nfloat left;

nfloat top;

nfloat right;

nfloat bottom;

if (OperatingSystem.IsIOSVersionAtLeast(11) || OperatingSystem.IsMacCatalystVersionAtLeast(11)

#if TVOS

|| OperatingSystem.IsTvOSVersionAtLeast(11)

#endif

)

{

left = View.SafeAreaInsets.Left;

top = View.SafeAreaInsets.Top;

right = View.SafeAreaInsets.Right;

bottom = View.SafeAreaInsets.Bottom;

}

else

{

left = 0;

top = TopLayoutGuide.Length;

right = 0;

bottom = BottomLayoutGuide.Length;

}

if (tabThickness > 0)

_additionalSafeArea = new UIEdgeInsets(tabThickness, 0, 0, 0);

else

_additionalSafeArea = UIEdgeInsets.Zero;

if (_didLayoutSubviews)

{

var newInset = new Thickness(left, top, right, bottom);

if (newInset != _lastTabThickness || tabThickness != _lastTabThickness)

{

_lastTabThickness = tabThickness;

_lastInset = new Thickness(left, top, right, bottom);

((IShellSectionController)ShellSection).SendInsetChanged(_lastInset, _lastTabThickness);

}

}

UpdateAdditionalSafeAreaInsets();

}

So someone could read from the SafeAreaInsets property to get the value and then use that for padding if they want to.
@PureWeen
Oh yes, you're right. There is a double spacing added when ios:Page.UseSafeArea="True" Nevertheless, I think it might be a bit confusing for users that setting ios:Page.UseSafeArea="True" is necessary to make shells with top tabs rendered correctly (I wouldn't come up at this initially 😅). Maybe to avoid reporting such issues by users, the padding should be only applied when ios:Page.UseSafeArea is set to false. What do you think about it?

kubaflo · 2024-02-08T10:08:12Z

I don't know if we can do this.
We intentionally got rid of the padding path. With NET8 we set the additional insets

maui/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs

Lines 553 to 613 in 0da0a61

void LayoutHeader()

{

if (ShellSection == null)

return;

int tabThickness = 0;

if (_header != null)

{

tabThickness = HeaderHeight;

var headerTop = (OperatingSystem.IsIOSVersionAtLeast(11) || OperatingSystem.IsMacCatalystVersionAtLeast(11)

#if TVOS

|| OperatingSystem.IsTvOSVersionAtLeast(11)

#endif

) ? View.SafeAreaInsets.Top : TopLayoutGuide.Length;

CGRect frame = new CGRect(View.Bounds.X, headerTop, View.Bounds.Width, HeaderHeight);

_blurView.Frame = frame;

_header.ViewController.View.Frame = frame;

}

nfloat left;

nfloat top;

nfloat right;

nfloat bottom;

if (OperatingSystem.IsIOSVersionAtLeast(11) || OperatingSystem.IsMacCatalystVersionAtLeast(11)

#if TVOS

|| OperatingSystem.IsTvOSVersionAtLeast(11)

#endif

)

{

left = View.SafeAreaInsets.Left;

top = View.SafeAreaInsets.Top;

right = View.SafeAreaInsets.Right;

bottom = View.SafeAreaInsets.Bottom;

}

else

{

left = 0;

top = TopLayoutGuide.Length;

right = 0;

bottom = BottomLayoutGuide.Length;

}

if (tabThickness > 0)

_additionalSafeArea = new UIEdgeInsets(tabThickness, 0, 0, 0);

else

_additionalSafeArea = UIEdgeInsets.Zero;

if (_didLayoutSubviews)

{

var newInset = new Thickness(left, top, right, bottom);

if (newInset != _lastTabThickness || tabThickness != _lastTabThickness)

{

_lastTabThickness = tabThickness;

_lastInset = new Thickness(left, top, right, bottom);

((IShellSectionController)ShellSection).SendInsetChanged(_lastInset, _lastTabThickness);

}

}

UpdateAdditionalSafeAreaInsets();

}

So someone could read from the SafeAreaInsets property to get the value and then use that for padding if they want to.
@PureWeen
Oh yes, you're right. There is a double spacing added when ios:Page.UseSafeArea="True" Nevertheless, I think it might be a bit confusing for users that setting ios:Page.UseSafeArea="True" is necessary to make shells with top tabs rendered correctly (I wouldn't come up at this initially 😅). Maybe to avoid reporting such issues by users, the padding should be only applied whenios:Page.UseSafeArea is set to false. What do you think about it?

That would apply only to the top space, so that the content would always be below the tab bars. Another solution would to move tab bars and nav bar up so that they would ignore the safe area if ios:Page.UseSafeArea is false

PureWeen · 2024-02-09T23:41:43Z

That would apply only to the top space, so that the content would always be below the tab bars. Another solution would to move tab bars and nav bar up so that they would ignore the safe area if ios:Page.UseSafeArea is false

I don't think so, I think because we are using AdditionalSafeAreaInsets the SafeAreaInsets property should be (notch + navbar + AdditionalSafeAreaInsets)

If it's not, then that's a bug with our SafeAreaInsets platform specific.

I think it might be a bit confusing for users

Yea, the entire safe area story is super confusing :-/ we need to just rethink it with new APIs so we're hesitant to make more changes right now.

kubaflo · 2024-02-09T23:49:31Z

That would apply only to the top space, so that the content would always be below the tab bars. Another solution would to move tab bars and nav bar up so that they would ignore the safe area if ios:Page.UseSafeArea is false

I don't think so, I think because we are using AdditionalSafeAreaInsets the SafeAreaInsets property should be (notch + navbar + AdditionalSafeAreaInsets)

If it's not, then that's a bug with our SafeAreaInsets platform specific.

I think it might be a bit confusing for users

Yea, the entire safe area story is super confusing :-/ we need to just rethink it with new APIs so we're hesitant to make more changes right now.

Yes, if you're going to rethink the safe area stuff with new APIs, I think it might be better not to change anything and leave it as it is :)

PureWeen · 2024-02-09T23:52:14Z

Yes, if you're going to rethink the safe area stuff with new APIs, I think it might be better not to change anything and leave it as it is :)

Yea, we're all very grumpy about safe areas right now. That new API IgnoreSafeAreas that's on layouts kind of snuck by early on and we never revisited it. It's led to a ton of very confusing behavior.

Is it alright if we close this PR for now?

kubaflo · 2024-02-09T23:54:17Z

Yes, if you're going to rethink the safe area stuff with new APIs, I think it might be better not to change anything and leave it as it is :)

Yea, we're all very grumpy about safe areas right now. That new API IgnoreSafeAreas that's on layouts kind of snuck by early on and we never revisited it. It's led to a ton of very confusing behavior.

Is it alright if we close this PR for now?

Sure, no problem!

kubaflo added 2 commits February 3, 2024 01:09

Added a UiTest (dotnet#19159)

fa20b8c

Fixed content overlaid by top tabs on shell pages (dotnet#19159)

af08592

kubaflo requested a review from a team as a code owner February 3, 2024 00:18

kubaflo requested review from mattleibow and PureWeen February 3, 2024 00:18

ghost added the community ✨ Community Contribution label Feb 3, 2024

jsuarezruiz added area-controls-shell Shell Navigation, Routes, Tabs, Flyout platform/iOS 🍎 labels Feb 5, 2024

jsuarezruiz reviewed Feb 5, 2024

View reviewed changes

Updated the UITest (dotnet#19159)

b95b176

PureWeen requested changes Feb 8, 2024

View reviewed changes

kubaflo closed this Feb 9, 2024

github-actions bot locked and limited conversation to collaborators Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed shell pages with top bar #20337

Fixed shell pages with top bar #20337

kubaflo commented Feb 3, 2024 •

edited

Loading

ghost commented Feb 3, 2024

jsuarezruiz commented Feb 5, 2024

azure-pipelines bot commented Feb 5, 2024

jsuarezruiz Feb 5, 2024

kubaflo Feb 5, 2024

jsuarezruiz Feb 6, 2024

kubaflo Feb 6, 2024

jsuarezruiz commented Feb 6, 2024

azure-pipelines bot commented Feb 6, 2024

PureWeen left a comment

kubaflo commented Feb 8, 2024

kubaflo commented Feb 8, 2024

PureWeen commented Feb 9, 2024

kubaflo commented Feb 9, 2024

PureWeen commented Feb 9, 2024

kubaflo commented Feb 9, 2024

	void LayoutHeader()
	{
	if (ShellSection == null)
	return;

	int tabThickness = 0;
	if (_header != null)
	{
	tabThickness = HeaderHeight;
	var headerTop = (OperatingSystem.IsIOSVersionAtLeast(11) \|\| OperatingSystem.IsMacCatalystVersionAtLeast(11)
	#if TVOS
	\|\| OperatingSystem.IsTvOSVersionAtLeast(11)
	#endif
	) ? View.SafeAreaInsets.Top : TopLayoutGuide.Length;

	CGRect frame = new CGRect(View.Bounds.X, headerTop, View.Bounds.Width, HeaderHeight);
	_blurView.Frame = frame;
	_header.ViewController.View.Frame = frame;
	}

	nfloat left;
	nfloat top;
	nfloat right;
	nfloat bottom;
	if (OperatingSystem.IsIOSVersionAtLeast(11) \|\| OperatingSystem.IsMacCatalystVersionAtLeast(11)
	#if TVOS
	\|\| OperatingSystem.IsTvOSVersionAtLeast(11)
	#endif
	)
	{
	left = View.SafeAreaInsets.Left;
	top = View.SafeAreaInsets.Top;
	right = View.SafeAreaInsets.Right;
	bottom = View.SafeAreaInsets.Bottom;
	}
	else
	{
	left = 0;
	top = TopLayoutGuide.Length;
	right = 0;
	bottom = BottomLayoutGuide.Length;
	}

	if (tabThickness > 0)
	_additionalSafeArea = new UIEdgeInsets(tabThickness, 0, 0, 0);
	else
	_additionalSafeArea = UIEdgeInsets.Zero;

	if (_didLayoutSubviews)
	{
	var newInset = new Thickness(left, top, right, bottom);
	if (newInset != _lastTabThickness \|\| tabThickness != _lastTabThickness)
	{
	_lastTabThickness = tabThickness;
	_lastInset = new Thickness(left, top, right, bottom);
	((IShellSectionController)ShellSection).SendInsetChanged(_lastInset, _lastTabThickness);
	}
	}

	UpdateAdditionalSafeAreaInsets();
	}

Fixed shell pages with top bar #20337

Fixed shell pages with top bar #20337

Conversation

kubaflo commented Feb 3, 2024 • edited Loading

Description of Change

Issues Fixed

ghost commented Feb 3, 2024

jsuarezruiz commented Feb 5, 2024

azure-pipelines bot commented Feb 5, 2024

jsuarezruiz Feb 5, 2024

Choose a reason for hiding this comment

kubaflo Feb 5, 2024

Choose a reason for hiding this comment

jsuarezruiz Feb 6, 2024

Choose a reason for hiding this comment

kubaflo Feb 6, 2024

Choose a reason for hiding this comment

jsuarezruiz commented Feb 6, 2024

azure-pipelines bot commented Feb 6, 2024

PureWeen left a comment

Choose a reason for hiding this comment

kubaflo commented Feb 8, 2024

kubaflo commented Feb 8, 2024

PureWeen commented Feb 9, 2024

kubaflo commented Feb 9, 2024

PureWeen commented Feb 9, 2024

kubaflo commented Feb 9, 2024

kubaflo commented Feb 3, 2024 •

edited

Loading