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

cancelling profile creation with "Esc" not fully working #4365

Closed
r10s opened this issue Nov 27, 2024 · 5 comments · Fixed by #4396
Closed

cancelling profile creation with "Esc" not fully working #4365

r10s opened this issue Nov 27, 2024 · 5 comments · Fixed by #4396
Labels
bug Something isn't working

Comments

@r10s
Copy link
Member

r10s commented Nov 27, 2024

when cancelling profile creation with the "Back Arrow", the dialog is closed, the account in creation is removed, and one is back in the last profile - all as expected.

however, using the "Esc" key instead, the account in creation is not removed and one is not back in the last profile. also the dialog is only removed partly, the background images stays:

Screenshot 2024-11-27 at 20 49 17

weirdly, the dialog comes back when the window is resized

@r10s r10s added the bug Something isn't working label Nov 27, 2024
@WofWca
Copy link
Collaborator

WofWca commented Dec 2, 2024

I did some debugging. Looks like on first Esc press it invokes onCancel, which gets prevented, but the second Esc press invokes onCancel and onClose. And onClose does close dialog.

const onClose = (value: any) => {
props.onClose && props.onClose(value)
dialog.current!.style.display = 'none'
}
const onCancel = (ev: React.BaseSyntheticEvent) => {
if (!canEscapeKeyClose) {
ev.preventDefault()
}
}

Answers here https://stackoverflow.com/questions/61021135/prevent-dialog-from-closing-on-keydown-esc-in-chrome indicate that this is a Chromium thing.

So this affects all dialogs that use canEscapeKeyClose={false}.

Let's consider handling onClose in such cases, and closing the dialog, if the users wants it so bad. In this case this would be calling onExitWelcomeScreen.

nicodh added a commit that referenced this issue Dec 8, 2024
resolves #4365

Pressing escape key twice will close open dialogs in Chrome no matter
how onCancel is configured https://issues.chromium.org/issues/346597066

To handle that we move the onClickBackButton function one component
higher to add it as onClose prop to the Dialog and handle the event
the same way a BackButton Click would be handled
@WofWca
Copy link
Collaborator

WofWca commented Dec 8, 2024

I discovered that on the second cancel event ev.cancelable === false, unlike on the first one.

Another hack I can suggest is to dialogRef.current.showModal() immediately after such an event.

@WofWca
Copy link
Collaborator

WofWca commented Dec 8, 2024

Oh, and another thing: maybe we shouldn't utilize showModal for such un-closable dialogs at all? Looks like we only want showModal for the backdrop:

// calling showModal is "only" the way to have ::backdrop
dialog.current?.showModal()
dialog.current!.style.display = 'flex'

But I guess we still want other contents to be non-interactive for such dialogs, so IDK...

nicodh added a commit that referenced this issue Dec 8, 2024
resolves #4365

Pressing escape key twice will close open dialogs in Chrome no matter
how onCancel is configured https://issues.chromium.org/issues/346597066

To handle that we move the onClickBackButton function one component
higher to add it as onClose prop to the Dialog and handle the event
the same way a BackButton Click would be handled
@WofWca
Copy link
Collaborator

WofWca commented Dec 8, 2024

Let's reopen to track other such dialogs as suggested in #4396 (review)

@nicodh
Copy link
Contributor

nicodh commented Dec 8, 2024

Closing since the bug is fixed. Followup is here: #4397

@nicodh nicodh closed this as completed Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants