-
Notifications
You must be signed in to change notification settings - Fork 70
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
Close Popup/Dialog by clicking any mouse button outside #1280
Conversation
|
Also, let's add unit tests for all these scenarios. |
As mentioned in the description - TBD. That's why it's in draft |
Ah, right, I see it only responds to mouse pressed/released events.
Not what I meant. The layer should probably close on any button. I'm just wondering what should be done on mouse-wheel events when they happen outside the layer bounds. Should we send them to the layers below? Do we need yet another layer flag here? Maybe something to consider & discuss later.
Either that, or rename the function. |
d023b8c
to
a4158d7
Compare
buttons = buttons, | ||
button = PointerButton.Secondary | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should send a release here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's on purpose - Popup
should be closed on down event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So dialogs close on release, but popups close on press? I see that in the code, but is there a reason for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. TLDR - Android
Fine tune #1280 change. The problem is some components like bottom sheet or navigation drawer define custom scrim and detect click on them via regular detection like `detectTapGestures` (that filters only primary mouse button). So to keep things consistent, we need to revert primary button filtering for `Dialog`, but keep it as-is for `Popup` to avoid breaking dropdowns, ## Testing Unit test: `DialogTest` Try to close `Popup` and `Dialog` by outside click via mouse buttons and touch. This should be tested by QA ## Release Notes ### Fixes - Desktop - _(prerelease fix)_ Fix inconsistency in closing `Dialog` by mouse clicking on scrim that was introduced by `1.6.10-beta02`
Fine tune #1280 change. The problem is some components like bottom sheet or navigation drawer define custom scrim and detect click on them via regular detection like `detectTapGestures` (that filters only primary mouse button). So to keep things consistent, we need to revert primary button filtering for `Dialog`, but keep it as-is for `Popup` to avoid breaking dropdowns, ## Testing Unit test: `DialogTest` Try to close `Popup` and `Dialog` by outside click via mouse buttons and touch. This should be tested by QA ## Release Notes ### Fixes - Desktop - _(prerelease fix)_ Fix inconsistency in closing `Dialog` by mouse clicking on scrim that was introduced by `1.6.10-beta02`
Proposed Changes
Testing
Test:
PopupTest
/DialogTest
Issues Fixed
Fixes JetBrains/compose-multiplatform#4549