Skip to content

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

Merged
merged 2 commits into from
Jun 13, 2022
Merged

More flexible modality. #808

merged 2 commits into from
Jun 13, 2022

Conversation

rjrjr
Copy link
Contributor

@rjrjr rjrjr commented Jun 11, 2022

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 the first commit for ease of review.

@rjrjr rjrjr force-pushed the ray/hello-modal-my-old-friend branch 2 times, most recently from 19dc66c to 7044813 Compare June 13, 2022 14:44
@rjrjr rjrjr force-pushed the ray/hello-modal-my-old-friend branch from 7044813 to 0b476a3 Compare June 13, 2022 15:01
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.
@rjrjr rjrjr force-pushed the ray/hello-modal-my-old-friend branch from 0b476a3 to ab76403 Compare June 13, 2022 16:18
@rjrjr rjrjr marked this pull request as ready for review June 13, 2022 16:58
@rjrjr rjrjr requested review from a team and zach-klippenstein as code owners June 13, 2022 16:58
Comment on lines 29 to 33
/**
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty doc?

Copy link
Contributor Author

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.
   */

Comment on lines +16 to +18
internal class OverlayArea(
val bounds: StateFlow<Rect>
) {
Copy link
Contributor

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?

Copy link
Contributor Author

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 BodyAndOverlayContainers.

Copy link
Contributor Author

@rjrjr rjrjr Jun 13, 2022

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.

@rjrjr rjrjr force-pushed the ray/hello-modal-my-old-friend branch from ab76403 to c5446a9 Compare June 13, 2022 20:27
@rjrjr rjrjr requested a review from steve-the-edwards June 13, 2022 20:46
@rjrjr rjrjr merged commit 73edd7e into main Jun 13, 2022
@rjrjr rjrjr deleted the ray/hello-modal-my-old-friend branch June 13, 2022 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants