Skip to content

[go_router] Detect both framework and material_ui/cupertino_ui apps for Hero controller - #12695

Closed
yashas-hm wants to merge 5 commits into
flutter:mainfrom
yashas-hm-os:fix-issue-192043
Closed

[go_router] Detect both framework and material_ui/cupertino_ui apps for Hero controller#12695
yashas-hm wants to merge 5 commits into
flutter:mainfrom
yashas-hm-os:fix-issue-192043

Conversation

@yashas-hm

Copy link
Copy Markdown
Contributor

Description

go_router 18.0.0 migrated its Material/Cupertino helpers to the material_ui /
cupertino_ui packages. As part of that migration, isMaterialApp /
isCupertinoApp (in lib/src/pages/material.dart and lib/src/pages/cupertino.dart)
resolve MaterialApp / CupertinoApp to the material_ui / cupertino_ui types
and detect the app with findAncestorWidgetOfExactType.

Because that is an exact type match, an app built with the framework's own
MaterialApp / CupertinoApp (from package:flutter/material.dart /
package:flutter/cupertino.dart) is no longer detected. RouteBuilder then falls
through to a bare HeroController() instead of createMaterialHeroController() /
createCupertinoHeroController(), and Hero animations silently stop flying inside
ShellRoute / StatefulShellRoute. There is no compile error and no warning — the
animation just disappears.

This is a transition-period problem: while Material and Cupertino are being decoupled
from the SDK (flutter/flutter#184093), package:flutter/material.dart remains fully
supported and is what the vast majority of apps still use.

This PR makes app-type detection recognize both the framework's
MaterialApp / CupertinoApp and the material_ui / cupertino_ui variants, so the
correct HeroController is installed regardless of which Material/Cupertino library
the app is built with. A code comment notes the framework check can be removed once
package:flutter/material.dart is sunset.

Related Issues

Fixes flutter/flutter#192043
Context: flutter/flutter#184093 (Material/Cupertino decoupling)

Tests

  • Added regression tests asserting that a shell-route app selects
    createMaterialHeroController() / createCupertinoHeroController() for both a
    framework MaterialApp / CupertinoApp and a material_ui / cupertino_ui
    MaterialApp / CupertinoApp.
  • Existing go_router tests continue to pass.

Pre-Review Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools.
  • I read the [Tree Hygiene] page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran [the auto-formatter].
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I followed [the version and CHANGELOG instructions], using [semantic versioning] and the [repository CHANGELOG style], or I have commented below to indicate which documented exception this PR falls under[^1].
  • I updated/added any relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or I have commented below to indicate which [test exemption] this PR falls under[^1].
  • All existing and new tests are passing.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request updates go_router to version 18.0.1, resolving an issue where Hero flight animations failed to play for nested routes inside shell routes when using Flutter's framework MaterialApp or CupertinoApp. The fix updates isCupertinoApp and isMaterialApp to check for both the framework-specific and package-specific variants of these widgets, and adds corresponding regression tests. The reviewer suggests reordering these checks to prioritize the framework-specific types first, allowing the lookup to short-circuit earlier for most applications and improve efficiency.

Comment thread packages/go_router/lib/src/pages/cupertino.dart Outdated
Comment thread packages/go_router/lib/src/pages/material.dart Outdated

@Piinks Piinks 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.

go_router 18.0.0 migrated its Material/Cupertino helpers to the material_ui /
cupertino_ui packages. As part of that migration, isMaterialApp /
isCupertinoApp (in lib/src/pages/material.dart and lib/src/pages/cupertino.dart)
resolve MaterialApp / CupertinoApp to the material_ui / cupertino_ui types
and detect the app with findAncestorWidgetOfExactType.

Because that is an exact type match, an app built with the framework's own
MaterialApp / CupertinoApp (from package:flutter/material.dart /
package:flutter/cupertino.dart) is no longer detected.

This is intentional, and the reason why the migration to material_ui and cupertino_ui of this package was a major release. We are not intending to try to support both flutter/material and material_ui in this package at the same time.

@yashas-hm

Copy link
Copy Markdown
Contributor Author

@Piinks Should it rather have some error instead of silently failing? I came across this issue after an update by a dependabot.

@Piinks

Piinks commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@Piinks Should it rather have some error instead of silently failing? I came across this issue after an update by a dependabot.

I don't think so. Since the class names are all the same I am not sure how it could fail for this reason. This package uses semantic versioning, so the major release bump is meant to indicate that it is not backwards compatible.

@yashas-hm

Copy link
Copy Markdown
Contributor Author

@Piinks Should it rather have some error instead of silently failing? I came across this issue after an update by a dependabot.

I don't think so. Since the class names are all the same I am not sure how it could fail for this reason. This package uses semantic versioning, so the major release bump is meant to indicate that it is not backwards compatible.

Thanks for pushing back, let me clarify, because this one is genuinely counterintuitive.

The class names being identical is exactly why it breaks. findAncestorWidgetOfExactType matches on runtime type identity (ancestor.widget.runtimeType == T), not on the class's name. As of 18.0.0 these
files import MaterialApp from package:material_ui/material_ui.dart, which declares its own class MaterialApp (material_ui/lib/src/app.dart). That is a different type from package:flutter/material.dart's
MaterialApp same simple name, two separate declarations in two packages. So findAncestorWidgetOfExactType<material_ui.MaterialApp>() returns null when the real ancestor is a framework MaterialApp, and didChangeDependencies in builder.dart falls through to the plain HeroController() branch. Same story for CupertinoApp.

This isn't hypothetical the regression test reproduces it. Under a framework MaterialApp.router with a ShellRoute, on current main go_router installs a plain HeroController on both the root and the nested
shell navigator (the test counts 0 material controllers); with the fix it's 2. The test fails on the unpatched library and passes after. Happy to point you at the exact assertion.

On semver: agreed that the 18.0.0 major bump signals breaking changes but the intended breakage is the SDK floor and go_router's internal migration to material_ui, not a requirement that every consumer
swap their own MaterialApp for material_ui.MaterialApp. package:flutter/material.dart remains fully supported throughout the Material/Cupertino decoupling (flutter/flutter#184093) and is what the
overwhelming majority of apps still use. Silently degrading their shell-route Hero animations isn't a documented migration step it's the regression reported in flutter/flutter#192043. The fix is purely
additive: it keeps detecting material_ui/cupertino_ui and also recognizes the framework types, so both worlds work during the transition. Once the in-SDK libraries are sunset, the framework checks can simply
be dropped.

@Piinks

Piinks commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

The intention is that users will not migrate to 18.0.0 of go_router until they have migrated to material_ui. Supporting both libraries at the same time through material_ui is not something we want to do. material_ui and cupertino_ui do provide utilities for bridging the gap, allowing users to migrate to material_ui or cupertino_ui while their dependencies have not migrated yet. More details are in https://docs.flutter.dev/release/breaking-changes/material-ui-and-cupertino-ui

Since this is not a change we want to make, I am going to close it. Thanks for contributing!

@Piinks Piinks closed this Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[go_router] Hero animation regression between 17.5.0 and 18.0.0 in ShellRoute / StatefulShellRoute

2 participants