This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Material] + [Android, iOS] Refactored and fixed lots of button layout issues #4967

Merged

PureWeen merged 19 commits into master from mattleibow/button-layout

Jan 30, 2019

Contributor

mattleibow commented Jan 11, 2019 •

edited

Loading

Description of Change

Fixed ALL button issues 😝 (I am going to try and see if this PR will finally clinch all the issues). For all iOS/Android buttons (except the classic, non-AppCompat button), I have refactored the layout logic into a layout manager so that all the fixes get applied to all the buttons. There are some "feature flags" that you set on the manager to more finely control the appearance.

Issues Resolved

API Changes

namespace Xamarin.Forms.Platform.Android {
    // new types
    public class ButtonLayoutManager { ... }
    public interface IButtonLayoutRenderer { ... }
}

namespace Xamarin.Forms.Platform.iOS {
    // new types
    public class ButtonLayoutManager { ... }
    public interface IButtonLayoutRenderer { ... }
}

Platforms Affected

Android (Material, AppCompat, Fast)
iOS (Default, Material)

Behavioral/Visual Changes

Button layout should be good 🤞

Before/After Screenshots

Material with some padding	Defaults	Defaults with some padding and layout

This is what the buttons look like with AppCompatButton via FastRenderers in this PR:

Basics (base, background, corners)	Borders (width, color)	Shadows (platform specifics)

This is what it looks like on iOS:

Basics (base, background, corners)	Borders (width, color)	Shadows (platform specifics)

This is what it looks like on Android with Material:

Basics (base, background, corners)	Borders (width, color)	Shadows (platform specifics)

This is what it looks like on iOS with Material:

Basics (base, background, corners)	Borders (width, color)	Shadows (platform specifics)

Testing Procedure

Play around in the galleries:

"Button Layout"
"Button Border & Background"

PR Checklist

Has automated tests
Rebased on top of the target branch at time of PR
Changes adhere to coding standard

mattleibow requested a review from StephaneDelcroix as a code owner

January 11, 2019 17:15

mattleibow commented

View reviewed changes

Xamarin.Forms.Core/Button.cs

               		public static readonly BindableProperty CommandParameterProperty = ButtonElement.CommandParameterProperty;
               		public static readonly BindableProperty ContentLayoutProperty =
-              			BindableProperty.Create("ContentLayout", typeof(ButtonContentLayout), typeof(Button), new ButtonContentLayout(ButtonContentLayout.ImagePosition.Left, DefaultSpacing));
+              			BindableProperty.Create("ContentLayout", typeof(ButtonContentLayout), typeof(Button), new ButtonContentLayout(ButtonContentLayout.ImagePosition.Left, DefaultSpacing),
+              				propertyChanged: (bindable, oldVal, newVal) => ((Button)bindable).InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged));

Contributor Author

mattleibow Jan 11, 2019

Added to actually update the layout when the image is moved.

Xamarin.Forms.Core/Button.cs Outdated Show resolved Hide resolved

Xamarin.Forms.Platform.Android/ButtonLayoutManager.cs

+              namespace Xamarin.Forms.Platform.Android
+              {
+              	// TODO: Currently the drawable is reloaded if the text or the layout changes.

Contributor Author

mattleibow Jan 11, 2019

The image is reloaded too often, but no more than currently. We can optimize!

Xamarin.Forms.Platform.Android/ButtonLayoutManager.cs

+              						if (view.TransformationMethod != null)
+              							buttonText = view.TransformationMethod.GetTransformationFormatted(buttonText, view);
+              						var measuredTextWidth = view.Paint.MeasureText(buttonText, 0, buttonText.Length());

Contributor Author

mattleibow Jan 11, 2019

We measure the text manually here so we can get the best position. This is "hidden" behind a flag (_alignIconWithText) for the old buttons (non-material) to preserve backwards compatibility.

Xamarin.Forms.Platform.Android/ButtonLayoutManager.cs Outdated


		Thickness padding = _element.Padding;

		// TODO: The padding options here are both wrong. Need to sort this out.

Contributor Author

mattleibow Jan 11, 2019

The button padding needs discussion. But, this is just to make it look pretty. The old way is commented out below.

Xamarin.Forms.Platform.Android/ButtonLayoutManager.cs

+              				return;
+              			}
+              			using (var image = Context.GetDrawable(imageFile))

Contributor Author

mattleibow Jan 11, 2019

This is where the image is loaded, too often. We probably want to optimize before we release...

Xamarin.Forms.Platform.Android/ButtonLayoutManager.cs Outdated

+              		Button _element;
+              		bool _alignIconWithText;
+              		public ButtonLayoutManager(IButtonLayoutRenderer renderer, bool alignIconWithText = false)

