Sitelet https://github.com/flutter/flutter/pull/185397
Skip to content

test: dynamic _tapOutside helper for TapRegion navigation tests - #185397

Merged
auto-submit[bot] merged 12 commits into
flutter:masterfrom
shivanshu877:fix/177414-misplaced-tests
Aug 31, 2026
Merged

auto-submit[bot] merged 12 commits into
flutter:masterfrom
shivanshu877:fix/177414-misplaced-tests

Conversation

@shivanshu877

@shivanshu877 shivanshu877 commented Apr 22, 2026 •

Copy link
Copy Markdown
Contributor

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:

  • Replace the two duplicated Future<void> tapOutside(WidgetTester tester) local closures (each tapping a hardcoded Offset(200, 200)) with a single top-level _tapOutside(tester, regionKey) helper that locates the visible region's RenderBox via find.byKey(regionKey) and computes the outside point as renderBox.localToGlobal(Offset.zero) + Offset(200, 200).

This addresses @justinmc's review feedback that the original material/-based tapOutside was more robust because it found the region dynamically rather than relying on assumptions about the test surface.

Pre-launch Checklist

  • I read the [Contributor Guide].
  • I read the [Tree Hygiene] page.
  • I read and followed the [Flutter Style Guide].
  • I signed the [CLA].
  • All existing and new tests pass.

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
@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages labels Apr 22, 2026
@google-cla

google-cla Bot commented Apr 22, 2026

Copy link
Copy Markdown

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 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.

Comment thread packages/flutter/test/material/tap_region_test.dart Outdated
@shivanshu877

Copy link
Copy Markdown
Contributor Author

Thank you for the review, @gemini-code-assist!

All three suggestions have been addressed:

  1. Deduplicated tapOutside: Extracted into a single top-level helper function before main(), removing the duplication across the two test bodies.
  2. Removed unused regionFinder parameter: The helper now takes only WidgetTester tester.
  3. Fixed inaccurate comment: Updated from "Find the RenderBox of the region" to "Find the RenderBox of the top-most Scaffold and tap outside any region."

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Thanks for the update, @shivanshu877. The refactoring of tapOutside into a helper function and the cleanup of the unused regionFinder parameter are great improvements. The updated comment also accurately reflects the test's intent. Everything looks good to go.

@justinmc justinmc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these tests should probably stay in widgets and be refactored to not use material.

Comment thread packages/flutter/test/material/tap_region_test.dart Outdated
@justinmc justinmc moved this from Todo to In Progress in Test cross-imports Review Queue Apr 29, 2026
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.
@github-actions github-actions Bot removed the p: material_ui material_ui package in flutter/packages label Apr 30, 2026
@victorsanni victorsanni added CICD Run CI/CD override code freeze Override an active code freeze. labels Apr 30, 2026
Comment thread packages/flutter/test/widgets/tap_region_test.dart Outdated
Comment thread packages/flutter/test/widgets/tap_region_test.dart Outdated

@victorsanni victorsanni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@github-actions github-actions Bot removed the CICD Run CI/CD label May 1, 2026
@shivanshu877 shivanshu877 changed the title test: move Material-dependent tap region tests to material/ directory test: refactor TapRegion navigation regression tests off Material May 1, 2026
@shivanshu877

Copy link
Copy Markdown
Contributor Author

@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!

Comment thread packages/flutter/test/widgets/tap_region_test.dart
Comment thread packages/flutter/test/widgets/tap_region_test.dart Outdated
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.
@shivanshu877 shivanshu877 changed the title test: refactor TapRegion navigation regression tests off Material test: dynamic _tapOutside helper for TapRegion navigation tests May 4, 2026
@justinmc
justinmc requested review from justinmc and victorsanni May 5, 2026 22:43
Comment thread packages/flutter/test/widgets/tap_region_test.dart Outdated
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.
@Piinks Piinks removed the override code freeze Override an active code freeze. label Jun 2, 2026
@justinmc
justinmc requested a review from victorsanni June 30, 2026 22:13
victorsanni
victorsanni previously approved these changes Jul 21, 2026
Comment thread packages/flutter/test/widgets/tap_region_test.dart Outdated
@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.
Comment on lines +29 to +33
// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is necessary, the assert message makes it obvious.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

Comment thread packages/flutter/test/widgets/tap_region_test.dart Outdated
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.
@victorsanni
victorsanni self-requested a review August 31, 2026 17:48
@victorsanni victorsanni added the CICD Run CI/CD label Aug 31, 2026
@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Aug 31, 2026
@victorsanni victorsanni added the CICD Run CI/CD label Aug 31, 2026

@justinmc justinmc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 . Thanks for following up with this!

@justinmc justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Aug 31, 2026
Merged via the queue into flutter:master with commit 7350e1b Aug 31, 2026
27 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Test cross-imports Review Queue Aug 31, 2026
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2026
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Sep 1, 2026
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
victorsanni pushed a commit to victorsanni/packages that referenced this pull request Sep 9, 2026
…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
victorsanni pushed a commit to victorsanni/packages that referenced this pull request Sep 9, 2026
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD framework flutter/packages/flutter repository. See also f: labels.

Projects

Development

Successfully merging this pull request may close these issues.

5 participants