unoplatform / Uno.Themes

This library is designed to help you use the Material, Fluent or Cupertino design system with the Uno Platform
https://platform.uno
Apache License 2.0
170 stars 32 forks source link

[Android/iOS] CommandBar padding is not applied properly #904

Open Guidemarcus opened 2 years ago

Guidemarcus commented 2 years ago

Current behavior

toolkit:VisibleBoundsPadding.PaddingMask.Top is only applied on the first child of a page.

Expected behavior

toolkit:VisibleBoundsPadding.PaddingMask.Top should be applied on any child if it is at the top of the page

How to reproduce it (as minimally and precisely as possible)

<Page x:Class="ApplicationTemplate.Views.Content.DadJokesPage"
      xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
      xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
      xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
      xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
      xmlns:toolkit="using:Uno.UI.Toolkit"
      mc:Ignorable="d">
    <Grid Background="{StaticResource MaterialBackgroundBrush}">

        <Rectangle Fill="Red"
                   Height="30"
                   toolkit:VisibleBoundsPadding.PaddingMask="Top" />
    </Grid>
</Page>

Workaround

none found

Works on UWP/WinUI

No response

Environment

Uno.UI / Uno.UI.WebAssembly / Uno.UI.Skia

NuGet package version(s)

uno.ui 4.2.6

Affected platforms

iOS, Android

IDE

Visual Studio 2022

IDE version

17.1.0

Relevant plugins

No response

Anything else we need to know?

No response

Guidemarcus commented 2 years ago

MicrosoftTeams-image (2)

jeromelaban commented 2 years ago

I'm not sure to understand the ask here. What's the expected result, visually ?

Guidemarcus commented 2 years ago

I would expect the red rectangle to be under the Status bar, unless I misunderstood how VisibleBoundsPadding.PaddingMask="Top". You can ignore the blue rectangle.

The main issue here: We cannot set the CommandBar under the status bar unless we set the VisibleBoundsPadding on every Grid from every page. This behavior is not good either because sometimes, we want to have a picture under the notch part (specified by design).

jeromelaban commented 2 years ago

The Padding property on a control only offsets the contents of the control it is applied to. In the case of a Rectangle, as there's no content, it will not do anything. If you apply it to a Border the background color will not be padded (margin would), but the contents of the Border, will.

If you have a more specific example of an actual content, it'll be helpful. Also, @agneszitte-nventive may be able to give some additional guidance.

Guidemarcus commented 2 years ago

MicrosoftTeams-image (1)

If this talks more, The "blue rectangle" is a CommandBar. It has a content, but nothing is pushed. Actually, setting a padding does not change anything either

Guidemarcus commented 2 years ago

I feel like the content is pushed properly, but since the command bar is set to a specific height, it gives this weird behavior

agneszitte commented 2 years ago

@jeromelaban / @Guidemarcus I will take a closer look at all of the details next week but @kazo0 and @Xiaoy312 if you already have an idea don't hesitate to let us know.

agneszitte commented 2 years ago

@jeromelaban / @Guidemarcus Ongoing investigation:

kazo0 commented 2 years ago

The issue is stemming from the fact that we have windowTranslucentStatus=true in the styles.xml. This causes the VisibleBoundsPadding to ignore the inset of the status bar since setting windowTranslucentStatus to true allows you to render the UI underneath the status bar.

The current Material CommandBar styles are attempting to set the Height of the Bar to 48, which looks fine when it cannot be drawn under the status bar. But, with a translucent status bar, the CommandBar needs a larger height. Some changes will need to be made to the Material CommandBar style to make it work properly in both scenarios.

For now, a quicker workaround would be to either set the following properties for each CommandBar or set them in a custom CommandBar style that is BasedOn the Material one:

Height="NaN" toolkit.VisibleBoundsPadding.PaddingMask="Top"

With those properties set you shouldn't need to have the PaddingMask set on the parent Grid

Guidemarcus commented 2 years ago

@MaximeDion-Work @MaximeDionNventiveCom not sure which one is the right one :D. I see that we were the on working on this on the nventive side. Could you please explain the status of the issue? If it is still impacting us?

bug from our side https://dev.azure.com/nventive/Practice%20committees/_backlogs/backlog/Mobile%20(.Net)/Features/?workitem=252898

Azure DevOps Services | Sign In
MaximeDion-Work commented 2 years ago

Looking at the confusion surrounding toolkit.VisibleBoundsPadding.PaddingMask should we not also have a version that uses margin too? Like toolkit.VisibleBoundsMargin.MarginMask

MaximeDion-Work commented 2 years ago

Created a pull request so that

Height="NaN" toolkit.VisibleBoundsPadding.PaddingMask="Top"

are set in the command bar style in UnoApplicationtemplate

https://github.com/nventive/UnoApplicationTemplate/pull/203

agneszitte commented 2 years ago

(cc @kazo0 for latest comments regarding this issue)

francoistanguay commented 1 year ago

@kazo0 Is this fixed by SafeArea?

kazo0 commented 1 year ago

It looks like there is already a workaround in UnoApplicationTemplate for this. Moving forward, the Toolkit NavigationBar does not hardcode its height so we will not have this problem when migrating from CommandBar to NavigationBar. I will make a small PR to Themes in order to remove the Height on the cupertino and material CommandBar styles.