-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Android] Fix ImageButton Padding the third #25223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| <?xml version="1.0" encoding="utf-8" ?> | ||
| <ContentPage | ||
| xmlns="http://schemas.microsoft.com/dotnet/2021/maui" | ||
| xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" | ||
| xmlns:ns="clr-namespace:Maui.Controls.Sample.Issues" | ||
| x:Class="Maui.Controls.Sample.Issues.Issue25201"> | ||
|
|
||
| <VerticalStackLayout> | ||
| <!-- https://github.com/dotnet/maui/issues/18001 --> | ||
| <!-- https://github.com/dotnet/maui/issues/25201 --> | ||
| <Switch x:Name="Switch1" AutomationId="Switch1" IsToggled="False" /> | ||
| <ImageButton BackgroundColor="Red" | ||
| WidthRequest="100" | ||
| HeightRequest="100" | ||
| Padding="20" | ||
| Source="dotnet_bot" | ||
| IsVisible="{Binding Source={x:Reference Switch1}, Path=IsToggled}" | ||
| /> | ||
|
|
||
| <!-- https://github.com/dotnet/maui/issues/16713 --> | ||
| <Switch x:Name="Switch2" AutomationId="Switch2" IsToggled="True" /> | ||
| <AbsoluteLayout> | ||
| <ImageButton | ||
| AbsoluteLayout.LayoutFlags="PositionProportional" | ||
| AbsoluteLayout.LayoutBounds="0.5,0.5,120,120" | ||
| WidthRequest="100" | ||
| HeightRequest="100" | ||
| Padding="20" | ||
| Source="dotnet_bot.png" | ||
| CornerRadius="60" | ||
| BackgroundColor="Beige" | ||
| /> | ||
|
|
||
| <ContentView | ||
| AbsoluteLayout.LayoutFlags="SizeProportional" | ||
| AbsoluteLayout.LayoutBounds="0,0,1.0,1.0" | ||
| BackgroundColor="#80FFFFFF" | ||
| IsVisible="{Binding Source={x:Reference Switch2}, Path=IsToggled}" | ||
| /> | ||
| </AbsoluteLayout> | ||
| </VerticalStackLayout> | ||
| </ContentPage> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| namespace Maui.Controls.Sample.Issues | ||
| { | ||
| [XamlCompilation(XamlCompilationOptions.Compile)] | ||
| [Issue(IssueTracker.Github, "25201", "[Android] ImageButton Padding Incorrect After IsVisible False", PlatformAffected.Android)] | ||
| public partial class Issue25201 : ContentPage | ||
| { | ||
| public Issue25201() | ||
| { | ||
| InitializeComponent(); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| #if ANDROID | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues | ||
| { | ||
| public class Issue25201 : _IssuesUITest | ||
| { | ||
| public Issue25201(TestDevice testDevice) : base(testDevice){} | ||
|
|
||
| public override string Issue => "[Android] ImageButton Padding Incorrect After IsVisible False"; | ||
|
|
||
| [Test] | ||
| [Category(UITestCategories.ImageButton)] | ||
| public void ImageButtonPaddingDoesNotChangeWhenIsVisibleChanges() | ||
| { | ||
| App.WaitForElement("Switch1"); | ||
|
|
||
| // https://github.com/dotnet/maui/issues/25201 | ||
| App.Tap("Switch1"); // ImageButton IsVisible changes to true | ||
| // https://github.com/dotnet/maui/issues/16713 | ||
| App.Tap("Switch2"); // Hides overlay ContentView | ||
|
|
||
| VerifyScreenshot(); | ||
|
|
||
| // https://github.com/dotnet/maui/issues/18001 | ||
| App.Tap("Switch1"); // ImageButton IsVisible changes to false | ||
| App.Tap("Switch1"); // ImageButton IsVisible changes to true | ||
|
|
||
| VerifyScreenshot(); | ||
| } | ||
| } | ||
| } | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ public static void UpdateStrokeThickness(this ShapeableImageView platformButton, | |
| public static void UpdateCornerRadius(this ShapeableImageView platformButton, IButtonStroke buttonStroke) => | ||
| platformButton.UpdateButtonStroke(buttonStroke); | ||
|
|
||
| public static async void UpdatePadding(this ShapeableImageView platformButton, IImageButton imageButton) | ||
| public static void UpdatePadding(this ShapeableImageView platformButton, IImageButton imageButton) | ||
| { | ||
| var padding = platformButton.Context!.ToPixels(imageButton.Padding); | ||
| var (strokeWidth, _, _) = imageButton.GetStrokeProperties(platformButton.Context!, true); | ||
|
|
@@ -32,29 +32,9 @@ public static async void UpdatePadding(this ShapeableImageView platformButton, I | |
|
|
||
| // Because there is only a single padding property, we need to reset the padding to 0 otherwise we | ||
| // probably will get a double padding. Trust me. I've seen it happen. It's not pretty. | ||
| // The padding is also reset in MauiShapeableImageView. | ||
| platformButton.SetPadding(0, 0, 0, 0); | ||
|
|
||
| // The padding has a few issues, but setting and then resetting after some calculations | ||
| // are done seems to work. This is a workaround for the following issue: | ||
| // https://github.com/material-components/material-components-android/issues/2063 | ||
| await Task.Yield(); | ||
|
|
||
| if (!platformButton.IsAlive()) | ||
| return; | ||
|
|
||
| // We must re-set all the paddings because the first time was not hard enough. | ||
| platformButton.SetContentPadding((int)padding.Left, (int)padding.Top, (int)padding.Right, (int)padding.Bottom); | ||
| platformButton.SetPadding(0, 0, 0, 0); | ||
|
|
||
| // Just like before, the bugs are not done. This needs to trigger a re-calculation of | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mattleibow You have added this part (ShapeAppearanceModel) in #22298. I am not sure if I can just remove it. None of the tests that looked relevant failed on my machine.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am scared tbh. I forget the corner cases where this happens - and they yield is especially scary. I think I was going off the fixes the native side was doing. The current test set may not be enough and we will have to go remove this and see what breaks manually, and then make your change to see if it fixes it. |
||
| // the shape appearance mode to avoid clipping issues. | ||
| if (platformButton.Drawable is not null) | ||
| { | ||
| platformButton.ShapeAppearanceModel = | ||
| platformButton.ShapeAppearanceModel | ||
| .ToBuilder() | ||
| .Build(); | ||
| } | ||
| } | ||
|
|
||
| internal static void UpdateButtonStroke(this ShapeableImageView platformView, IButtonStroke button) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| using Android.Content; | ||
| using Android.Runtime; | ||
| using Android.Util; | ||
| using Google.Android.Material.ImageView; | ||
|
|
||
| namespace Microsoft.Maui.Platform | ||
| { | ||
| internal class MauiShapeableImageView : ShapeableImageView | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just looking at this and I think that this is all Java code so we probably should just do it in Java and let the bonding do the magic. Doing it in C# is probably going have a big perf impact.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New issue for this Java work: #28532 |
||
| { | ||
| public MauiShapeableImageView(Context? context) : base(context) | ||
| { | ||
| } | ||
|
|
||
| public MauiShapeableImageView(Context? context, IAttributeSet? attrs) : base(context, attrs) | ||
| { | ||
| } | ||
|
|
||
| public MauiShapeableImageView(Context? context, IAttributeSet? attrs, int defStyle) : base(context, attrs, defStyle) | ||
| { | ||
| } | ||
|
|
||
| protected MauiShapeableImageView(nint javaReference, JniHandleOwnership transfer) : base(javaReference, transfer) | ||
| { | ||
| } | ||
|
|
||
| protected override void OnMeasure(int widthMeasureSpec, int heightMeasureSpec) | ||
| { | ||
| // The padding has a few issues. This is a workaround for the following issue: | ||
| // https://github.com/material-components/material-components-android/issues/2063 | ||
|
|
||
| // ShapeableImageView combines ContentPadding with Padding and updates | ||
| // Padding with the result. | ||
| base.OnMeasure(widthMeasureSpec, heightMeasureSpec); | ||
|
|
||
| // We need to reset the padding to 0 to avoid a double padding. | ||
| SetPadding(0, 0, 0, 0); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we run the test in all the platforms? In that way, we can just verify that the Padding property behavior is aligned.