Skip to content

[iOS] Fixed ImageButton not honoring Padding value. #26785

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

Merged
merged 19 commits into from
Mar 3, 2025

Conversation

NirmalKumarYuvaraj
Copy link
Contributor

Issue Details

The ImageButton size calculation is incorrect. When an ImageButton has both padding and an image, its measured size does not adapt to include the padding values, causing the image to render smaller than expected.

Root Cause

The ImageButton DesiredSize is determined solely based on the ImageSize during the SizeThatFitsImage calculation. As a result, the ImageButton renders according to the image size alone, without accounting for the padding values in its measurement.

Description of Change

Updated the ImageButton to implement ICrossPlatformLayout for handling measurements, ensuring that content measurement accounts for padding values as expected, thereby improving the layout behavior.

Tested the behaviour in the following platforms

  • Android
  • Windows
  • iOS
  • Mac

Issue Fixed

Fixes #22521

Output

Before After
image image

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Dec 23, 2024
@sheiksyedm sheiksyedm added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Dec 23, 2024
@PureWeen PureWeen added the area-controls-button Button, ImageButton label Dec 26, 2024
CGSize imageSize = platformButton.CurrentImage?.Size ?? CGSize.Empty;

double contentWidth = imageSize.Width + contentEdgeInsets.Left + contentEdgeInsets.Right;
double contentHeight = imageSize.Height + contentEdgeInsets.Top + contentEdgeInsets.Bottom;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thickness has a HorizontalThickness and VerticalThickness properties you could use.

@NirmalKumarYuvaraj NirmalKumarYuvaraj marked this pull request as ready for review January 8, 2025 05:05
@NirmalKumarYuvaraj NirmalKumarYuvaraj requested a review from a team as a code owner January 8, 2025 05:05
@jfversluis
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Padding_Add failing

@prakashKannanSf3972
Copy link
Contributor

Padding_Add failing

@PureWeen , The failure was due to the macOS snapshot for Padding_Add not being updated earlier. The Snapshot has been updated, and the test is expected to pass in the next CI run.

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Fix conflicts

@prakashKannanSf3972
Copy link
Contributor

Fix conflicts

@PureWeen, Conflicts have been resolved.

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen added this to the .NET 9 SR4 milestone Jan 14, 2025
@prakashKannanSf3972
Copy link
Contributor

@MartyIX, @PureWeen,

Thank you for the review! The nullable directive code has been removed as suggested.

@PureWeen
Copy link
Member

PureWeen commented Jan 16, 2025

/azp run

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Contributor

Choose a reason for hiding this comment

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

Must update the macOS snapshots after: #27531

Copy link
Contributor

Choose a reason for hiding this comment

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

@jsuarezruiz,

Updated the macOS snapshots as per your suggestion in the latest commit. Let me know if you have any further concerns. Thanks!

@prakashKannanSf3972
Copy link
Contributor

The test Issue18242Test is failing on iOS. Small differences in the image related with the Padding: image Could you review it?

The test Issue18242Test is failing on iOS. Small differences in the image related with the Padding: image Could you review it?

The failing test case has been verified and resolved. It has been successfully validated locally, and expect it will pass in the next CI run.

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

is there a way we can encapsulate this a bit into this method?

internal static CGSize SizeThatFitsImage(this UIImageView imageView, CGSize constraints)

It looks like they are basically the same except for the padding part?

Maybe we can create an internal version of this for now that takes padding?

@prakashKannanSf3972
Copy link
Contributor

is there a way we can encapsulate this a bit into this method?

internal static CGSize SizeThatFitsImage(this UIImageView imageView, CGSize constraints)

It looks like they are basically the same except for the padding part?

Maybe we can create an internal version of this for now that takes padding?

@PureWeen,

Thanks for the suggestion! I have refactored the code to encapsulate the logic as recommended. The updated method now includes padding values for improved reusability while preserving the existing constraints. Please review and let me know if you have any further feedback!

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Looks like some new images will need to be added to the test

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@prakashKannanSf3972
Copy link
Contributor

Looks like some new images will need to be added to the test

New images have been added.

@github-project-automation github-project-automation bot moved this from Changes Requested to Approved in MAUI SDK Ongoing Mar 2, 2025
@PureWeen PureWeen merged commit 24db165 into dotnet:main Mar 3, 2025
123 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Done in MAUI SDK Ongoing Mar 3, 2025
tj-devel709 pushed a commit that referenced this pull request Mar 3, 2025
* Fixed-ImageButton-Padding-Issue-iOS

* Modified-code

* Optimized-code

* Added-Mac-Snapshot

* fixed-conflicts

* Removed-nullable-directive

* Resolved-Conflicts

* Modified-Code

* Updated-Snapshots

* Modified-Code

* Modified-Snapshot

---------

Co-authored-by: prakashKannanSf3972 <127308739+prakashKannanSf3972@users.noreply.github.com>
Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
bhavanesh2001 pushed a commit to bhavanesh2001/maui that referenced this pull request Mar 7, 2025
* Fixed-ImageButton-Padding-Issue-iOS

* Modified-code

* Optimized-code

* Added-Mac-Snapshot

* fixed-conflicts

* Removed-nullable-directive

* Resolved-Conflicts

* Modified-Code

* Updated-Snapshots

* Modified-Code

* Modified-Snapshot

---------

Co-authored-by: prakashKannanSf3972 <127308739+prakashKannanSf3972@users.noreply.github.com>
Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-button Button, ImageButton community ✨ Community Contribution partner/syncfusion Issues / PR's with Syncfusion collaboration
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ImageButton Padding resizes image down unexpectedly even when there is available space
9 participants