Support not-blocking pointer inputs outside of focusable Popups#2992
Support not-blocking pointer inputs outside of focusable Popups#2992MatkovIvan wants to merge 1 commit intojb-mainfrom
Conversation
MatkovIvan
force-pushed
the
ivan.matkov/block-input-outside-popup
branch
from
f3d422e to
679e24d
Compare
| usePlatformDefaultWidth = usePlatformDefaultWidth, | ||
| usePlatformInsets = usePlatformInsets | ||
| usePlatformInsets = usePlatformInsets, | ||
| blockPointerInputOutside = blockPointerInputOutside, |
There was a problem hiding this comment.
| blockPointerInputOutside = blockPointerInputOutside, | |
| consumePointerInputOutside = consumePointerInputOutside, |
When I read "block", I think that it doesn't work at all, even in this case:
PopupProperties(dismissOnClickOutside = true, blockPointerInputOutside = true)
But this:
PopupProperties(dismissOnClickOutside = true, consumePointerInputOutside = true)
tells that is still handled, just isn't passed further.
There was a problem hiding this comment.
Is there a valid scenario where block/consumePointerInputOutside=false but dismissOnClickOutside=true?
Maybe we need an outsidePointerEventsMode parameter, with possible values:
OutsidePointerEventsMode.Block(dismissOnClick: Boolean)OutsidePointerEventsMode.Passthrough
?
There was a problem hiding this comment.
Is there a valid scenario where
block/consumePointerInputOutside=falsebutdismissOnClickOutside=true?
It won't trigger dismissing. Similar with dismissOnBackPress/focusable combination now
dismissOnClickOutside is a parameter from commonMain, we should not change it
There was a problem hiding this comment.
dismissOnClickOutside is a parameter from commonMain, we should not change it
Since we're changing the API anyway, we could add an overload that takes outsidePointerEventsMode instead of dismissOnClickOutside.
There was a problem hiding this comment.
It's an extension of the existing common API, let's keep it aligned and simple. Also, there is nothing wrong with such a combination in terms of forcing to avoid it
There was a problem hiding this comment.
It is okay for me to have 2 parameters, maybe even it is needed all 4 combinations for different cases.
I delegate closing the comment or continuing to discuss to Sasha
There was a problem hiding this comment.
Not a blocker; just a combination of parameters that doesn't have a use-case.
There was a problem hiding this comment.
This combination is not new. it was dismissOnClickOutside=true + focusable=false before.
I've re-checked and this combination works and makes sense - it closes Popup on any interaction with the main content without interrupting this particular interaction
| usePlatformDefaultWidth = usePlatformDefaultWidth, | ||
| usePlatformInsets = usePlatformInsets | ||
| usePlatformInsets = usePlatformInsets, | ||
| blockPointerInputOutside = blockPointerInputOutside, |
There was a problem hiding this comment.
Is there a valid scenario where block/consumePointerInputOutside=false but dismissOnClickOutside=true?
Maybe we need an outsidePointerEventsMode parameter, with possible values:
OutsidePointerEventsMode.Block(dismissOnClick: Boolean)OutsidePointerEventsMode.Passthrough
?
MatkovIvan
force-pushed
the
ivan.matkov/block-input-outside-popup
branch
from
679e24d to
7b043aa
Compare
MatkovIvan
force-pushed
the
ivan.matkov/block-input-outside-popup
branch
from
7b043aa to
7587c0c
Compare
|
|
||
| public final class androidx/compose/ui/window/PopupProperties { | ||
| public static final field $stable I | ||
| public fun <init> ()V |
There was a problem hiding this comment.
It's caused by our "filter experimental" API rules. It's still there, everything is compatible
| * only to the "interested" component "under" the mouse pointer. | ||
| */ | ||
| private fun redispatchUnconsumedMouseEvent(event: MouseEvent) { | ||
| private fun redispatchUnconsumedMouseEvent(event: MouseEvent, target: Component? = null) { |
There was a problem hiding this comment.
target seems unneeded now that you removed redispatching between layers.
| override var consumePointerInputOutside: Boolean | ||
| get() = false | ||
| set(_) {} | ||
|
|
There was a problem hiding this comment.
Maybe better to leave it unimplemented here and let each subclass decide?
| } | ||
|
|
||
| @Test | ||
| fun focusableNonBlockingPopup_withComponentLayerType_passesClicksThrough() { |
There was a problem hiding this comment.
Is this a good place for this test? It doesn't seem to test ComposePanel itself.
|
|
||
| /** | ||
| * Indicates if pointer input events outside of this layer's bounds should be blocked. | ||
| * When set to true, touch events outside of this layer's bounds are not propagated to |
|
Probably worth asking someone from the iOS team to review it too, as there are |
3 participants
CMP-8852 Support not-blocking pointer inputs outside of focusable Popups
Release Notes
Features - Multiple Platforms
blockPointerInputOutsideflag toPopupPropertiesto support not-blocking pointer inputs outside of focusablePopups