ScaffoldMessenger only shows to root nested Scaffold by Piinks · Pull Request #74093 · flutter/flutter

Piinks · 2021-01-16T00:54:15Z

This change makes it such that when a ScaffoldMessenger has nested Scaffold descendants, it will only show SnackBars on the root Scaffold of the subtree. This will remove the possibility of duplicate SnackBars showing up in the UI in this use case.

This change is courtesy of early customer feedback. :)
See b/171649782

Fixes #69929

Other options discussed were to

assert for nested Scaffolds
- this makes for a really poor experience trying to migrate to the new API. Especially in complex applications, determining the right scopes to create for ScaffoldMessenger & nested Scaffolds is difficult.
introduce a ScaffoldMessengerBehavior a la present to root or present to leaf for nested Scaffolds.
- When evaluating the leaf option, we discovered it would not work properly in some use cases, and could not find a case where it was explicitly required. Since you can close or tighten the scope of the ScaffoldMessenger by instantiating your own further down the tree, having this option seemed unnecessary, with the root option making the most sense for the issue at hand.

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 listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

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

skia-gold · 2021-01-16T01:01:22Z

Gold has detected about 2 untriaged digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/74093

skia-gold · 2021-01-16T01:07:22Z

Gold has detected about 1 untriaged digest(s) on patchset 2.
View them at https://flutter-gold.skia.org/cl/github/74093

xu-baolin · 2021-01-18T08:18:58Z

Just update your branch can fix the CI checks :)

…Msgr

…tScaffoldMsgr

skia-gold · 2021-01-19T20:36:22Z

Gold has detected about 2 untriaged digest(s) on patchset 4.
View them at https://flutter-gold.skia.org/cl/github/74093

flutter-dashboard · 2021-01-19T21:15:57Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #74093 at sha c8587dc

HansMuller

LGTM

HansMuller · 2021-01-20T01:29:17Z

    for (final ScaffoldState scaffold in _scaffolds) {
-      scaffold._updateSnackBar();
+      if (_isRoot(scaffold))
+        scaffold._updateSnackBar();


At this point it would be safe to return.

Not necessarily. There can be other Scaffolds that need to get a SnackBar. For example, on another route. That is how we get the nice hero animation as the SnackBar persists across the transition.

I see. Hopefully there's a test that will catch someone (like me) adding the short-circuit return without realizing the implications

HansMuller · 2021-01-20T01:37:46Z

+      ),
+    ));
+
+    final ScaffoldMessengerState scaffoldMessengerState = tester.state(


With the type parameter: tester.state<ScaffoldMessengerState>(

It's a shame that the same identifier has to be repeated 3x :-(

What is being repeated 3x?

final ScaffoldMessengerState scaffoldMessengerState = tester.state<ScaffoldMessengerState>(

goderbauer

LGTM

goderbauer · 2021-01-20T17:27:33Z

+/// ScaffoldMessenger will only present a [SnackBar] to the root Scaffold of
+/// the subtree of Scaffolds. In order to show SnackBars in the inner, nested
+/// Scaffolds, set a new scope for your SnackBars by instantiating a new
+/// ScaffoldMessenger in between the levels of nesting.


optional: may be nice to show a code sample of that here.

What do you think of linking to an example in the gallery? I think this sample would be very large to include inline, and I don't want to encourage nesting scaffolds... there is a section in the Scaffold docs that says nested Scaffolds is not typically necessary. There are a few cases where it is relevant - like in the gallery where there is embedded Flutter content, but the Scaffold docs recommend avoiding nesting.

Outdated

skia-gold · 2021-01-20T21:27:22Z

Gold has detected about 1 untriaged digest(s) on patchset 5.
View them at https://flutter-gold.skia.org/cl/github/74093

flutter-dashboard · 2021-01-20T22:06:56Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #74093 at sha 04b33f0

ScaffoldMEssenger only shows to root nested Scaffold

5fc5cc4

Piinks added framework flutter/packages/flutter repository. See also f: labels. a: quality A truly polished experience customer: money (g3) a: layout SystemChrome and Framework's Layout Issues labels Jan 16, 2021

flutter-dashboard Bot added the f: material design flutter/packages/flutter/material repository. label Jan 16, 2021

google-cla Bot added the cla: yes label Jan 16, 2021

Piinks commented Jan 16, 2021

View reviewed changes

Comment thread packages/flutter/test/material/snack_bar_test.dart Outdated

Update packages/flutter/test/material/snack_bar_test.dart

a8bbb62

Piinks requested a review from HansMuller January 16, 2021 01:00

xu-baolin previously requested changes Jan 18, 2021

View reviewed changes

Comment thread packages/flutter/lib/src/material/scaffold.dart Outdated

Comment thread packages/flutter/lib/src/material/scaffold.dart

xu-baolin reviewed Jan 18, 2021

View reviewed changes

Comment thread packages/flutter/lib/src/material/scaffold.dart

xu-baolin self-requested a review January 18, 2021 08:03

Piinks added 2 commits January 19, 2021 14:25

Merge branch 'master' of github.com:flutter/flutter into rootScaffold…

9a2c2d0

…Msgr

Merge branch 'rootScaffoldMsgr' of github.com:Piinks/flutter into roo…

c8587dc

…tScaffoldMsgr

flutter-dashboard Bot added the will affect goldens Changes to golden files label Jan 19, 2021

HansMuller approved these changes Jan 20, 2021

View reviewed changes

goderbauer approved these changes Jan 20, 2021

View reviewed changes

Review feedback

04b33f0

Piinks added the waiting for tree to go green label Jan 21, 2021

fluttergithubbot merged commit eb9bac6 into flutter:master Jan 22, 2021

Piinks mentioned this pull request Mar 5, 2021

Fix missing root Scaffold check in ScaffoldMessenger #77410

Merged

8 tasks

Uh oh!

Conversation

Piinks commented Jan 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

skia-gold commented Jan 16, 2021

Uh oh!

skia-gold commented Jan 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xu-baolin commented Jan 18, 2021

Uh oh!

skia-gold commented Jan 19, 2021

Uh oh!

flutter-dashboard Bot commented Jan 19, 2021

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

HansMuller Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

Piinks Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

HansMuller Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HansMuller Jan 20, 2021 • 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!

Piinks Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

HansMuller Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

Piinks Jan 20, 2021 • 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!

skia-gold commented Jan 20, 2021

Uh oh!

flutter-dashboard Bot commented Jan 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Piinks commented Jan 16, 2021 •

edited

Loading

HansMuller Jan 20, 2021 •

edited

Loading

Piinks Jan 20, 2021 •

edited

Loading