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.

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

[iOS] Use updated APIs for the button with images #20953

Closed
wants to merge 22 commits into from

Conversation

tj-devel709
Copy link
Member

@tj-devel709 tj-devel709 commented Mar 1, 2024

Description of Change

Cherry pick iOS only changes from #19834
This PR updates a deprecated API in iOS to use the newer UIButton.Configuration API. Resizing is not really an option anymore with the new API and requires creating a new image each time. For now, let's update to the new API.

Issues Fixed

Makes progress towards #9734
Makes progress towards #21394

@tj-devel709 tj-devel709 added platform/iOS 🍎 legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor area-controls-button Button, ImageButton labels Mar 1, 2024
@tj-devel709 tj-devel709 changed the title Dev/tj/i os18242 [iOS] Fix image button scaling Mar 1, 2024
@tj-devel709
Copy link
Member Author

Bringing over this comment:

It appears that there is an issue on iOS with an ImageButton inside a scrollview. The image does not resize with the width request as android and Windows does. Check out the bottom imagebutton in each of the screenshots:

iOS:
image

Android:
image

Windows:
image

This is the relevant xaml:

<ScrollView HeightRequest="100" WidthRequest="50">
   <ImageButton Source="dotnet_bot.png" />
</ScrollView>

@jsuarezruiz
Copy link
Contributor

Bringing over this comment:

It appears that there is an issue on iOS with an ImageButton inside a scrollview. The image does not resize with the width request as android and Windows does. Check out the bottom imagebutton in each of the screenshots:

iOS: image

Android: image

Windows: image

This is the relevant xaml:

<ScrollView HeightRequest="100" WidthRequest="50">
   <ImageButton Source="dotnet_bot.png" />
</ScrollView>

Could we move ResizeImageSource extension method to the ButtonExtensions and then, use the same on the ImageButtonImageSourcePartSetter from ButtonHandler and ImageButtonHandler?

@tj-devel709 tj-devel709 changed the base branch from main to release/8.0.1xx-sr3 March 6, 2024 18:39
@tj-devel709 tj-devel709 marked this pull request as ready for review March 6, 2024 18:50
@tj-devel709 tj-devel709 requested a review from a team as a code owner March 6, 2024 18:50
@tj-devel709 tj-devel709 requested review from rmarinho and removed request for a team March 6, 2024 18:50
@tj-devel709 tj-devel709 marked this pull request as draft March 7, 2024 15:56
@tj-devel709 tj-devel709 force-pushed the dev/TJ/iOS18242 branch 2 times, most recently from e5aec15 to b4d09d9 Compare March 11, 2024 23:51
Copy link
Contributor

1 file(s) have code issues.

File Issues
.github/policies_readme.md Exception during deserialization. Invalid cast from 'System.String' to 'GitOps.Primitives.Abstractions.ConfigAsCode'.

Total execution time: 0.00 seconds

@tj-devel709 tj-devel709 changed the base branch from release/8.0.1xx-sr3 to main March 11, 2024 23:51
@tj-devel709 tj-devel709 marked this pull request as ready for review March 21, 2024 19:52
Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

image
Is the first Button behavior an expected result?

@@ -4,7 +4,7 @@
namespace Maui.Controls.Sample.Issues
{
[XamlCompilation(XamlCompilationOptions.Compile)]
[Issue(IssueTracker.Github, 18242, "Button ImageSource not Scaling as expected", PlatformAffected.UWP)]
[Issue(IssueTracker.Github, 18242, "Button ImageSource not Scaling as expected", PlatformAffected.All)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we include here a sample using a different ContentLayout (for example, Top)?
Could we include a case using dynamic size?. We can include a button, and tapping it just increase at runtime the height using a random value.

@tj-devel709 tj-devel709 changed the title [iOS] Fix image button scaling [iOS] Use updated APIs for the button with images Mar 22, 2024
@tj-devel709
Copy link
Member Author

Since this PR is not addressing resizing, removed the existing UITest for iOS that was testing resizing and instead added a UITest with a screenshot test to make sure the different image placements work with the button. Will wait for the UITest to produce the diffs!


App.WaitForElement("WaitForStubControl");

VerifyScreenshot();
Copy link
Contributor

Choose a reason for hiding this comment

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

The snapshots for Android and Windows are pending.
image

@tj-devel709
Copy link
Member Author

The windows screenshot is looking a little funky. Testing on windows

@tj-devel709 tj-devel709 marked this pull request as draft March 26, 2024 23:22
@PureWeen
Copy link
Member

Possibly related
#21488

Foda pushed a commit that referenced this pull request Mar 27, 2024
PureWeen pushed a commit that referenced this pull request Mar 28, 2024
* Ensure images in buttons never scale up, only down

* Use `GetImageSourceSize`

* Port UI tests from #20953

* Ensure max height is reset
Ensure source is set after image opened event is subscribed to

---------

Co-authored-by: Mike Corsaro <[email protected]>
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
@tj-devel709
Copy link
Member Author

Closing for now until coming back to this one!

@tj-devel709 tj-devel709 closed this Sep 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants