Skip to content

Check to see if previous page is laid out before starting hero flight #169633

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MitchellGoodwin
Copy link
Contributor

Fixes #168267

When going back to a page that never rendered with a hero transition, the app was hanging. This adds a check to see if a page needs to layout before starting the hero flight, and if so, adding a frame delay.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label May 28, 2025
@MitchellGoodwin MitchellGoodwin changed the title Backswipe hang Check to see if previous page is laid out before starting hero flight May 28, 2025
@MitchellGoodwin MitchellGoodwin requested a review from chunhtai May 29, 2025 17:02
if (isUserGestureTransition && flightType == HeroFlightDirection.pop && toRoute.maintainState) {
// immediately because their page's layout is still valid. Unless due to directly
// adding routes to the pages stack causing the route to never get laid out.
final bool needsLayout = toRoute.subtreeContext?.findRenderObject()?.debugNeedsLayout ?? true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chunhtai do you happen to know if this check would still work in prod apps, or if there's a better way to check for if a route has done it's layout logic yet?

Copy link
Contributor

Choose a reason for hiding this comment

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

no the getter is wrapped in an assert and will crash in release mode, and any method with debug keyword should only be called in assert anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know there is no direct way to know whether a renderobject is layout dirty or not. Can you do something like RenderBox.hasSize?

// completed stage with the initial value at 1.0.
? initial.status == AnimationStatus.completed
// driving the reverse transition, but should have a starting value
// approaching 1.0. In cases where the toRoute begane offstage, there
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// approaching 1.0. In cases where the toRoute begane offstage, there
// approaching 1.0. In cases where the toRoute became offstage, there

if (isUserGestureTransition && flightType == HeroFlightDirection.pop && toRoute.maintainState) {
// immediately because their page's layout is still valid. Unless due to directly
// adding routes to the pages stack causing the route to never get laid out.
final bool needsLayout = toRoute.subtreeContext?.findRenderObject()?.debugNeedsLayout ?? true;
Copy link
Contributor

Choose a reason for hiding this comment

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

no the getter is wrapped in an assert and will crash in release mode, and any method with debug keyword should only be called in assert anyway

if (isUserGestureTransition && flightType == HeroFlightDirection.pop && toRoute.maintainState) {
// immediately because their page's layout is still valid. Unless due to directly
// adding routes to the pages stack causing the route to never get laid out.
final bool needsLayout = toRoute.subtreeContext?.findRenderObject()?.debugNeedsLayout ?? true;
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know there is no direct way to know whether a renderobject is layout dirty or not. Can you do something like RenderBox.hasSize?

// driving the reverse transition, but should have a starting value
// approaching 1.0. In cases where the toRoute begane offstage, there
// is a slight delay while the toRoute laysout before the hero's
// flight begins, so the animation may not begin completed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// flight begins, so the animation may not begin completed.
// flight begins, so the animation may begin in completed status

// flight begins, so the animation may not begin completed.
? initial.value == 1.0
? initial.status == AnimationStatus.completed
: initial.status == AnimationStatus.forward
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't wrap my head around this. Do you know what is the situation that makes this a forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for when the transition is being controlled by a user gesture, before a formal pop has taken place. During that, the gesture controller in a Cupertino route directly updates the animation controller's value without having it go forward or back. So the status returns as forward because the value is between 1.0 and 0.0 and the direction is still forward from either the default value or from when the route transitioned onto the screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is really awkward for this scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah gotcha, yeah in this case I think status is not important, we should just return true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App Hang during Cupertino swipe-back gesture: HeroController accesses un-laid-out RenderBox for offstage route
2 participants