Fix ZoomPageTransitionsBuilder hardcoded fill color #154057
Conversation
github-actions
bot
added
framework
packages/flutter/test/material/page_transitions_theme_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter/test/material/page_transitions_theme_test.dart
Outdated
Show resolved
Hide resolved
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
packages/flutter/test/material/page_transitions_theme_test.dart
Outdated
Show resolved
Hide resolved
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Hey! Any updates on what is still needed for this to get merged? |
Some google tests are failing. I am working to get it merged. |
There was a problem hiding this comment.
There's another PR that seems to be fixing the same issue at #152815.
A question I have: is there a case were somebody may want a different color for the background/scrim than what comes from the theme? The other PR has an argument for the builder itself, so that can be set in the PageTransitionsTheme separate from the other color in the theme. So it might be good to add a parameter to ZoomTransitionBuilder similar to the other PR. But going further, would there be a need to set a different color for different pages in an app? I don't think wrapping different pages with a new theme would work.
Just took a peek over there, you're right, they seem to be doing the same. I'm all for more customization so I actually like the approach of it being a parameter. I would, however, keep the color we chose here as the default if none is passed. I saw they are struggling with testing the change, I don't believe it is worth keeping both PRs so that PR should copy my test and add the color discussed here as the default or I add the ZoomPageBuilder parameter on this PR, if we are to keep their idea. Both are fine by me, let me know what you think. (I would add another test if we follow this route, one for the default color and another for testing if passing the parameter color changes it) |
|
I think after thinking about it more, let's get this PR merged as is, and the other PR can build on top of it. The other case I mentioned can wait to see if it actually becomes an issue. |
|
For the Google testing failures, I reached out to both scuba owners, and they've approved updating the scubas with the changes in this PR. So we can go ahead and merge this PR irrespective of those failures. |
flutter/flutter@45ef8f3...2e221e7 2024-09-06 leroux_bruno@yahoo.fr Fix DropdownMenu focused item styles (flutter/flutter#153159) 2024-09-06 phucloi.nguyen@manabie.com Support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods. (flutter/flutter#154048) 2024-09-06 51940183+Sameri11@users.noreply.github.com [tool] Add `dartFileName` setting for platform plugins (flutter/flutter#153099) 2024-09-06 reidbaker@google.com [Conductor] Add ability to override mirror, add tests for default arg parsing and custom arg parsing (flutter/flutter#154363) 2024-09-06 59215665+davidhicks980@users.noreply.github.com Improve CupertinoPopupSurface appearance (flutter/flutter#151430) 2024-09-06 engine-flutter-autoroll@skia.org Roll Packages from 71e827e to 56df73e (1 revision) (flutter/flutter#154725) 2024-09-06 goderbauer@google.com Quick access to style guide (flutter/flutter#154689) 2024-09-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from c50eb8a65097 to 015f3b1dec53 (2 revisions) (#154691)" (flutter/flutter#154726) 2024-09-05 737941+loic-sharma@users.noreply.github.com Improve iOS unpack target's error messages (flutter/flutter#154649) 2024-09-05 30870216+gaaclarke@users.noreply.github.com Made some pixel tests fuzzy (flutter/flutter#154680) 2024-09-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from c50eb8a65097 to 015f3b1dec53 (2 revisions) (flutter/flutter#154691) 2024-09-05 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 7.0.0 to 7.0.1 (flutter/flutter#154690) 2024-09-05 36861262+QuncCccccc@users.noreply.github.com Normalize Dialog theme (flutter/flutter#153982) 2024-09-05 chris@bracken.jp iOS,macOS: Do not copy unsigned_binaries.txt to build outputs (flutter/flutter#154684) 2024-09-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from e042ff5df7af to c50eb8a65097 (1 revision) (flutter/flutter#154679) 2024-09-05 34871572+gmackall@users.noreply.github.com Add proguard rule to keep the class for all implementations of FlutterPlugin (flutter/flutter#154677) 2024-09-05 bruno.leroux@gmail.com Fix DropdownMenu menu does not follow the text field (flutter/flutter#154667) 2024-09-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from a156e713f4dc to e042ff5df7af (1 revision) (flutter/flutter#154678) 2024-09-05 103767860+dy0gu@users.noreply.github.com Fix ZoomPageTransitionsBuilder hardcoded fill color (flutter/flutter#154057) 2024-09-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 34b61eb53b99 to a156e713f4dc (1 revision) (flutter/flutter#154672) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
|
Hello I am currently facing the same Issue on latest Flutter version 3.24.2, when is this merge expected to be reflected in flutter? thank you |
While it's available on master now, it will be in the next major stable build. https://github.com/flutter/flutter/blob/master/docs/releases/Where's-my-commit.md |
Fixes: #115275
Fixes: #116127
Fixes: #126682
Continuing on: #139078 (Credits to @LowLevelSubmarine for his initial work!)
When using
ZoomPageTransitionsBuilder, which is the default forThemeDatawith aMaterialApp, dark edges would show around the exiting page that was being zoomed out in the background. Other times, a scrim (what looked like a slightly transparent dark overlay over the page) would appear.After some experimenting it was concluded that, in the first case, this was because both pages don't fully fill the enclosing scaffold area during the transition and the color for filling the remaining space was set hard coded as
Colors.black. The second case (scrim) happens when navigating from a page with an enclosing scaffold to a nested one, without a scaffold, unlike the first case that happens when both pages have a (different) enclosing scaffold, except this time it would be the hard coded color covering the page with a slight opacity reduction.Changes
Replaced the hard coded color for transition filling with the current
ThemeData.colorScheme.surfaceAdded a RenderBox based test to verify the correct color is being used in the transition.
Preview
Before, notice the dark outline flash when navigating to the first page and the scrim when navigating to the second:
flutter_before.mp4
After, using the theme relative color (in this case the default white) to replace the hard coded value:
flutter_after.mp4
Pre-launch Checklist