Skip to content

Conversation

@TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Feb 10, 2022

Fixes #90223

This PR adds dismiss callback to onTapCancel modal barrier.

Native

22-02-10-13-09-39.mp4

Flutter (with this PR)

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 the framework flutter/packages/flutter repository. See also f: labels. label Feb 10, 2022
@TahaTesser TahaTesser changed the title Dismiss Modal Barrier on TapCancel Dismiss Modal Barrier on onTapCancel Feb 10, 2022
@TahaTesser TahaTesser changed the title Dismiss Modal Barrier on onTapCancel Dismiss Modal Barrier on handleTapCancel Feb 10, 2022
@HansMuller HansMuller requested a review from chunhtai February 11, 2022 23:02
if (onAnyTapUp == null)
if (onAnyTapUp == null) {
return false;
} else if (onAnyTapCancel == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The current logic return false if one of onAnyTapUp and onAnyTapCancel is null, maybe they should be &&?

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense, thanks!

@TahaTesser TahaTesser requested a review from chunhtai February 16, 2022 14:01
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@chunhtai
Copy link
Contributor

chunhtai commented Mar 25, 2022

This change causes a regression.
repro:
https://gist.github.com/chunhtai/db68a074f63f96fe2b997597599ebbbb

  1. tap show dialog
  2. tap and hold one finger on modal barrier without lifting it.
  3. use other finger to tap ok

expect: alert dialog is dismissed and page stays at /abc
actual: alert dialog is dismissed, page /abc is popped

Preparing for revert.

chunhtai added a commit to chunhtai/flutter that referenced this pull request Mar 25, 2022
chunhtai added a commit that referenced this pull request Mar 25, 2022
@nosmirck
Copy link

nosmirck commented Apr 1, 2022

Since the original issue #90233 is locked, I'm adding a few things.

If I'm not mistaken, the last time I saw the code for the popup it's using a ModalBarrier which is used on other types of popups (like dialogues). I believe it should have its own barrier, one that closes as soon as an interaction outside the popu menu is done.

Take for example the Gmail App on Android.

Here is an example of the drawer menu:
Screenshot_20220331-234139_Gmail.jpg

If you hold the barrier or drag up/down it stays open. It closes by a right to left gesture (even initiated from the barrier) or a single tap on the barrier.

Screenshot_20220331-234154_Gmail.jpg

This pop up to switch accounts. Stats open with hold or drags on the barrier but a single tap closes it.

Screenshot_20220331-234247_Gmail.jpg

Finally this pop-up menu that opens when tapping the meatballs menu on the top right of the email. Any gesture outside of it closes it, as soon as the screen is touched (tap down) it closes immediately. There's no way to keep it open when interacting with anything outside of the menu.

Now, the main reason I opened this issue is because I have had users complain that the app is frozen on a screen and sent me screenshots of a screen very similar to the last screenshot, where they open a pop-up menu that allows them to have multiple selections while open and then they try to scroll the screen behind it, the popup doesn't close and the screen behind doesn't scroll, which gives them the feeling that the app is frozen. Also, it's unbelievable the amount of people that think that to close a popup menu like the one in the third screenshot they have to "swipe up" instead of just a tap.

I have had reports of people trying to get back to the screen for several minutes before filing the support ticket and even sent recorded videos of them picking options from the menu (which doesn't close since it's just picking on/off options) and then swiping outside the menu several times, fast and slow, in every direction but never tapping the screen with a single tap, believing somehow that the app is frozen and only the menu is interactive until they have to force close the app and reopen.

My issue is temporarily resolved because I implemented a custom popup menu that closes as soon as a gesture is detected on the custom barrier, but it's a mess of a code, only works with my data.

Then I tried the same thing on web and it's even worse.

You'd expect that when you have a popup menu open and scroll with the mousewheel, that at least the content behind scrolls freely, but no. (Like it does here in GitHub if you click the meatballs menu on the top right of this comment and then scroll up/down). On some pages, this behaves differently. Some retain the menu in place relative to the initial position where it opened, others keep it attached to the button that originated the menu and others close it immediately. O have a couple of examples, like using Google docs and opening one of the context menu options (file, edit) then scrolling the page below, or clicking the options menu in chrome and scrolling the page while open (it closes).

I believe that the Material Design guidelines states that a popup menu should close with any gesture, but dialogs can remain open unless dismissable with a tap on the barrier, but I'm not sure about this, it's just the behaviour I've see on apps like Google Play Store and Gmail, while other apps have alight modifications (like the steam app, where some popup menus on the screen remain open and attached to the button that originated it while allowing for scrolling of the whole content).

In any case, the current implementation doesn't allow anything but the same behavior than dialogs, which is dismiss with a tap where it should, at least, provide a few options (like close any gesture or passthrough, maybe with options to remain open relative to screen or attached to the button that opened it).

@chunhtai
Copy link
Contributor

chunhtai commented Apr 1, 2022

I didn't mean the current behavior is correct. The reason the change is reverted is that it introduced a different issue. We should find a find a fix that does not cause other regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[PopUpMenuButton] When the menu is open, scroll gesture doesn't dismiss the modal barrier

4 participants