Contributor Author

mattleibow Jan 11, 2019

The alignIconWithText parameter basically either positions the image at the edge of the button (old school), or next to the text (new, pretty school).

mattleibow assigned mattleibow and unassigned mattleibow

Contributor

PureWeen commented Jan 11, 2019

is it possible for ButtonLayoutManager to be static and just methods that are called into?
Mainly to reduce allocations so we aren't taking on an additional allocation per button

like this one
https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Platform.Android/FastRenderers/ImageElementManager.cs (edited)

samhouts added p/Android e/7 🕖 t/bug 🐛 labels

Member

samhouts commented Jan 11, 2019

What happens if you change the background color? What happens if you just change the CornerRadius?

mattleibow mentioned this pull request

[Android] Image on Button is not centered with text #3527

Open

Contributor Author

mattleibow commented Jan 11, 2019

Let me test and add some screenshots. But, that should not have changed as this just handles the text, padding and image properties. the border and background are still in the renderer.

mattleibow added the DO-NOT-MERGE-!!! 🛑 label

mattleibow changed the title ~~[Android] Refactored and fixed lots of button layout issues~~ [WIP] [Android] Refactored and fixed lots of button layout issues

Contributor Author

mattleibow commented Jan 12, 2019 •

edited

Loading

This has been an exciting few days... I think I got the general layout bits working, but I need to fix a few things that I spotted.

But, before we talk bugs, the state. There are 4 buttons for Android, the original, the AppCompat, the Fast and the Material. I have decided not to touch the original, as that should not be used in new apps. I have done a bit of refactoring and managed to get the AppCompat and Fast working the same. They technically are the same native control, but one is in a view. The Material one is new and should be similar to the Fast - as MaterialButton is AppCompatButton with a few style changes.

For iOS, there is just the classic and the Material.

Now, bugs...

The start of this PR fixed a majority of the Android layout issues (does not affect background, border, corners). But as I started testing all the various combinations, I found a few issues:

runtime changes to the image position and spacing does not update the button (fixed on Android in the PR, but iOS needs to be done)
the borders render differently
- they do not affect the layout and either overlap or go underneath the content (iOS Material is interesting as it goes half outside half inside, but under the shadow)
- I added an opt-in platform specific to indicate that the border width should not modify the client area but rather grow the button (like UWP and CSS) - this defaults to off for existing buttons, but on for material.

Contributor Author

mattleibow commented Jan 12, 2019 •

edited

Loading

While trying to figure Android out, I noticed some buttons didn't actually resize the same - depending on the contents. This is because the buttons have a minimum size. As a result, setting a border or padding may not actually do anything if the content is small. Here are some images to demonstrate:

	Border=0	Border=8
Padding=0
Padding=8

Just for information, the first button in each image has small text of 6, the middle is the default size and the last is a big 24. This sizing logic is using the new flags to control whether the border width should affect the bounds of the button. With the flag off, the border does not affect the bounds and just slips under the text - making the (border=8, padding=8) image be exactly the same as the (border=8, padding=0) in the table above.

(border=0, padding=0): If you have a look at the values, when both padding and borders are 0, the first two buttons are the same size - the minimum height in effect. The last button is big because the content is bigger that the minimum.
(border=8, padding=0): If we move right, with a border of 8, the first button still has not changed size as the border does not yet add enough size to make the button pass the minimums. The last two buttons are bigger, as the border adds some size.
(border=0, padding=8): In the next row, the a padding of 8 has exactly the same effect as a border of 8 - this is because the border is actually added to the padding.
(border=8, padding=8): To the right, with a padding AND a border of 8, the first button has finally gained enough size to pass the minimum limits.

mattleibow changed the title ~~[WIP] [Android] Refactored and fixed lots of button layout issues~~ [WIP] [Android, iOS] Refactored and fixed lots of button layout issues

This was referenced Jan 13, 2019

Xamarin.Forms Button ContentLayout property not working as expected #4272

Closed

Regression: Xamarin.Forms Button Image position broken on Android #4354

Closed

Border issue and inconsistency #4853

Open

Contributor Author

mattleibow commented Jan 14, 2019

I also spotted another thing on iOS - when setting the Spacing to say 20, there is also a padding of 10 (basically half) applied to the button. I am not sure if this is a bug or a feature, but do we want to reproduce this for vertical? Consistency says yes, but my heart says bug. This can be "undone" with a negative padding, and I am sure changing this is going to break everyone. It is just weird and only in the direction of the layout.

Also, you can see that the horizontal padding is also inconsistent when left/right vs top/bottom. Not sure yet what is causing that, but is that a feature, bug or something that we can't change? (probably the last one)

Here are some pictures 😺

