Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change snack bar default hitTestBehavior to deferToChild when SnackBarThemeData.insetPadding is not null #148568

Merged
merged 8 commits into from
May 29, 2024

Conversation

hany-achraf
Copy link
Contributor

The PR changes the default value of hitTestBehavior in snack bars to HitTestBehavior.deferToChild when snackBarTheme.insetPadding is not null, so that widgets behind snack bars affected by the value set to insetPadding, remain interactive even while a snack bar is visible. This PR can be considered as an extension to what have been done in PR #127959 which fixes the same problem but for individual snack bars with margin not being null. This PR works on the theme level.

List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
#148566

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels May 17, 2024
@goderbauer goderbauer requested a review from Piinks May 17, 2024 21:04
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Hi @hany-achraf welcome!

@@ -387,7 +387,7 @@ class SnackBar extends StatefulWidget {

/// Defines how the snack bar area, including margin, will behave during hit testing.
///
/// If this property is null and [margin] is not null, then [HitTestBehavior.deferToChild] is used by default.
/// If this property is null, and [margin] is not null or [snackBarTheme.insetPadding] is not null, then [HitTestBehavior.deferToChild] is used by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is what is causing the doc reference failure. Can you take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Piinks! Thanks for flagging that issue. I've taken a look and applied the fix. All checks have now passed successfully. Please go ahead and verify that everything is working as expected on your end.

expect(completer.isCompleted, true);
});

testWidgets('Cannot interact with widgets behind SnackBar if hitTestBehavior is set to HitTestBehavior.opaque even if insetPadding is set in SnackBarThemeData', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

This passes without this change, is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behavior you're observing is expected. This PR modifies the default value of hitTestBehavior to HitTestBehavior.deferToChild instead of HitTestBehavior.opaque when SnackBarThemeData.insetPadding is not null. The purpose of this test is to ensure that even with SnackBarThemeData.insetPadding set, the default value of hitTestBehavior can still be overridden, which is consistent with the existing behavior before this PR.
The test passes both with and without this change because the intended functionality remains intact. I added this test after noticing a similar one in #127959 named "Can't tap on button behind snack bar defined by margin and HitTestBehavior.opaque" in the same snack_bar_test.dart file. However, I'm unsure whether keeping this new test is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Piinks! I've just removed that test in my latest commit on this PR. Please review and let me know if you'd like me to revert that change.

@hany-achraf hany-achraf changed the title Change snack bar default hitTestBehavior to deferToChild when snackBarTheme.insetPadding is not null Change snack bar default hitTestBehavior to deferToChild when SnackBarThemeData.insetPadding is not null May 25, 2024
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Thanks for the update! This LGTM!
I will need to find a second reviewer, please hold. :)

Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for putting this together!

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label May 28, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 29, 2024
Copy link
Contributor

auto-submit bot commented May 29, 2024

auto label is removed for flutter/flutter/148568, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label May 29, 2024
@auto-submit auto-submit bot merged commit 9ea9726 into flutter:master May 29, 2024
72 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 29, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 29, 2024
flutter/flutter@a1a33e6...c85fa6a

2024-05-29 polinach@google.com Clean leak in editable_text_test.dart. (flutter/flutter#149223)
2024-05-29 sokolovskyi.konstantin@gmail.com Add tests for animated_switcher.0.dart API example. (flutter/flutter#149180)
2024-05-29 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from d0323905fc2f to b26e1b023cdb (16 revisions) (flutter/flutter#149220)
2024-05-29 hany212mohamed@gmail.com Change snack bar default hitTestBehavior to deferToChild when SnackBarThemeData.insetPadding is not null (flutter/flutter#148568)
2024-05-29 98614782+auto-submit[bot]@users.noreply.github.com Reverts "sliverGridDelegate mainAxisExtent add assert (#148470)" (flutter/flutter#149224)
2024-05-29 yinxulolol@gmail.com sliverGridDelegate mainAxisExtent add assert (flutter/flutter#148470)
2024-05-29 luis901101@gmail.com Fix `SearchAnchor` suggestions not refreshing after long API call (flutter/flutter#148767)
2024-05-28 737941+loic-sharma@users.noreply.github.com Add link to golden file test docs in the framework gardener guide (flutter/flutter#149207)
2024-05-28 polinach@google.com Remove opt out for CurvedAnimation. (flutter/flutter#147594)
2024-05-28 31859944+LongCatIsLooong@users.noreply.github.com Fix the RenderFlex.computeDryBaseline implementation to match computeDistanceToActualBaseline (flutter/flutter#149062)
2024-05-28 polinach@google.com Clean leaky test. (flutter/flutter#149199)
2024-05-28 34871572+gmackall@users.noreply.github.com Change `android_plugin_new_output_dir_test.dart` test description (flutter/flutter#149198)
2024-05-28 leroux_bruno@yahoo.fr fix M2 InputDecorator suffix icon doesn't turn red on error (flutter/flutter#149161)
2024-05-28 77919688+varunkamanibosc@users.noreply.github.com Add selectionOverlayBuilder in CupertinoDatePicker and CupertinoTimer� (flutter/flutter#143079)
2024-05-28 jmccandless@google.com Mouse onEnter and onExit now support hovering stylus (flutter/flutter#149006)
2024-05-28 31859944+LongCatIsLooong@users.noreply.github.com Remove `TextEditingController` private member access (flutter/flutter#149042)
2024-05-28 engine-flutter-autoroll@skia.org Roll Packages from b7bcb4b to a933c30 (1 revision) (flutter/flutter#149184)
2024-05-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from b1751088c7e9 to d0323905fc2f (2 revisions) (flutter/flutter#149169)
2024-05-28 kevmoo@users.noreply.github.com [tool] Use kebabCase directly (flutter/flutter#149150)
2024-05-28 katelovett@google.com [wiki migration] Leftover wiki pages and links (flutter/flutter#148989)

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 camillesimon@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
@hany-achraf
Copy link
Contributor Author

Thank you @Piinks and @MitchellGoodwin so much!

victorsanni pushed a commit to victorsanni/flutter that referenced this pull request May 31, 2024
…rThemeData.insetPadding is not null (flutter#148568)

The PR changes the default value of hitTestBehavior in snack bars to `HitTestBehavior.deferToChild` when snackBarTheme.insetPadding is not null, so that widgets behind snack bars affected by the value set to insetPadding, remain interactive even while a snack bar is visible. This PR can be considered as an extension to what have been done in PR flutter#127959 which fixes the same problem but for individual snack bars with margin not being null. This PR works on the theme level.

*List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.*
flutter#148566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants