Skip to content

fix(ios): large title transition accounts for back button with no text - #29327

Merged
liamdebeasi merged 9 commits into
mainfrom
FW-5818
Apr 30, 2024
Merged

fix(ios): large title transition accounts for back button with no text #29327
liamdebeasi merged 9 commits into
mainfrom
FW-5818

Conversation

@liamdebeasi

@liamdebeasi liamdebeasi commented Apr 11, 2024

Copy link
Copy Markdown
Contributor

Issue number: resolves #28751


What is the current behavior?

The large title transition does not account for back buttons with no text value. We assume that the .button-text element is always defined, but that is not the case when text="" on the back button. As a result, devs were getting errors because we tried to get the bounding box of a undefined.

What is the new behavior?

  • Revised the large title logic to only grab values from the back button text if the back button text element is actually defined

There should be no behavior change when the back button text element is defined.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev Build: 7.8.5-dev.11713282122.18cabf98

⚠️ Reviewers: Please test this in the sample application on the linked issue. Please be sure to test the following conditions:

  1. When the back button text is defined
  2. When the back button text is not defined
  3. With the default font scale
  4. With a larger font scale

@vercel

vercel Bot commented Apr 11, 2024

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 16, 2024 3:44pm

@liamdebeasi liamdebeasi changed the title Fw 5818 fix(iOS): large title transition accounts for back button with no text Apr 16, 2024
@liamdebeasi liamdebeasi changed the title fix(iOS): large title transition accounts for back button with no text fix(ios): large title transition accounts for back button with no text Apr 16, 2024
@liamdebeasi
liamdebeasi marked this pull request as ready for review April 16, 2024 15:49
@liamdebeasi
liamdebeasi requested a review from a team as a code owner April 16, 2024 15:49
@sean-perkins

Copy link
Copy Markdown
Contributor

Observing a shift to both the button and the large title when popping the view. Doesn't happen consistently, but seems to be related to adjusting the text size.

Could be a Safari quirk if you aren't able to reliable reproduce. I can get Safari's header UI to break/run into the content, so there could be more factors here.

RPReplay_Final1713460282.MP4

@liamdebeasi

Copy link
Copy Markdown
Contributor Author

If you set text=" " on the BackButton (note the space) based off main, does the issue reproduce?

@thetaPC thetaPC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, worked on the original repo and on a simulator

@sean-perkins sean-perkins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested against the latest changes of v7 with text=" " and was unable to reproduce the video I attached earlier.

Tested again with the dev-build and was only able to get in that state once with changing the dynamic font scaling, but was unable to reproduce with the same steps consistently. Navigated 40+ times without the issue presenting itself with a consistent dynamic font scale.

I think these changes are good to merge. There is a rendering quirk in Safari's UI with changing the dynamic font scale with the address bar at the top of the screen that is having other impact (at random, but not very common). If we get issue reports we can do a deeper evaluation of what is going on there.

@liamdebeasi
liamdebeasi added this pull request to the merge queue Apr 30, 2024
Merged via the queue into main with commit bd8d065 Apr 30, 2024
@liamdebeasi
liamdebeasi deleted the FW-5818 branch April 30, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: large title transition does not account for empty back button text

3 participants