- Notifications
You must be signed in to change notification settings - Fork30k
Comments
[CP-stable] fixes a status bar tap crash#182691
Draft
LongCatIsLooong wants to merge 4 commits intoflutter:flutter-3.41-candidate.0from
Draft
[CP-stable] fixes a status bar tap crash#182691LongCatIsLooong wants to merge 4 commits intoflutter:flutter-3.41-candidate.0from
LongCatIsLooong wants to merge 4 commits intoflutter:flutter-3.41-candidate.0from
Conversation
…on status bar tap (flutter#182391)Relandflutter#179643, with[changes](flutter@3f7d345)tofixflutter#182233. The relandclosesflutter#177992,closesflutter#175606.The crash happened because `Scaffold.handleStatusBar` (and subsequently`ScrollController.animateTo`) is called on a scaffold that [has neverbeen laidout](flutter#182233 (comment)).Beforeflutter#179643, the iOS embedder sends a fake touch down pointer eventat `(0, 0)` to the framework, followed by a touch up pointer event atthe same location. That ensured only the foreground `Scaffold` couldclaim the tap gesture and handle the status bar tap.The fix tries to restore the previous behavior (where the iOS behaviorsends fake touches instead of platform messages) by performing ahit-test at `(0, 0)` before trying to scroll the primary controller of a`Scaffold` to the top, this should make sure that only the foregroundscaffold (in theory it's still possible for more than one Scaffold toreceive the hittest and handle the status bar tap event, but theforeground scaffold should block the hit-test on background ones insteadof relying on gesture disambiguation).- [ ] I read the [Contributor Guide] and followed the process outlinedthere for submitting PRs.- [ ] I read the [Tree Hygiene] wiki page, which explains myresponsibilities.- [ ] I read and followed the [Flutter Style Guide], including [Featureswe expect every widget to implement].- [ ] I signed the [CLA].- [ ] I listed at least one issue that this PR fixes in the descriptionabove.- [ ] 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 DrivenFixes] where supported.- [ ] All existing and new tests are passing.If you need help, consider asking for advice on the #hackers-new channelon [Discord].**Note**: The Flutter team is currently trialing the use of [Gemini CodeAssist forGitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).Comments from the `gemini-code-assist` bot should not be taken asauthoritative feedback from the Flutter team. If you find its commentsuseful you can update your code accordingly, but if you are unsure ordisagree with the feedback, please feel free to wait for a Flutter teammember's review for guidance on which automated comments should beaddressed.<!-- 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[Discord]:https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md[Data Driven Fixes]:https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This pull request was opened from and to a release candidate branch. This should only be done as part of the officialFlutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions atTree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
Contributor
There was a problem hiding this comment.
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 addresses a crash on iOS when tapping the status bar by ensuring only the topmost, hit-testable scaffold responds to the tap. This also improves the user experience for apps with multiple nested scaffolds. The implementation correctly uses a hit-testable widget in the status bar area to determine if a scaffold is in the foreground. The test changes are good, with a newsimulateStatusBarTap helper and tests covering the new behavior. My main feedback is on code duplication: the_HitTestableAtOrigin helper class is duplicated in bothmaterial/scaffold.dart andcupertino/page_scaffold.dart. This should be refactored into a shared utility to improve maintainability.
Comment on lines +3483 to +3519
| finalclass_HitTestableAtOriginextendsStatelessWidget { | ||
| const_HitTestableAtOrigin(this.globalKey); | ||
| finalGlobalKey globalKey; | ||
| /// Whether the render box of the[_HitTestableAtOrigin] widget associated | ||
| /// with the given global`key` is hit-testable at[Offset.zero]. | ||
| /// | ||
| /// This is used by the`handleStatusBarTap` implementation to avoid sending | ||
| /// status bar tap events to scroll views in offscreen subtrees. | ||
| staticboolhitTestableAtOrigin(GlobalKey key) { | ||
| final context= key.currentContextasElement?; | ||
| if (context==null) { | ||
| assert( | ||
| false, | ||
| 'BuildContext associated with $key is not mounted. ' | ||
| 'If you see this in a test, this is likely because the test was trying ' | ||
| 'to simulate status bar tap on a non-iOS platform', | ||
| ); | ||
| returnfalse; | ||
| } | ||
| final renderObject= context.renderObject!asRenderMetaData; | ||
| finalint viewId=View.of(context).viewId; | ||
| final result=HitTestResult(); | ||
| WidgetsBinding.instance.hitTestInView(result,Offset.zero, viewId); | ||
| return result.path.any((HitTestEntry entry)=> entry.target== renderObject); | ||
| } | ||
| @override | ||
| Widgetbuild(BuildContext context) { | ||
| returnMetaData( | ||
| key: globalKey, | ||
| behavior:HitTestBehavior.translucent, | ||
| child:constSizedBox.expand(), | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The_HitTestableAtOrigin class is nearly identical to the one inpackages/flutter/lib/src/cupertino/page_scaffold.dart. Duplicating code like this makes maintenance harder and violates the DRY (Don't Repeat Yourself) principle.
To improve maintainability, this class should be extracted to a shared location (e.g., a new file underlib/src/widgets/) and made public. The slightly differentassert messages in the two versions can be merged into a single, more descriptive one.
References
- The style guide advises to 'Avoid duplicating state', which by extension applies to code logic as well to maintain a single source of truth and improve maintainability.(link)
…l views on status bar tap (flutter#182391)"This reverts commitd2eeab6.
…9643)" (flutter#182223)This reverts commitf4c83de.See b/482565401.## Pre-launch Checklist- [x] I read the [Contributor Guide] and followed the process outlinedthere for submitting PRs.- [x] I read the [Tree Hygiene] wiki page, which explains myresponsibilities.- [x] I read and followed the [Flutter Style Guide], including [Featureswe expect every widget to implement].- [x] I signed the [CLA].- [x] I listed at least one issue that this PR fixes in the descriptionabove.- [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 DrivenFixes] where supported.- [x] All existing and new tests are passing.If you need help, consider asking for advice on the #hackers-new channelon [Discord].**Note**: The Flutter team is currently trialing the use of [Gemini CodeAssist forGitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).Comments from the `gemini-code-assist` bot should not be taken asauthoritative feedback from the Flutter team. If you find its commentsuseful you can update your code accordingly, but if you are unsure ordisagree with the feedback, please feel free to wait for a Flutter teammember's review for guidance on which automated comments should beaddressed.<!-- 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[Discord]:https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md[Data Driven Fixes]:https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…on status bar tap (flutter#182391)Relandflutter#179643, with[changes](flutter@3f7d345)tofixflutter#182233. The relandclosesflutter#177992,closesflutter#175606.The crash happened because `Scaffold.handleStatusBar` (and subsequently`ScrollController.animateTo`) is called on a scaffold that [has neverbeen laidout](flutter#182233 (comment)).Beforeflutter#179643, the iOS embedder sends a fake touch down pointer eventat `(0, 0)` to the framework, followed by a touch up pointer event atthe same location. That ensured only the foreground `Scaffold` couldclaim the tap gesture and handle the status bar tap.The fix tries to restore the previous behavior (where the iOS behaviorsends fake touches instead of platform messages) by performing ahit-test at `(0, 0)` before trying to scroll the primary controller of a`Scaffold` to the top, this should make sure that only the foregroundscaffold (in theory it's still possible for more than one Scaffold toreceive the hittest and handle the status bar tap event, but theforeground scaffold should block the hit-test on background ones insteadof relying on gesture disambiguation).- [ ] I read the [Contributor Guide] and followed the process outlinedthere for submitting PRs.- [ ] I read the [Tree Hygiene] wiki page, which explains myresponsibilities.- [ ] I read and followed the [Flutter Style Guide], including [Featureswe expect every widget to implement].- [ ] I signed the [CLA].- [ ] I listed at least one issue that this PR fixes in the descriptionabove.- [ ] 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 DrivenFixes] where supported.- [ ] All existing and new tests are passing.If you need help, consider asking for advice on the #hackers-new channelon [Discord].**Note**: The Flutter team is currently trialing the use of [Gemini CodeAssist forGitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).Comments from the `gemini-code-assist` bot should not be taken asauthoritative feedback from the Flutter team. If you find its commentsuseful you can update your code accordingly, but if you are unsure ordisagree with the feedback, please feel free to wait for a Flutter teammember's review for guidance on which automated comments should beaddressed.<!-- 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[Discord]:https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md[Data Driven Fixes]:https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
a: tests"flutter test", flutter_test, or one of our tests f: cupertinoflutter/packages/flutter/cupertino repository f: material designflutter/packages/flutter/material repository. frameworkflutter/packages/flutter repository. See also f: labels.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
This pull request is manually created because there were merge conflicts.
Click here for the conflicts
Issue Link:
What is the link to the issue this cherry-pick is addressing?
#182233
The bug was introduced in#179643 which was cherry picked to 3.41 beta (so the fix should exist on both current beta and stable).
Impact Description:
On iOS, the app may crash when the user taps on the status bar to scroll to top, when the app has a primary scroll view that has never been laid out (for example, in a route that has been obstructed by other routes).
Changelog Description:
Additionally this patch only dispatches the "scroll to top" command to the topmost Scaffold which is generally better UX for most apps (this doesn't have to be in the changlog)
Workaround:
No known easy workarounds.
Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
What are the steps to validate that this fix works?
I've manually tested the fix on iOS/iPadOS simulators. There are also unit tests.