-
Notifications
You must be signed in to change notification settings - Fork 108
More flexible modality. #808
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
Conversation
19dc66c
to
7044813
Compare
...-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/BodyAndModalsContainer.kt
Outdated
Show resolved
Hide resolved
workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/LayeredDialogs.kt
Outdated
Show resolved
Hide resolved
workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/container/ModalOverlay.kt
Show resolved
Hide resolved
7044813
to
0b476a3
Compare
Introduces the `ModalOverlay` interface. Moves as much code as possible from `BodyAndModalsContainer` to `LayeredDialogs`, and allows them to turn on modal event blocking behavior per managed `Overlay` instance, rather than always enforcing it. Very sad that I wasn't able to get rid of `BodyAndOverlaysContainer.onAttachedToWindow`, but the SavedStateRegistry timing is just too fussy to allow it. Git was dumb about diff'ing across renames, so they aren't all done in this commit for ease of review. The rest of the renames follow.
0b476a3
to
ab76403
Compare
/** | ||
*/ |
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.
Empty doc?
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.
Thanks.
/**
* The unique `SavedStateRegistry` key passed to [LayeredDialogs.onAttachedToWindow],
* derived from the first rendering passed to [update]. See the doc on
* [LayeredDialogs.onAttachedToWindow] for details.
*/
internal class OverlayArea( | ||
val bounds: StateFlow<Rect> | ||
) { |
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 this is how we specify whether to block events around overlays now?
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.
Big response here, I'll update the commit message to include it, once you confirm it's intelligible.
This isn't new, I just renamed it from ModalOverlayArea
b/c it's not specific to modal behavior. OverlayArea
is still how we broadcast the bounds of the view under the dialogs, so that the dialogs' size and position can be restricted to that area. It's always been about positioning, not modality. And it's mainly about making sure that the dialog isn't too big, e.g. so that we can build status and tutorial bars that don't get eclipsed by our modal windows.
[ StatusBarContainer ]
[ BodyAndOverlaysContainer ]
[ TutorialBarContainer ]
OverlayArea
here tracks the size and position of BodyAndOverlaysContainer
, so any related dialog windows won't stray over the status or tutorial bars while they're visible. None of that changes with this PR.
The big change here is related to the tricky code we've built to make covered views start ignoring events the instant we decide to show a dialog, because there is a period after the call to Android's Dialog.show()
where events that should be intercepted by the new dialog window aren't. It's fundamentally about when we set allowEvents
fields in DialogHolder
and LayeredDialogs
to true or false.
While allowEvents
is false, the covered view ignores all UI events, regardless of the size of the dialog windows above it. In our illustration above, even if the dialog windows over the body in BodyAndOverlaysContainer
decide to be tiny, the body will ignore all events while any dialog is showing.
Previously, LayeredDialogs.modal
was set at construction time, and BodyAndModalsContainer
set it to true. That meant that we wanted all managed dialog windows to be considered modal, meaning allowEvents
should be false while any of them are visible. So, if you wanted to use smaller dialog windows for something like a floating toast, which should not block events, you're hosed. You have to fork BodyAndModalsContainer
to create LayeredDialogs
with modal
false, and have a separate wrapping container to host your non-modal toasts.
With this PR, LayeredDialogs
looks for the ModalOverlay
marker interface. LayeredDialog.allowEvents
is set to false only if one of the managed Overlay
renderings also implements ModalOverlay
. DialogHolder.allowEvents
is set to false only for dialogs whose z index is lower than a ModalOverlay
sibling.
Also note how we still look for the value of the CoveredByModal
ViewEnvironment
value while deciding if we're in that situation, so that it's still possible to nest BodyAndOverlayContainer
s.
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.
I'm adding this discussion to the kdoc on ScreenOverlayDialogFactory
in the next PR, gonna merge this as is.
ab76403
to
c5446a9
Compare
Introduces the
ModalOverlay
interface. Moves as much code as possible fromBodyAndModalsContainer
toLayeredDialogs
, and allows them to turn on modal event blocking behavior per managedOverlay
instance, rather than always enforcing it.Very sad that I wasn't able to get rid of
BodyAndOverlaysContainer.onAttachedToWindow
, but the SavedStateRegistry timing is just too fussy to allow it.Git was dumb about diff'ing across renames, so they aren't all done in the first commit for ease of review.