Skip to content
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

Remove click-outside handling for Dialog #2925

Closed
grebaldi opened this issue Jul 18, 2021 · 1 comment · Fixed by #3492
Closed

Remove click-outside handling for Dialog #2925

grebaldi opened this issue Jul 18, 2021 · 1 comment · Fixed by #3492
Assignees
Labels

Comments

@grebaldi
Copy link
Contributor

Motivation

I've come across multiple scenarios (namely: https://github.com/sitegeist/Sitegeist.Archaeopteryx and https://github.com/Flowpack/media-ui) in which UI extensions need to stack dialogs on top of each other.

This interferes with our current click-outside handling for Dialogs, which makes the assumption that such stacking never happens.

The effect of that can be seen here:

Peek.2021-07-18.16-30.mp4

You see that the new Media UI is embedded in the Node Creation Dialog (Dialog 1). Then we click on "Upload", which opens the Upload Modal of the new Media UI (Dialog 2). If we click anywhere inside Dialog 2, both dialogs close. When we click ouside Dialog 2, both dialogs close as well.

BothDialogsCloseOnClickOutside.mp4

The reason is, that after opening Dialog 2, the entire screen is now an area that is considered to be outside Dialog 1. So if we click anywhere, Dialog 1 receives its request to close. Since Dialog 1 is removed from VDOM, and Dialog 2 is a child of Dialog 1, Dialog 2 has to leave as well.

Possible Solution

Imho, we don't need such aggressive click-outside handling for our Dialogs. It would be sufficient to just react to clicks on the semi-transparent overlay, since it covers (almost) the entire screen.

I say almost, because the top bar of the ui is not covered:

Screenshot_2021-07-18_16-47-08

This can potentially be worked around, because Dialogs live in an isolated part of the DOM tree, but I don't know if this behvaior is even intentional in the first place 😅. (At least it doesn't seem right to me 🧐)

I'd suggest to implement a solution as a bugfix for 5.3+.

Affected Versions

Neos: >=3.3
UI: >=1.0

@grebaldi grebaldi added Bug Label to mark the change as bugfix UI & UX Technical debt Accessibility labels Jul 18, 2021
grebaldi added a commit to grebaldi/media-ui that referenced this issue Jul 18, 2021
This commit creates a (partial) workaround for the issue described in
neos/neos-ui#2925.

It adds a custom decorator component for the Neos UI Dialog that sets
the attribute `data-ignore_click_outside="true"` on the dialog
container.

This solution is crude and relies on a deprecated ReactDOM-API. But for
the time being, it'll at least make the inner dialogs usable in most
cases.

Once neos/neos-ui#2925 is fixed, this solution
should be removed.
grebaldi added a commit to grebaldi/media-ui that referenced this issue Jul 18, 2021
This commit creates a (partial) workaround for the issue described in
neos/neos-ui#2925.

It adds a custom decorator component for the Neos UI Dialog that sets
the attribute `data-ignore_click_outside="true"` on the dialog
container.

This solution is crude and relies on a deprecated ReactDOM-API. But for
the time being, it'll at least make the inner dialogs usable in most
cases.

Once neos/neos-ui#2925 is fixed, this solution
should be removed.
grebaldi added a commit to grebaldi/media-ui that referenced this issue Jul 18, 2021
This commit creates a (partial) workaround for the issue described in
neos/neos-ui#2925.

It adds a custom decorator component for the Neos UI Dialog that sets
the attribute `data-ignore_click_outside="true"` on the dialog
container.

This solution is crude and relies on a deprecated ReactDOM-API. But for
the time being, it'll at least make the inner dialogs usable in most
cases.

Once neos/neos-ui#2925 is fixed, this solution
should be removed.
@Sebobo
Copy link
Member

Sebobo commented Jul 19, 2021

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants