Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix for button cornerradius 0 on android #18959

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Fix for button cornerradius 0 on android #18959

merged 2 commits into from
Dec 8, 2023

Conversation

WdeBruin
Copy link
Contributor

@WdeBruin WdeBruin commented Nov 22, 2023 •
edited by jsuarezruiz
Loading

Description of Change

Issue:

Cornerradius was ignored when equal to 0 on Android.

Reason:

  • The ButtonHandler.Android uses extension method UpdateCornerRadius in ButtonExtensions.
  • If the platformView.Background is of type BorderDrawable, it calls UpdateBorderDrawable
  • In that method it checks on > 0 instead of >= 0
  • This is not an issue for iOS, because this > 0 check is only in Android in the specific path that reaches UpdateBorderDrawable.

Fix:

  • Set the check for applying CornerRadius to >= 0 instead of > 0, specifically in this path for Android

Issues Fixed

Fixes #18807
Fixes #18090
Fixed #19052
Fixes #19421

Also setcornerradius if radius is 0. It didn't go into this block and…
10f31b4 
… used default cornerradius of 6, when set to 0.
@WdeBruin WdeBruin requested a review from a team as a code owner November 22, 2023 15:49
@ghost ghost added the community ✨ Community Contribution label Nov 22, 2023
@ghost
Copy link

ghost commented Nov 22, 2023

Hey there @WdeBruin! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@WdeBruin WdeBruin changed the title Also setcornerradius if radius is 0. It didn't go into this block and… Fix cornerradius android 0, works now Nov 23, 2023
@WdeBruin WdeBruin changed the title Fix cornerradius android 0, works now Fix for button cornerradius 0 on android. Nov 23, 2023
@WdeBruin WdeBruin changed the title Fix for button cornerradius 0 on android. Fix for button cornerradius 0 on android Nov 23, 2023
@WdeBruin
Copy link
Contributor Author

When will this be reviewed? @hartez @mattleibow

@WdeBruin WdeBruin mentioned this pull request Nov 28, 2023
Closed
@Eilon Eilon added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Nov 28, 2023
hartez
hartez previously approved these changes Nov 28, 2023
View reviewed changes
@WdeBruin
Copy link
Contributor Author

It still needs a device test I suppose, that is in progress

@jfversluis
Copy link
Member

@WdeBruin feel free to ping me when you get it done!

mattleibow
mattleibow previously requested changes Nov 29, 2023
View reviewed changes
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.I think I saw this and thought it was the same PR and wondered where my comments went :)

Just marking this as needs changes so we don't merge this too quickly. It is just waiting on your tests. Shout if we made the test code to weird 😆

src/Core/src/Platform/Android/ButtonExtensions.cs
Comment on lines 96 to 97
if (button.StrokeThickness > 0)
mauiDrawable.SetBorderWidth(button.StrokeThickness);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also need to be changed? Sure, this PR is not meant for both because of the issue, but maybe both can be fixed at the same time? Not sure if you observed strange things with borders?

If you would like to keep these 2 things separate, the that is good. I'll just create an issue so we don't miss it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem at a time, man.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this as well, but didn't find an issue related to it. So I am unsure if strange things happen here, to be checked.

@WdeBruin
Copy link
Contributor Author

Added a test @jfversluis
Did take some time to figure out the device tests, and get it working. Is there a guide for this?

On the test itself, two things to consider:

  • Using CornerRadius type instead of one integer, so we can check all corners seperately.
  • I use reflection to check the private value _cornerRadius on the Drawable. It is used to draw a Path with vectors, a curve with several small steps (in PathF). The other approach would be to check the vector path with an expected path, but that is difficult to read/understand.

@WdeBruin
Copy link
Contributor Author

@dotnet-policy-service agree

@jfversluis

This comment was marked as off-topic.

Sign in to view
@azure-pipelines Azure Pipelines

This comment was marked as off-topic.

Sign in to view
@WdeBruin
Copy link
Contributor Author

WdeBruin commented Dec 4, 2023

Done? Or anything else you need from me for this one to cross the finish line? @mattleibow @jfversluis

@jfversluis jfversluis enabled auto-merge (squash) December 4, 2023 14:18
@jfversluis jfversluis dismissed mattleibow’s stale review December 8, 2023 09:51

Comments have been addressed

@jfversluis jfversluis merged commit 1373dc1 into dotnet:main Dec 8, 2023
47 checks passed
@jfversluis
Copy link
Member

Congratulations @WdeBruin on your very first .NET MAUI contribution! Go add ".NET MAUI code ninja" to your LinkedIn and have some of our delicious virtual cake 🍰

Thank you so much for your time, effort and patience on this! Those are all very much appreciated.

@NielsKnaap NielsKnaap mentioned this pull request Dec 18, 2023
Closed
@DawidBester
Copy link

Corner radius fix was working in 8.0.6-nightly.9876, but no longer working in 8.0.6 for Android.

I thought the github Merged label and the fact that the fix was part of nightly build meant it will be included in the next SR?

8.0.6-nightly.9876 screenshot

image

8.0.6 Screenshot

image

Code for above: https://github.com/DawidBester/ButtonCornersTest

@awp-sirius
Copy link

Fixed in version 8.0.7 (.net 8.0)
Thanks!

@PureWeen PureWeen mentioned this pull request Feb 29, 2024
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2024
@Eilon Eilon added area-controls-button Button, ImageButton and removed legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor labels May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jfversluis jfversluis jfversluis approved these changes

@mattleibow mattleibow Awaiting requested review from mattleibow

@hartez hartez Awaiting requested review from hartez

Assignees
No one assigned
Labels
area-controls-button Button, ImageButton community ✨ Community Contribution fixed-in-8.0.7 fixed-in-9.0.100-preview.1.9973 platform/android 🤖
Projects
None yet
Milestone
No milestone
8 participants
@WdeBruin @jfversluis @DawidBester @awp-sirius @hartez @mattleibow @Eilon @samhouts