Spacing = 0	Spacing = 20	Spacing = 40

Contributor Author

mattleibow commented Jan 14, 2019

I figured out what was causing the weird horizontal padding when top/bottom. It is that the button is measured in the default state: [image|text] Then, we go ahead and move things.

For material, I probably am going to be consistent with Android and remove this padding.

But on Material and Buttons. Android has a minimum size - both horizontally and vertically. iOS... well that just has a minimum height (0 width buttons are valid). And, not just that, Android has significant padding - in the ranges of 20-30 pixels. iOS just has a minimum height. To be consistent, we might want to also add a base padding on iOS...

Member

samhouts commented Jan 15, 2019

Tell me this PR negates the need for #3251 🙏

Contributor Author

mattleibow commented Jan 15, 2019

@samhouts sadly not :( this PR just works with the layout of the elements above the background - text, image and padding.

Contributor Author

mattleibow commented Jan 15, 2019 •

edited

Loading

So, I think I am getting there. There are a few things that need investigating:

iOS Material button borders are weird.
- There is an update to the native library, so that might be fixed
iOS does not support anything other than single line buttons
- The text can be made to wrap, but then the button will have to be manually laid out as the default implementation does not do anything extra
- As the positions of the elements are updated after the button is laid out, it is often the case that things aren't perfect. In most cases, if the properties are set before it is rendered, then it is fine. If the image, text or layout is changed at runtime, then the bits are not technically correct.
Android Material border does not fit the button nicely
The text layout of Material is not the same on Android and iOS: Android wraps, and iOS truncates in the middle of the line.
We support more than what Material allows per it's guidelines (https://material.io/design/components/buttons.html#contained-button)
- DON'T WRAP text
- A single, optional icon to the LEFT
- NO BORDERS, if you want a border, use the "Outlined Button"
Android AppCompatButton does not really look native (probably fixed here: [Android] Improve default Button shadow & size appearance #3251)
- when setting a background, the entire button changes (maybe we should use the color tint list in the specific case that everything is default, except the background color)
I added a flag to update the button bounds/padding when the border thickness changes - default to on with material. Do we actually want this? This will prevent the border from ever covering the content.

samhouts added the retarget-branch-required label

PureWeen mentioned this pull request

[Material] Contained Button Spec #4951

Closed

Member

samhouts commented Jan 29, 2019

@mattleibow You removed the "fixes" keyword from the PR body. Was that intentional?


          Merge branch 'master' into mattleibow/button-layout

e5bf7af

Contributor

PureWeen commented Jan 30, 2019

build

Contributor Author

mattleibow commented Jan 30, 2019 •

edited

Loading

@samhouts nope. all magical now.


          fix bad merge

4a69a4d

Contributor

PureWeen commented Jan 30, 2019

build

PureWeen approved these changes

View reviewed changes


          adding delay to ui tests

7dde423

Contributor

PureWeen commented Jan 30, 2019

build

PureWeen merged commit 0dd61b7 into master

PureWeen deleted the mattleibow/button-layout branch

January 30, 2019 23:44

PureWeen mentioned this pull request

Android Buttons shows ripple effect only on the upper left area of the button on Samsung devices when changing/setting the buttons background color #2096

Open

samhouts added this to the 3.5.0 milestone

pikausp commented Feb 7, 2019

Hey,

first of all, thanks for the PR! I installed the latest preview and there are some things that are not behaving correctly (or so I believe at least).

The horizontal alignment of the image when ImagePosition is Top is now broken on iOS

Before https://i.imgur.com/yPJvFup.png
After https://i.imgur.com/0BiNAdL.png

I cannot verify this on a physical device right now, so I do hope it's not just a weird emulator bug.

There is still artificial spacing between the text and the image on Androids (iOS works as expected). There are slight changes in the spacing/sizing between versions, but none of them demonstrates the desired behavior.

4.0-pre4 + Material Visual https://i.imgur.com/HHxXmS5.png
4.0-pre4 + Without Visual https://i.imgur.com/EYefWcN.png
3.5-rc1 + Material Visual https://i.imgur.com/O6ioHCM.png
3.5-rc1 + Without Visual https://i.imgur.com/dRBNy7J.png

A workaround is to set negative spacing in ContentLayout i.e ContentLayout="Top, -40".

mattleibow mentioned this pull request

Image and text alignment / spacing is incorrect #5160

Closed

Contributor Author

mattleibow commented Feb 7, 2019

@pikausp I moved the this to a new issue #5160 so we can track it and any progress.

samhouts mentioned this pull request

Button image is not centered #5861

Closed

samhouts added t/bug 🐛 i/regression labels

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a/visual e/7 🕖 i/regression p/Android t/bug 🐛 t/enhancement ➕