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

Tap on button behind snack bar defined by margin #127959

Merged
merged 2 commits into from
Jul 14, 2023
Merged

Tap on button behind snack bar defined by margin #127959

merged 2 commits into from
Jul 14, 2023

Conversation

lsaudon
Copy link
Contributor

@lsaudon lsaudon commented May 31, 2023

If the margin is used, set the HitTestBehavior to deferToChild.

List which issues are fixed by this PR. You must list at least one issue.
#78537
#114810

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • 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.
  • All existing and new tests are passing.

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

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 31, 2023
@github-actions github-actions bot removed the framework flutter/packages/flutter repository. See also f: labels. label May 31, 2023
@kitsuniru
Copy link

why not also let the developer choose the hitTestBehavior parameter himself?

#114810 - like here

@lsaudon
Copy link
Contributor Author

lsaudon commented Jun 22, 2023

why not also let the developer choose the hitTestBehavior parameter himself?

#114810 - like here

Because I don't want to have to know that I have to put in a hitTestBehavior when I change the margin.
I don't find that logical.
Leaving the choice means that I fix the bug only for those who know about it.

I can do this

behavior: widget.hitTestBehavior ?? widget.margin != null
            ? HitTestBehavior.deferToChild
            : HitTestBehavior.opaque,

@bleroux
Copy link
Contributor

bleroux commented Jun 23, 2023

I can do this

behavior: widget.hitTestBehavior ?? widget.margin != null
            ? HitTestBehavior.deferToChild
            : HitTestBehavior.opaque,

I too think it would be great to update the PR this way.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 23, 2023
Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

A few minor comments on the tests code.

@lsaudon
Copy link
Contributor Author

lsaudon commented Jun 23, 2023

I can do this

behavior: widget.hitTestBehavior ?? widget.margin != null
            ? HitTestBehavior.deferToChild
            : HitTestBehavior.opaque,

I too think it would be great to update the PR this way.

I added it.

Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks @lsaudon and @hot-moms

cc @justinmc for a review (because you reviewed #114810)

@bleroux bleroux requested a review from justinmc June 23, 2023 09:48
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Good call adding the hitTestBehavior parameter as well.

Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

@lsaudon Thanks for you patience, just another step left to polish the comments.

packages/flutter/lib/src/material/snack_bar.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/snack_bar.dart Outdated Show resolved Hide resolved
@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 14, 2023
@auto-submit auto-submit bot merged commit c3cd016 into flutter:master Jul 14, 2023
73 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
If the margin is used, set the `HitTestBehavior` to `deferToChild`. 

*List which issues are fixed by this PR. You must list at least one issue.*
flutter#78537 
flutter#114810 

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
@lsaudon lsaudon deleted the snackbar-margin-tap branch September 15, 2023 07:47
auto-submit bot pushed a commit that referenced this pull request May 29, 2024
…rThemeData.insetPadding is not null (#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 #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
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

4 participants