Border issue and inconsistency

[andreinitescu]

Description

The way border is computed, drawn and the way it accounts for Button's size is not consistent across platforms.

The following XAML:

<ContentPage xmlns="http://xamarin.com/schemas/2014/forms"
             xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
             x:Class="App36.MainPage">
    <StackLayout VerticalOptions="Center"
                 Padding="20">
        <Button Text="Button" 
                BorderWidth="40"
                BorderColor="Green"
                ContentLayout="Right, 20"
                Padding="0">
            <Button.Image>
                <OnPlatform x:TypeArguments="FileImageSource">
                    <On Platform="iOS" Value="icon.png" />
                    <On Platform="Android" Value="icon.png" />
                    <On Platform="UWP" Value="Assets/icon.png" />
                </OnPlatform>
            </Button.Image>
        </Button>
    </StackLayout>
</ContentPage>

renders on Android, iOS and UWP like this:

On Android and iOS, the border is drawn behind/above the button's content (image in this case), and it does not influence the button size.
On UWP, the border is drawn around the button's content, and the button size accounts it, the button size is bigger than without the border. The button's content size is preserved. (Note that on UWP, although I have set Padding = 0, there's still some padding, but that's a separate issue not related to this ticket).

Steps to Reproduce

See XAML above or attached demo app.

Expected Behavior

In my opinion, I think UWP's behavior is the correct one. A "border" should be exactly that, it should be around the view's content, it should not be drawn over the content. As a consequence, the view's size should increase to take the border width into account. I think this should be addressed before adding border to more controls.

Padding works correctly, it increases the size of the view.

Actual Behavior

See description and picture above.

Basic Information

• Version with issue: Latest release (3.4.0.1008975)

• Last known good version: ?

• IDE:

• Platform Target Frameworks:

  • iOS:
  • Android:
  • UWP:

• Android Support Library Version:

• Nuget Packages:

• Affected Devices:

Reproduction Link

App36.zip

[PureWeen]

Yea :-/ This was made more consistent on the ImageButton and how it processes borders. For that one we chose iOS as the behavior to go with.

This is a tricky one for button because whatever we do it's going to be a breaking change for the other two platforms.

We'll probably end up needing to make this a platform specific to cause the button to act like the ImageButton or maybe add a property indicating if the border will be drawn above or below. I think insetting the border is fine and if the user wants to grow the Button they can just use padding.

The other thing to note is that the Material components with Visual are going to draw consistent borders. as the top layer

[andreinitescu]

I think insetting the border is fine and if the user wants to grow the Button they can just use padding.

That's merely a hack. Adding padding just so it works right is not the right solution in my opinion.

This is a tricky one for button because whatever we do it's going to be a breaking change for the other two platforms.

Since this has to change anyway, why not do it correctly, why choose to draw border over the content?

I haven't seen any technology/UI framework where the concept of "border" implies drawing over/behind content.

For that one we chose iOS as the behavior to go with.

Is there a reason why choose to draw it over the content and not around the content like on UWP?

[PureWeen]

I haven't seen any technology/UI framework where the concept of "border" implies drawing over/behind content.

https://developer.apple.com/documentation/quartzcore/calayer/1410917-borderwidth

The border is drawn inset from the receiver’s bounds by the value specified in this property. It is composited above the receiver’s contents and sublayers and includes the effects of the cornerRadius property.

The ImageButton iOS implementation is just using straight native iOS for drawing the border by setting the UILayer

		public static void UpdateBorder(IVisualNativeElementRenderer renderer, IBorderElement backgroundView)
		{
			var control = renderer.Control;
			var ImageButton = backgroundView;

			if (control == null)
			{
				return;
			}

			if (ImageButton.BorderColor != Color.Default)
				control.Layer.BorderColor = ImageButton.BorderColor.ToCGColor();

			control.Layer.BorderWidth = Math.Max(0f, (float)ImageButton.BorderWidth);

			nfloat cornerRadius = _defaultCornerRadius;

			if (ImageButton.IsCornerRadiusSet() && ImageButton.CornerRadius != ImageButton.CornerRadiusDefaultValue)
				cornerRadius = ImageButton.CornerRadius;

			control.Layer.CornerRadius = cornerRadius;
		}

So to make iOS draw under we would have to create a custom implementation for iOS or grow the bounds of the UIView to whatever the user set them to

I like not messing with the bounds and image size based on border because I don't want to make those assumptions for the developer. If the image shrinks into the border automagically that's limiting. Maybe the border is a slight mask? What about rounded images where the cornerradius masks the image to a circle and draws a slight border? In those cases it should draw above it but I don't want to increase the size of the view or decrease the size of the image myself. The developer can just decide what they want

[andreinitescu]

Shane, thanks for your comment.

Yes, the CALayer's border width is drawn inwards (which makes sense to be so, because CALayer is the "drawing surface" after all). But on iOS you also have content inset which you can inflate to accommodate the border width if you want. Otherwise, drawing the border over the image doesn't make sense.

So to make iOS draw under we would have to create a custom implementation for iOS or grow the bounds of the UIView to whatever the user set them to

I don't understand, isn't the bounds grown already because of the Padding property?

	void UpdatePadding(UIButton button = null)
	{
		var uiElement = button ?? Control;
		if (uiElement == null)
			return;

		uiElement.ContentEdgeInsets = new UIEdgeInsets(
			(float)(Element.Padding.Top),
			(float)(Element.Padding.Left),
			(float)(Element.Padding.Bottom),
			(float)(Element.Padding.Right)
		);
	}

If the image shrinks into the border automagically that's limiting.

Why would the image have to shrink? What I'm suggesting is to inflate the content inset just like for the Padding. The image in your example would not change its size.

But I understand the alternative, which is to ignore consistency across platforms, and just have iOS and Android draw border inwards over the content. So on a second thought, maybe I'm overthinking it. I was only thinking from the consistency point of view across platforms but also because of the idea that you don't want to draw border over image and framework could help with this out of the box by inflating the bounds to accommodate the border (just how UWP/WPF does it). But if it's consistent across iOS and Android, I guess we're good?
So, just to summarize, I guess the only issue is to just be sure on Android it's drawn the same way like on iOS?

[PureWeen]

The concern with having the Border Width modifying that inset is mainly that it would be a breaking change that I think would be non trivial. So even if the initial decision was wrong. We probably can't change that.

But something is going to have to break somewhere if they are to be consistent so just need to figure out the friendliest way to do that or have to use a platform specific to enable the behavior and ideally we bring UWP on board in that consistency

Though I am remembering MaterialButton incorrectly and it appears that for MaterialButton the border is drawn behind the Icons though the way the padding is done is different between ios and android

On Android the border is just drawn as an inset but the size stays the same

On iOS it looks like it does a half and half thing which we'll probably end up setting clipsToBounds for

[mattleibow]

This one has a few issues when solving - namely backwards compatibility, but I am fixing things and adding platform specifics in the PR: #4967