Repository navigation
test: dynamic _tapOutside helper for TapRegion navigation tests - #185397
Conversation
Move three testWidgets that use MaterialApp/Scaffold/ElevatedButton/ MaterialPageRoute from test/widgets/tap_region_test.dart into a new test/material/tap_region_test.dart file. Replace flutter/material.dart import with flutter/widgets.dart in the widgets test file since the moved tests were the only Material usages. Part of flutter#177414
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request moves several regression tests for TapRegion from packages/flutter/test/widgets/tap_region_test.dart to a new file packages/flutter/test/material/tap_region_test.dart to correctly group tests that depend on the Material library. A review comment identifies an opportunity to refactor a duplicated helper function, remove an unused parameter, and correct an inaccurate internal comment.
|
Thank you for the review, @gemini-code-assist! All three suggestions have been addressed:
|
|
Thanks for the update, @shivanshu877. The refactoring of |
justinmc
left a comment
There was a problem hiding this comment.
I think these tests should probably stay in widgets and be refactored to not use material.
Per @justinmc's review on flutter#185397, the TapRegion regression tests for issues flutter#153093 and the consumeOutsideTaps post-navigation issue exercise widget-level Navigator behavior. They only used Material because the original repros wrapped pages in Scaffold/MaterialApp/ MaterialPageRoute, none of which are required to reproduce the bugs. This commit: * Deletes packages/flutter/test/material/tap_region_test.dart * Adds the same three tests to packages/flutter/test/widgets/tap_region_test.dart * Replaces MaterialApp + MaterialPageRoute with WidgetsApp + PageRouteBuilder * Replaces Scaffold + FloatingActionButton + ElevatedButton with Stack + GestureDetector (with HitTestBehavior.opaque for hit-testing) * Switches the tapOutside helper to a fixed (200, 200) coordinate since there is no Scaffold to anchor to — both regions sit at center of the 800x600 test surface, so (200, 200) is reliably outside. * Drives navigation through a GlobalKey<NavigatorState> rather than via context lookups on Material widgets. All 15 TapRegion tests pass.
victorsanni
left a comment
There was a problem hiding this comment.
Can you change the PR title and description to match the current status?
Per @victorsanni's review on flutter#185397, replaced the inline WidgetsApp+PageRouteBuilder boilerplate (and the local _navigatorAppWith helper) with TestWidgetsApp, which already provides the same wrapping. The two-page initial-routes setup is now done by push()ing tapRegion2 onto a TestWidgetsApp(home: tapRegion1) instead of configuring onGenerateInitialRoutes manually. All 15 TapRegion tests still pass.
|
@victorsanni — updated the PR title and description to reflect the actual change (refactoring all three tests off Material rather than just moving them). Thanks for the review! |
Per @justinmc's review on flutter#185397, the _tapOutside helper now finds the currently visible TapRegion's RenderBox via its ValueKey and taps outside it (region.localToGlobal(Offset.zero) + Offset(200,200)), mirroring the original material/-based tapOutside helper. This removes the hardcoded (200, 200) coordinate and the implicit assumption about the test surface size, and answers @justinmc's question about why the region keys were declared but never read — they're now used to locate the region for the tap. All 15 TapRegion tests still pass.
Master (flutter#185567) already merged the move/refactor of TapRegion navigation tests off Material. This merge adopts that version of test/widgets/tap_region_test.dart and re-applies the small robustness improvement on top: * Replace the two duplicated `Future<void> tapOutside(...)` local closures with a single top-level `_tapOutside(tester, regionKey)` helper that locates the visible region's RenderBox dynamically (renderBox.localToGlobal(Offset.zero) + Offset(200, 200)) instead of using a hardcoded global coordinate. This addresses @justinmc's review feedback that the original material/-based tapOutside was more robust than the closure with hardcoded coords.
Per @victorsanni's review on flutter#185397: the previous _tapOutside used `renderBox.localToGlobal(Offset.zero) + Offset(200, 200)`, which is only outside the region by coincidence (because the regions in the tests happen to be smaller than 200x200). It still relies on a hardcoded displacement. Compute the region's global rect and tap at `rect.bottomRight + Offset(1, 1)`, which is mathematically guaranteed to be outside the region regardless of its size. An assert near the tap site documents the invariant. All 15 TapRegion tests still pass.
@victorsanni's inline review comment on flutter#185397 pointed out that the old sanity-check assertion was tautological: rect.contains() on a point one pixel past rect.bottomRight is false by definition. Replaced with a check that catches the actual failure mode: if the region under test is positioned so close to the bottom-right that the 'just outside' tap escapes the default 800x600 flutter_test surface, flutter_test silently no-ops the gesture and the whole test passes vacuously. Asserting the surface bound catches that layout mistake loudly with a message pointing at the responsible tap coordinate. Verified locally: all 16 tests in tap_region_test.dart pass.
| // Sanity-check: the tap must land within the default flutter_test surface | ||
| // (800×600). If the region under test is positioned so close to the | ||
| // bottom-right corner that the "just outside" tap escapes the surface, | ||
| // flutter_test won't dispatch a real hit — silently no-oping the whole | ||
| // test. Asserting the surface bound catches that layout mistake loudly. |
There was a problem hiding this comment.
I don't think this comment is necessary, the assert message makes it obvious.
Replaces the hardcoded 800x600 rect in the _tapOutside assert with the logical size of the test view, so the bound follows a resized surface instead of producing a false failure on one. Also drops the explanatory comment above the assert, which the assert message already covers.
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍 . Thanks for following up with this!
flutter/flutter@c8c5e3b...0cbd1a4 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from 6be7f8547c3c to 3911a1fe7f7a (1 revision) (flutter/flutter#192110) 2026-09-01 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from EPzxifoyt36b5qiBy... to idslm9FikVLy2K_A-... (flutter/flutter#192104) 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from 47df2ae3226c to 6be7f8547c3c (1 revision) (flutter/flutter#192103) 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from e22ebf131e44 to 47df2ae3226c (4 revisions) (flutter/flutter#192099) 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from 5489a16a5998 to e22ebf131e44 (5 revisions) (flutter/flutter#192093) 2026-09-01 bkonyi@google.com [flutter_tools] Safely handle broken symlinks and existing files during plugin symlink creation (flutter/flutter#191496) 2026-09-01 97480502+b-luk@users.noreply.github.com Support wide gamut colors in gradient_generator's CreateGradientTexture (flutter/flutter#191980) 2026-09-01 47866232+chunhtai@users.noreply.github.com Clean up semantics code (flutter/flutter#191620) 2026-08-31 49662805+jesskuras@users.noreply.github.com Add website documentation item to PR pre-launch checklist (flutter/flutter#192080) 2026-08-31 kiran@kryali.com [windows] Fix null-deref in HostWindowPopup::UpdatePosition (Fixes #191478) (flutter/flutter#191479) 2026-08-31 47866232+chunhtai@users.noreply.github.com Removes deprecated ignoreSemantics parementers (flutter/flutter#191493) 2026-08-31 engine-flutter-autoroll@skia.org Roll Dart SDK from b319095e317b to 9164def35347 (1 revision) (flutter/flutter#192073) 2026-08-31 112751483+shivanshu877@users.noreply.github.com test: dynamic _tapOutside helper for TapRegion navigation tests (flutter/flutter#185397) 2026-08-31 git@reb0.org [Windows] fix: Remove quotes from compiler warning suppression (flutter/flutter#190873) 2026-08-31 47866232+chunhtai@users.noreply.github.com Migrate to listen package (flutter/flutter#189111) 2026-08-31 engine-flutter-autoroll@skia.org Roll Skia from 15db98a90bbd to 5489a16a5998 (2 revisions) (flutter/flutter#192070) 2026-08-31 110348311+Devasy@users.noreply.github.com Add regression test for plugin compileSdkExtension warning (flutter/flutter#191281) 2026-08-31 34871572+gmackall@users.noreply.github.com Explicitly disable HCPP in platform view benchmarks and integration tests (flutter/flutter#191908) 2026-08-31 matt.boetger@gmail.com Documentation and script for Gradle Distribution cache for CI (flutter/flutter#190323) 2026-08-31 engine-flutter-autoroll@skia.org Roll Skia from 5549c93c9a1c to 15db98a90bbd (2 revisions) (flutter/flutter#192068) 2026-08-31 engine-flutter-autoroll@skia.org Roll Packages from cd4cdd0 to d642322 (7 revisions) (flutter/flutter#192063) 2026-08-31 engine-flutter-autoroll@skia.org Roll Dart SDK from 48f641e8b249 to b319095e317b (1 revision) (flutter/flutter#192061) 2026-08-31 269567208+reidbaker-agent@users.noreply.github.com [tool] Migrate dev/tools from dart_skills_lint to skills_lint package (flutter/flutter#191997) 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 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
…r#12712) flutter/flutter@c8c5e3b...0cbd1a4 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from 6be7f8547c3c to 3911a1fe7f7a (1 revision) (flutter/flutter#192110) 2026-09-01 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from EPzxifoyt36b5qiBy... to idslm9FikVLy2K_A-... (flutter/flutter#192104) 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from 47df2ae3226c to 6be7f8547c3c (1 revision) (flutter/flutter#192103) 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from e22ebf131e44 to 47df2ae3226c (4 revisions) (flutter/flutter#192099) 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from 5489a16a5998 to e22ebf131e44 (5 revisions) (flutter/flutter#192093) 2026-09-01 bkonyi@google.com [flutter_tools] Safely handle broken symlinks and existing files during plugin symlink creation (flutter/flutter#191496) 2026-09-01 97480502+b-luk@users.noreply.github.com Support wide gamut colors in gradient_generator's CreateGradientTexture (flutter/flutter#191980) 2026-09-01 47866232+chunhtai@users.noreply.github.com Clean up semantics code (flutter/flutter#191620) 2026-08-31 49662805+jesskuras@users.noreply.github.com Add website documentation item to PR pre-launch checklist (flutter/flutter#192080) 2026-08-31 kiran@kryali.com [windows] Fix null-deref in HostWindowPopup::UpdatePosition (Fixes #191478) (flutter/flutter#191479) 2026-08-31 47866232+chunhtai@users.noreply.github.com Removes deprecated ignoreSemantics parementers (flutter/flutter#191493) 2026-08-31 engine-flutter-autoroll@skia.org Roll Dart SDK from b319095e317b to 9164def35347 (1 revision) (flutter/flutter#192073) 2026-08-31 112751483+shivanshu877@users.noreply.github.com test: dynamic _tapOutside helper for TapRegion navigation tests (flutter/flutter#185397) 2026-08-31 git@reb0.org [Windows] fix: Remove quotes from compiler warning suppression (flutter/flutter#190873) 2026-08-31 47866232+chunhtai@users.noreply.github.com Migrate to listen package (flutter/flutter#189111) 2026-08-31 engine-flutter-autoroll@skia.org Roll Skia from 15db98a90bbd to 5489a16a5998 (2 revisions) (flutter/flutter#192070) 2026-08-31 110348311+Devasy@users.noreply.github.com Add regression test for plugin compileSdkExtension warning (flutter/flutter#191281) 2026-08-31 34871572+gmackall@users.noreply.github.com Explicitly disable HCPP in platform view benchmarks and integration tests (flutter/flutter#191908) 2026-08-31 matt.boetger@gmail.com Documentation and script for Gradle Distribution cache for CI (flutter/flutter#190323) 2026-08-31 engine-flutter-autoroll@skia.org Roll Skia from 5549c93c9a1c to 15db98a90bbd (2 revisions) (flutter/flutter#192068) 2026-08-31 engine-flutter-autoroll@skia.org Roll Packages from cd4cdd0 to d642322 (7 revisions) (flutter/flutter#192063) 2026-08-31 engine-flutter-autoroll@skia.org Roll Dart SDK from 48f641e8b249 to b319095e317b (1 revision) (flutter/flutter#192061) 2026-08-31 269567208+reidbaker-agent@users.noreply.github.com [tool] Migrate dev/tools from dart_skills_lint to skills_lint package (flutter/flutter#191997) 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 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
…r#12712) flutter/flutter@c8c5e3b...0cbd1a4 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from 6be7f8547c3c to 3911a1fe7f7a (1 revision) (flutter/flutter#192110) 2026-09-01 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from EPzxifoyt36b5qiBy... to idslm9FikVLy2K_A-... (flutter/flutter#192104) 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from 47df2ae3226c to 6be7f8547c3c (1 revision) (flutter/flutter#192103) 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from e22ebf131e44 to 47df2ae3226c (4 revisions) (flutter/flutter#192099) 2026-09-01 engine-flutter-autoroll@skia.org Roll Skia from 5489a16a5998 to e22ebf131e44 (5 revisions) (flutter/flutter#192093) 2026-09-01 bkonyi@google.com [flutter_tools] Safely handle broken symlinks and existing files during plugin symlink creation (flutter/flutter#191496) 2026-09-01 97480502+b-luk@users.noreply.github.com Support wide gamut colors in gradient_generator's CreateGradientTexture (flutter/flutter#191980) 2026-09-01 47866232+chunhtai@users.noreply.github.com Clean up semantics code (flutter/flutter#191620) 2026-08-31 49662805+jesskuras@users.noreply.github.com Add website documentation item to PR pre-launch checklist (flutter/flutter#192080) 2026-08-31 kiran@kryali.com [windows] Fix null-deref in HostWindowPopup::UpdatePosition (Fixes #191478) (flutter/flutter#191479) 2026-08-31 47866232+chunhtai@users.noreply.github.com Removes deprecated ignoreSemantics parementers (flutter/flutter#191493) 2026-08-31 engine-flutter-autoroll@skia.org Roll Dart SDK from b319095e317b to 9164def35347 (1 revision) (flutter/flutter#192073) 2026-08-31 112751483+shivanshu877@users.noreply.github.com test: dynamic _tapOutside helper for TapRegion navigation tests (flutter/flutter#185397) 2026-08-31 git@reb0.org [Windows] fix: Remove quotes from compiler warning suppression (flutter/flutter#190873) 2026-08-31 47866232+chunhtai@users.noreply.github.com Migrate to listen package (flutter/flutter#189111) 2026-08-31 engine-flutter-autoroll@skia.org Roll Skia from 15db98a90bbd to 5489a16a5998 (2 revisions) (flutter/flutter#192070) 2026-08-31 110348311+Devasy@users.noreply.github.com Add regression test for plugin compileSdkExtension warning (flutter/flutter#191281) 2026-08-31 34871572+gmackall@users.noreply.github.com Explicitly disable HCPP in platform view benchmarks and integration tests (flutter/flutter#191908) 2026-08-31 matt.boetger@gmail.com Documentation and script for Gradle Distribution cache for CI (flutter/flutter#190323) 2026-08-31 engine-flutter-autoroll@skia.org Roll Skia from 5549c93c9a1c to 15db98a90bbd (2 revisions) (flutter/flutter#192068) 2026-08-31 engine-flutter-autoroll@skia.org Roll Packages from cd4cdd0 to d642322 (7 revisions) (flutter/flutter#192063) 2026-08-31 engine-flutter-autoroll@skia.org Roll Dart SDK from 48f641e8b249 to b319095e317b (1 revision) (flutter/flutter#192061) 2026-08-31 269567208+reidbaker-agent@users.noreply.github.com [tool] Migrate dev/tools from dart_skills_lint to skills_lint package (flutter/flutter#191997) 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 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
Description
The bulk of this PR's original scope (move/refactor TapRegion navigation tests off Material into
test/widgets/tap_region_test.dart) was already merged in #185567. After merging upstream/master into this branch, the only remaining change is a small robustness improvement to the navigation test helper:Future<void> tapOutside(WidgetTester tester)local closures (each tapping a hardcodedOffset(200, 200)) with a single top-level_tapOutside(tester, regionKey)helper that locates the visible region'sRenderBoxviafind.byKey(regionKey)and computes the outside point asrenderBox.localToGlobal(Offset.zero) + Offset(200, 200).This addresses @justinmc's review feedback that the original material/-based
tapOutsidewas more robust because it found the region dynamically rather than relying on assumptions about the test surface.Pre-launch Checklist