Feat: Add opaque, isActive, isFirst, popDisposition aspects for ModalRoute by rkishan516 · Pull Request #167324 · flutter/flutter

rkishan516 · 2025-04-17T02:56:47Z

Feat: Add opaque, isActive, isFirst, popDisposition aspects for ModalRoute
fixes: #167058
fixes: #162009

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

justinmc

See my comment about more tests, otherwise I think this looks good. I like encouraging the use of InheritedModel for performance.

justinmc · 2025-04-22T22:09:54Z

packages/flutter/test/widgets/routes_test.dart

      );
    });
+
+    testWidgets('ModalRoute.isFirstOf only rebuilds when first route state changes', (


Would you mind adding a similar test for each of the other of methods that you added as well?

Sure, I will add.

justinmc

Thanks for adding all the tests. One comment about the docs, otherwise this looks good.

It seems like this should be merged regardless of the outcome of your PR #165289, right?

justinmc · 2025-04-29T23:00:43Z

packages/flutter/lib/src/widgets/routes.dart

      _of(context, _ModalRouteAspect.settings)?.settings;

+  /// Returns [ModalRoute.isActive] for the modal route most closely associated
+  /// with the given context.


I see that settingsOf and others above include a bit more in their docs, should you add them for the new parameters too?

/// Returns null if the given context is not associated with a modal route. /// /// Use of this method will cause the given [context] to rebuild any time that /// the [ModalRoute.settings] property of the ancestor [_ModalScopeStatus] changes.

Sure, thanks for the suggestion.

We should not mention _ModalScopeStatus in the doc since it's a private widget.

I also consulted with AI, who said the original version might be slightly ambiguous (since the reader might the the BuildContext itself would be rebuilt) and suggested the following. What do you think?

/// Calling this method creates a dependency on the [ModalRoute] associated /// with the given [context]. As a result, the widget corresponding to [context] /// will be rebuilt whenever the route's [ModalRoute.settings] changes.

I agree with not mentioning private members and I like the rewrite 👍

rkishan516 · 2025-04-30T00:46:56Z

It seems like this should be merged regardless of the outcome of your PR #165289, right?

I don't know. Because if we switch to selector pattern, than these will become just helper methods and soon we need to deprecate. I will rather wait for #165289, that which direction it will go and based on that take a call on this. Wdyt ?

navaronbracke · 2025-04-30T17:44:41Z

@rkishan516 This should also fix #162009 right? You can add that to the OP then.

rkishan516 · 2025-04-30T17:47:13Z

@rkishan516 This should also fix #162009 right? You can add that to the OP then.

Done, have updated.

justinmc · 2025-05-05T18:16:20Z

It seems like this should be merged regardless of the outcome of your PR #165289, right?

I don't know. Because if we switch to selector pattern, than these will become just helper methods and soon we need to deprecate. I will rather wait for #165289, that which direction it will go and based on that take a call on this. Wdyt ?

Ok I agree, let's wait for #165289. So this PR can stay open for now. I'll prioritize reviewing that one.

justinmc

LGTM 👍

It looks like #165289 maybe stalled out so I want to LGTM this in case we want to merge this without it. There are a few PRs that depend on this one now: #167032, #168277

rkishan516 · 2025-05-17T02:11:24Z

It looks like #165289 maybe stalled out so I want to LGTM this in case we want to merge this without it. There are a few PRs that depend on this one now: #167032, #168277

Yes @justinmc, I don't think we will be going towards selector, so it make sense to merge this.

dkwingsmt

LGTM except for the doc

dkwingsmt · 2025-05-21T23:33:32Z

packages/flutter/lib/src/widgets/routes.dart

      _of(context, _ModalRouteAspect.settings)?.settings;

+  /// Returns [ModalRoute.isActive] for the modal route most closely associated
+  /// with the given context.


We should not mention _ModalScopeStatus in the doc since it's a private widget.

I also consulted with AI, who said the original version might be slightly ambiguous (since the reader might the the BuildContext itself would be rebuilt) and suggested the following. What do you think?

/// Calling this method creates a dependency on the [ModalRoute] associated /// with the given [context]. As a result, the widget corresponding to [context] /// will be rebuilt whenever the route's [ModalRoute.settings] changes.

…Route

rkishan516 · 2025-05-22T01:39:48Z

We should not mention _ModalScopeStatus in the doc since it's a private widget.

I have updated the docs based on your suggestion.

justinmc

Renewing my LGTM 👍 .

#165289 was closed so I'll add autosubmit here.

…Route (flutter#167324) Feat: Add opaque, isActive, isFirst, popDisposition aspects for ModalRoute fixes: flutter#167058 fixes: flutter#162009 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.

…or ModalRoute (flutter/flutter#167324)

…rdsPageTransitionsBuilder (#167032) Reopens #165726 with the removed test that has become irrelevant. Depends on: #167324 Actual master: https://github.com/user-attachments/assets/28619355-9c1e-4f06-9ede-38c4dddd13df After fix: https://github.com/user-attachments/assets/a5f2ecf2-5d8e-445a-b5a9-a7d6c0e3ec5d ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: Justin McCandless <jmccandless@google.com> Co-authored-by: Qun Cheng <36861262+QuncCccccc@users.noreply.github.com>

…or ModalRoute (flutter/flutter#167324)

…rdsPageTransitionsBuilder (flutter#167032) Reopens flutter#165726 with the removed test that has become irrelevant. Depends on: flutter#167324 Actual master: https://github.com/user-attachments/assets/28619355-9c1e-4f06-9ede-38c4dddd13df After fix: https://github.com/user-attachments/assets/a5f2ecf2-5d8e-445a-b5a9-a7d6c0e3ec5d ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: Justin McCandless <jmccandless@google.com> Co-authored-by: Qun Cheng <36861262+QuncCccccc@users.noreply.github.com>

…or ModalRoute (flutter/flutter#167324)

…rdsPageTransitionsBuilder (flutter#167032) Reopens flutter#165726 with the removed test that has become irrelevant. Depends on: flutter#167324 Actual master: https://github.com/user-attachments/assets/28619355-9c1e-4f06-9ede-38c4dddd13df After fix: https://github.com/user-attachments/assets/a5f2ecf2-5d8e-445a-b5a9-a7d6c0e3ec5d ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: Justin McCandless <jmccandless@google.com> Co-authored-by: Qun Cheng <36861262+QuncCccccc@users.noreply.github.com>

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. labels Apr 17, 2025

TheLastFlame mentioned this pull request Apr 17, 2025

Fix visual overlap of transparent routes barrier when using FadeForwardsPageTransitionsBuilder #167032

Merged

9 tasks

justinmc reviewed Apr 22, 2025

View reviewed changes

rkishan516 force-pushed the modal-route-aspects branch from 288d8ac to 721d47a Compare April 23, 2025 01:40

justinmc reviewed Apr 29, 2025

View reviewed changes

rkishan516 force-pushed the modal-route-aspects branch from 721d47a to a1e40d0 Compare April 30, 2025 01:06

justinmc approved these changes May 16, 2025

View reviewed changes

justinmc requested a review from dkwingsmt May 20, 2025 22:11

dkwingsmt reviewed May 21, 2025

View reviewed changes

dkwingsmt approved these changes May 21, 2025

View reviewed changes

rkishan516 added 2 commits May 22, 2025 07:03

Feat: Add opaque, isActive, isFirst, popDisposition aspects for Modal…

532f981

…Route

docs: update docs for modal route aspectsOf methods

27bfdc5

rkishan516 force-pushed the modal-route-aspects branch from a1e40d0 to 27bfdc5 Compare May 22, 2025 01:39

justinmc approved these changes May 29, 2025

View reviewed changes

justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label May 29, 2025

auto-submit bot added this pull request to the merge queue May 29, 2025

Merged via the queue into flutter:master with commit df29894 May 29, 2025
76 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 29, 2025

engine-flutter-autoroll mentioned this pull request May 30, 2025

Roll Flutter from 0e536eb9fe4b to 8132e1828561 (46 revisions) flutter/packages#9343

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 30, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

5d8b237

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll mentioned this pull request May 30, 2025

Roll Flutter from 0e536eb9fe4b to 0b02682934ef (47 revisions) flutter/packages#9344

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

4431979

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

bb033f5

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

1754172

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

ce91d92

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

04d1957

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

036ce33

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

e9ab692

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

df753c3

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

ab98813

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

9c85ff2

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

af5f7fb

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

cee198a

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

e246dea

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

f6c7767

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

8e1fd28

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

5c535bd

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

153a9f6

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

3dbc0b4

…or ModalRoute (flutter/flutter#167324)

Piinks mentioned this pull request Jun 6, 2025

Flutter Notable Commits - 2025 #161460

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

4d785c1

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

260c428

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

d6e5e84

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

15f1651

…or ModalRoute (flutter/flutter#167324)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2025

df29894 Feat: Add opaque, isActive, isFirst, popDisposition aspects f…

b317c6c

…or ModalRoute (flutter/flutter#167324)

Conversation

rkishan516 commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

rkishan516 Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

rkishan516 Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

dkwingsmt May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc May 21, 2025

Choose a reason for hiding this comment

Uh oh!

rkishan516 commented Apr 30, 2025

Uh oh!

navaronbracke commented Apr 30, 2025

Uh oh!

rkishan516 commented Apr 30, 2025

Uh oh!

justinmc commented May 5, 2025

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

rkishan516 commented May 17, 2025

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkishan516 commented May 22, 2025

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rkishan516 commented Apr 17, 2025 •

edited

Loading

dkwingsmt May 21, 2025 •

edited

Loading

dkwingsmt May 21, 2025 •

edited

Loading