[Android] Button Border can be set independent of other properties and will not change the size of the Button. behavior change #1570

samhouts · 2018-01-12T19:27:42Z

Description of Change

This fix reverts #1178 and restores the changes made in #941, which will allow Android Buttons to have a border without a BorderRadius specified.

It goes further to allow a border to be added and/or border radius to be changed without changing the BackgroundColor of the Button.

It also keeps the size of the Button the same as a default Material button, draws a shadow, and preserves the "ripple" effect when the Button is pressed.

It does not preserve the state list animation that causes the Button to slowly fade to the pressed color and increase elevation when long pressed. That can be added in a future PR if necessary

This PR has a potential breaking behavior change. It hardcodes the default color of the button on pre-AppCompat because the theme color does not seem to match the actual color that is used.

We have also marked Button.BorderRadius as obsolete in favor of Button.CornerRadius. The BorderRadius property will forward to the CornerRadius property.

Before (AppCompat):

Before (pre-AppCompat):

Note: The test description lies on pre-AppCompat, since the default Button color is actually darker than the color that the "darker" button is. Apparently, the definition of Color.Gray is different.

with #941 (AppCompat):

with #941 (pre-AppCompat):

After (AppCompat):

After (pre-AppCompat):

Note that the shadow is a lighter color of the BackgroundColor, matching the pressed color of the Button. When a blue background is paired with a red border color, it looks a bit odd, like the Button is expanding past the border. However, it looks fine with a sensible color combination, as seen with the gray Button with red border.

Bugs Fixed

API Changes

Deprecated (note, marked obsolete but still present and functional):

- [Obsolete] Button.BorderRadius
- [Obsolete] Button.BorderRadiusProperty

Added:

- Button.CornerRadius
- Button.CornerRadiusProperty

Behavioral Changes

Button.BorderRadius is now obsolete. Please use Button.CornerRadius now.
Default value of Button.BackgroundColor is now hardcoded on pre-AppCompat instead of derived from a theme resource.
Shadow color in Pre-AppCompat is slightly lighter when BorderRadius/BorderColor/BorderWidth are specified.

PR Checklist

Has tests (if omitted, state reason in description)
Rebased on top of master at time of PR
Changes adhere to coding standard
Consolidate commits as makes sense

StephaneDelcroix · 2018-01-31T09:26:05Z

@samhouts to summarize the discussion we had yesterday, I'd recommend not using a defaultValueCreator, but using a default value of -1 meaning whatever-the-default-is-for-that-control-on-this-platform.

to mitigate the breaking change, you could e.g. deprecate BorderRadius in favor of CornerRadius (shared with Frame).

This reverts commit a4c7f31.

Fix typo

…dius, and BorderColor are changed Also add RippleDrawable when supported for the nice ripple effect on press, and set the PaddingTop for the ButtonDrawable. fixes #1436

StephaneDelcroix

I reviewed the Core part, see comments.

I understand why you added DefaultValue fields, for avoiding comparing against magic hardcoded numbers in renderers. Actually, the BindableProperty.DefaultValue serves that purposes, no need for the extra field.

If you prefer not doing the change for CSS support in this PR, that's fine. I can do it in another one as this one is merged.

Overall, it's 👍

StephaneDelcroix · 2018-02-03T12:01:17Z

Xamarin.Forms.Core/Button.cs

+		public const int DefaultCornerRadius = -1;
+		public const double DefaultBorderWidth = -1;
+		public static Color DefaultBorderColor = Color.Default;
+		public static Color DefaultTextColor = Color.Default;


those 2 aren't magic. they do not require fields.

StephaneDelcroix · 2018-02-03T12:03:17Z

Xamarin.Forms.Core/Button.cs

@@ -31,32 +39,42 @@ public class Button : View, IFontElement, ITextElement, IButtonController, IElem

 		public static readonly BindableProperty FontAttributesProperty = FontElement.FontAttributesProperty;

-		public static readonly BindableProperty BorderWidthProperty = BindableProperty.Create("BorderWidth", typeof(double), typeof(Button), -1d);
+		public static readonly BindableProperty BorderWidthProperty = BindableProperty.Create("BorderWidth", typeof(double), typeof(Button), DefaultBorderWidth);


I don't see the need to introduce a level of indirection here. one can check the default value by doing BorderWidthProperty.GetDefaultValue()

