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

Fix Regression with iOS button resizing with text or image change #25122

Merged
merged 10 commits into from
Oct 14, 2024

Conversation

tj-devel709
Copy link
Member

@tj-devel709 tj-devel709 commented Oct 7, 2024

Description of Change

This PR fixes a regression with iOS buttons re-updating the size of the button when the text changes. There was some code that was trying to use the UIButton.TitleLabel.Bounds for sizing, but there are times the TitleLabel is still set to the old title. This PR also allows changes to the button size when the image is changed.

Issues Fixed

Fixes #25074

Small demo showing changing text and images triggers a resize when needed. The small jump in animation was already there and is not new to this PR.

Screen.Recording.2024-10-07.at.4.01.35.PM.mov

@tj-devel709 tj-devel709 added this to the .NET 8 SR9.2 milestone Oct 7, 2024
@tj-devel709 tj-devel709 requested a review from a team as a code owner October 7, 2024 19:19
@PureWeen
Copy link
Member

PureWeen commented Oct 7, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

rmarinho commented Oct 8, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

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.

The changes impact one related UITest, ButtonResizesWhenTitleOrImageChanges.
Next, the visual representation of the visual changes.
image

Could you take a look, and update the snapshot if is necessary?

@tj-devel709
Copy link
Member Author

The changes impact one related UITest, ButtonResizesWhenTitleOrImageChanges. Next, the visual representation of the visual changes. image

Could you take a look, and update the snapshot if is necessary?

Yes thanks! I believe this is a rounding issue and have pushed a hopeful improvement

@tj-devel709
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@tj-devel709 tj-devel709 force-pushed the dev/TJ/ButtonTextChangeUpdate branch from 2e8301f to 837260b Compare October 8, 2024 21:11
@tj-devel709
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member Author

Choose a reason for hiding this comment

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

I was a little confused about this image, but testing on main, this also fails in the same pixel off way as here. I think this is okay

@PureWeen PureWeen requested a review from jsuarezruiz October 9, 2024 18:43

// _isFirstMeasure is a flag to make sure we manually recalculate the titleRect when there are dynamic changes to the button.
// There are times the platformButton.TitleLabel is updated on dynamic changes and reacts to the change by truncating the label when we actually
// have space in our constraints. We provide the space to be used in our first measure of the titleRect and then use the newly laid out titleRect in later iterations.
bool _isFirstMeasure = true;
internal bool _isFirstMeasure = true;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making this internal can we add a method that gets called?

ResetToFirstMeasureOfNewContent()

Or something of that nature?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good idea!

@tj-devel709
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@tj-devel709 tj-devel709 marked this pull request as draft October 9, 2024 20:02
@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@tj-devel709 tj-devel709 marked this pull request as ready for review October 11, 2024 01:56
@tj-devel709
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@tj-devel709
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@tj-devel709
Copy link
Member Author

/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.

@tj-devel709 The image in the last button (Button 2) has moved a few pixel from the center to the right. Its no longer centered.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is true. The way it was working prior to this PR worked pretty well when we would create a button, but it had some regressions with things on the button changing dynamically such as the text changing. It also led to some inconsistent behavior. The changes in this PR try to manually measure the title of the button when laying it out instead of using the UIButton.TitleLabel.Bounds since that UIButton.TitleLabel.Bounds is not updated right away when these dynamic changes happen. In .NET8 SR8 and before, it also used this same manually measure process but there are a few more optimizations now that will get the measuring and alignment a lot closer in most cases. There will be a few cases where the image may be a pixel off (particularly when the image is on top or bottom of the text of the button) and this will be corrected in the future.

@tj-devel709
Copy link
Member Author

/backport to 8.0.1xx-sr9

Copy link
Contributor

Started backporting to 8.0.1xx-sr9: https://github.com/dotnet/maui/actions/runs/11298662578

Copy link
Contributor

@tj-devel709 an error occurred while backporting to 8.0.1xx-sr9, please check the run log for details!

Error: @tj-devel709 is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=tj-devel709

@tj-devel709
Copy link
Member Author

/backport to 8.0.1xx-sr9

Copy link
Contributor

Started backporting to 8.0.1xx-sr9: https://github.com/dotnet/maui/actions/runs/11298681931

Copy link
Contributor

@tj-devel709 an error occurred while backporting to 8.0.1xx-sr9, please check the run log for details!

The process '/usr/bin/git' failed with exit code 1

@tj-devel709
Copy link
Member Author

Manually backported here to SR9 here: #25214

@PureWeen PureWeen enabled auto-merge (squash) October 14, 2024 14:54
@PureWeen PureWeen merged commit ee80709 into main Oct 14, 2024
97 checks passed
@PureWeen PureWeen deleted the dev/TJ/ButtonTextChangeUpdate branch October 14, 2024 15:23
@samhouts samhouts added the fixed-in-net8.0-nightly This may be available in a nightly release! label Oct 14, 2024
@jsuarezruiz jsuarezruiz restored the dev/TJ/ButtonTextChangeUpdate branch October 22, 2024 09:10
@github-actions github-actions bot locked and limited conversation to collaborators Nov 23, 2024
@samhouts samhouts added fixed-in-9.0.10 fixed-in-net8.0-nightly This may be available in a nightly release! and removed fixed-in-net8.0-nightly This may be available in a nightly release! labels Dec 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-button Button, ImageButton fixed-in-9.0.10 fixed-in-net8.0-nightly This may be available in a nightly release! platform/iOS 🍎
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS: Button not resizing on text Change
6 participants