fix: ZoomPageTransitionsBuilder always has black outlines during transitions #139078
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
github-actions
bot
added
framework
|
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. |
Regarding the missing testsI've never coded any tests in Flutter so i dont really know where to start. I'd like to know if this approach is acceptable for the flutter team first, before investing my time into writing tests for this PR. |
LowLevelSubmarine
changed the title
|
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. |
|
@HansMuller thank you for taking a look at my MR. I've written a comment above regarding the missing tests. Are you in a position where you can tell me if my approach is acceptable, so that i can go on writing the tests? |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. 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. |
|
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. |
|
Hi @LowLevelSubmarine! Are you still planning to add tests and keep working on this PR? |
|
Hey @LowLevelSubmarine thank you for the PR, since we have not heard back, I am going to close it for now. If you would like to continue working on this, we can reopen it. Thank you! |
|
Hey @Piinks, the changes here solve #126682 (closed due to waiting on a framework fix) over at If the flutter team is okay with they way this PR fixes it I believe the only thing left is to write a test. I fail to see how such a simple change could even merit a test, seeing as nothing is removed or added, we are simply switching the hard coded In any case, if you could give some pointers on how a test for this PR would look like I wouldn't mind taking a crack at it and having it reopened. Considering this is currently related to an issue with a P2 label (#116127), it would be great have it merged. |
|
Hi @dy0gu, thanks for reaching out. The framework requires tests to prevent your changes from being regressed. Someone could come along in the future and revert these changes, unless a test exists to at least alert them of the potential consequences of doing so. For this PR, a good test would be something like:
Feel free to draw inspiration from other tests in Thanks for your work so far, and we're looking forward to further changes so that we can reopen and merge this PR! |
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
Piinks
force-pushed
the
fix-zoom-page-transition-always-has-black-scrim
branch
from
129c579 to
1272d21
Compare
|
Hey @dy0gu can you open a new PR with these changes? I don't know that you'll be able to push changes to this PR in particular, so I am going to close it. |
|
Yea I noticed it when I first started working but completely forgot to tell you here that I would need to do that. I'll create a new PR as soon as I'm done and give credit for the initial changes I take from here, thanks again! |
|
For those coming to this PR from past threads, I have continued it at #154057, this time with tests! |
Fixes: #115275 Fixes: #116127 Fixes: #126682 Continuing on: #139078 (Credits to @LowLevelSubmarine for his initial work!) When using `ZoomPageTransitionsBuilder`, which is the default for `ThemeData` with a `MaterialApp`, 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.surface` - Added 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:** https://github.com/user-attachments/assets/b4cc8658-1008-49f4-8553-abd5fcc72989 **After, using the theme relative color (in this case the default white) to replace the hard coded value:** https://github.com/user-attachments/assets/b70f42d2-6246-4964-99d1-34ff8051ab06 ## 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. - [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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
4 participants
When using ZoomPageTransitionsBuilder which is the default for ThemeData on Android, we always get black edges around the exiting page that is beeing zoomed out in the background. This is because both pages dont fully fill the enclosing navigators area opaque and the color for filling these pixels is always set to black. This PR aims to change this color to a more pleasent one: The current themes canvasColor.
fixes: #115275, #116127
Before
unfixed.webm
After
fixed.webm
Pre-launch Checklist
///). (there were no docs to change)If you need help, consider asking for advice on the #hackers-new channel on Discord.