StephaneDelcroix · 2018-02-03T12:04:27Z

Xamarin.Forms.Core/VisualElement.cs

@@ -7,6 +7,8 @@ namespace Xamarin.Forms
 {
 	public partial class VisualElement : Element, IAnimatable, IVisualElementController, IResourcesProvider
 	{
+		public static Color DefaultBackgroundColor = Color.Default;


StephaneDelcroix · 2018-02-03T12:11:55Z

Xamarin.Forms.Core/Button.cs

+		public static readonly BindableProperty BorderRadiusProperty = BindableProperty.Create("BorderRadius", typeof(int), typeof(Button), defaultValue: DefaultBorderRadius,
+			propertyChanged: BorderRadiusPropertyChanged);
+
+		public static readonly BindableProperty CornerRadiusProperty = BindableProperty.Create("CornerRadius", typeof(int), typeof(Button), defaultValue: DefaultCornerRadius,


can we promote CornerRadius to BorderElement (shared with Frame) so we could add the border-radius CSS property targeting both ?

I'm targeting 15-5, so that's not in this branch. We can add it when it goes to master. :)

StephaneDelcroix · 2018-02-03T12:16:00Z

Xamarin.Forms.Core/Button.cs

+#pragma warning disable 0618 // retain until BorderRadiusProperty removed
+			bindable.SetValue(Button.BorderRadiusProperty, val);
+#pragma warning restore
+		}


I'm trying to think about all the things that could go wrong here:

binding => should be fine, unless user binds to the 2 properties and has a poor INPC implementation in VM

clearvalue => could you please add a unit test for button making sure that when a property is Cleared (using ClearValue both are actually cleared ? i.e. IsSet() == false; ?

Unit tests added! However, it looks like we don't clear the context when we call ClearValue, so IsSet returns true in that case. Added a test to BindableObject to show that it's not specific to this case.

StephaneDelcroix · 2018-02-03T12:23:07Z

Is this still [breaking] and a behavior change ?

StephaneDelcroix · 2018-02-06T09:36:48Z

Xamarin.Forms.Core.UnitTests/BindableObjectUnitTests.cs

+			bindable.ClearValue(bindableProperty);
+
+			var isSet = bindable.IsSet(bindableProperty);
+			Assert.IsTrue(isSet);


I'll look at fixing this, or investigating why it is that way. if ClearValue is dropping the context, you could ClearValue the other property when the first is !IsSet() in your onPropertyChanged

samhouts · 2018-02-07T19:26:52Z

Since we touched the Core code, we need to ensure that we didn't break iOS or UWP with this change. Here's proof:

iOS: Before

iOS: After

UWP Before:

UWP After

rmarinho

Great work.

bill2004158 · 2018-02-22T12:24:28Z

How can I disable shadow for the button which I don't need it?

bill2004158 · 2018-02-22T13:42:17Z

it seems the new version added some Padding for the button?

samhouts added the breaking Changes behavior or appearance label Jan 12, 2018

samhouts added DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. and removed DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. labels Jan 24, 2018

samhouts mentioned this pull request Jan 24, 2018

Expose BindableObject.GetIsDefault #1652

Closed

samhouts added 10 commits February 2, 2018 10:51

Revert "Revert "Fix border on android buttons (#941)""

96dc000

This reverts commit a4c7f31.

Add repro for 1436

bdb39f5

Fix typo

[Core] Use 2dp for Android default Button BorderRadius

a1681bb

[Android] Add shadow & padding to ButtonDrawable

e8ba9b9

[Android] Set BackgroundDrawable on Button when BorderWidth, BorderRa…

e7e8daa

…dius, and BorderColor are changed Also add RippleDrawable when supported for the nice ripple effect on press, and set the PaddingTop for the ButtonDrawable. fixes #1436

[Android] Default Color for Button is specified for AppCompat and AppAct

f85adf7

[Android] Check BorderRadius value against proper default

f9af624

Fix test case number

a210d9a

grumble grumble this branch is still vs2015 grumble

d041eef

[Android] Get button color from resources

52efff6

samhouts added 7 commits February 2, 2018 13:08

[Core] Obsolete Button.BorderRadius in favor of CornerRadius

452ed22

[Core] Added VisualElement.DefaultBackgroundColor

83aac40

Update tests to ignore obsolete prop warning

551e449

[Android] Use Button.CornerRadius instead of BorderRadius

af66bbe

[iOS] Use Button.CornerRadius instead of BorderRadius

0a9cb65

[macOS] Use Button.CornerRadius instead of BorderRadius

f118632

[UWP] Use Button.CornerRadius instead of BorderRadius

b179366

samhouts force-pushed the fix-issue1436 branch from 7fbe368 to b179366 Compare February 2, 2018 21:12

samhouts requested a review from StephaneDelcroix February 2, 2018 21:13

samhouts added 2 commits February 2, 2018 13:22

Update docs

d5aa485

Fix more cases of BorderRadius obsolete warnings

930d437

samhouts requested a review from rmarinho February 2, 2018 22:12

StephaneDelcroix suggested changes Feb 3, 2018

View reviewed changes

StephaneDelcroix reviewed Feb 6, 2018

View reviewed changes

StephaneDelcroix approved these changes Feb 6, 2018

View reviewed changes

This was referenced Feb 6, 2018

Move CornerRadius to BorderElement #1814

Closed

BindableObject.ClearValue() does not clear the context of the BindableProperty #1813

Closed

samhouts added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Feb 6, 2018

samhouts added 2 commits February 6, 2018 15:53

[iOS] Restore default Button.CornerRadius

650f100

[UWP] Add todo

f68f4c1

samhouts added breaking Changes behavior or appearance and removed breaking Changes behavior or appearance DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. labels Feb 7, 2018

rmarinho approved these changes Feb 8, 2018

View reviewed changes

rmarinho merged commit 2fba186 into 15-5 Feb 8, 2018

samhouts deleted the fix-issue1436 branch February 15, 2018 17:52

samhouts mentioned this pull request Feb 21, 2018

[Android] Buttons can be round; remove Padding & Shadow by default #1935

Merged

4 tasks

samhouts mentioned this pull request Apr 16, 2018

RoutingEffect on Android doesn't work anymore if Xamarin.Forms >= 2.5.0.280555 #2422

Closed

samhouts added this to the 2.5.0 milestone May 5, 2018

samhouts modified the milestones: 2.5.0, 2.3.0 Jun 27, 2018

samhouts mentioned this pull request Jun 27, 2018

[Android] Don't mess with the Button.Padding if the Padding property isn't set #3174

Merged

3 tasks

samhouts added p/Android s/reopened t/bug 🐛 labels Nov 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Button Border can be set independent of other properties and will not change the size of the Button. behavior change #1570

[Android] Button Border can be set independent of other properties and will not change the size of the Button. behavior change #1570

samhouts commented Jan 12, 2018 •

edited

Loading

StephaneDelcroix commented Jan 31, 2018

StephaneDelcroix left a comment

StephaneDelcroix Feb 3, 2018

StephaneDelcroix Feb 3, 2018

StephaneDelcroix Feb 3, 2018

StephaneDelcroix Feb 3, 2018

samhouts Feb 5, 2018

StephaneDelcroix Feb 3, 2018

samhouts Feb 5, 2018

StephaneDelcroix Feb 6, 2018

StephaneDelcroix commented Feb 3, 2018

StephaneDelcroix Feb 6, 2018

samhouts commented Feb 7, 2018

rmarinho left a comment

bill2004158 commented Feb 22, 2018

bill2004158 commented Feb 22, 2018

[Android] Button Border can be set independent of other properties and will not change the size of the Button. **behavior change** #1570

[Android] Button Border can be set independent of other properties and will not change the size of the Button. **behavior change** #1570

Conversation

samhouts commented Jan 12, 2018 • edited Loading

Description of Change

Bugs Fixed

API Changes

Deprecated (note, marked obsolete but still present and functional):

Added:

Behavioral Changes

PR Checklist

StephaneDelcroix commented Jan 31, 2018

StephaneDelcroix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephaneDelcroix commented Feb 3, 2018

Choose a reason for hiding this comment

samhouts commented Feb 7, 2018

rmarinho left a comment

Choose a reason for hiding this comment

bill2004158 commented Feb 22, 2018

bill2004158 commented Feb 22, 2018

[Android] Button Border can be set independent of other properties and will not change the size of the Button. behavior change #1570

[Android] Button Border can be set independent of other properties and will not change the size of the Button. behavior change #1570

samhouts commented Jan 12, 2018 •

edited

Loading