Skip to content
This repository has been archived by the owner on Sep 8, 2023. It is now read-only.

Improve escape keypress handling #897

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

julien-nc
Copy link
Collaborator

In Text, when opening the link picker without initial provider (via the menu action), then selecting a provider, then going back by pressing Escape, the provider multiselect can't be focused anymore.

I had a long fight to make progress but still no luck.

This works fine when used in RichContentEditable (and outside the Viewer btw).

I checked and the event does not go upper than <ReferencePicker>.

I tried using another key to go back and it works fine. So, for some reason, pressing Escape messes with the focus trap.

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc added the bug Something isn't working label Feb 3, 2023
@julien-nc julien-nc requested a review from juliusknorr February 3, 2023 17:50
…ements to its focus trap

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc marked this pull request as ready for review February 6, 2023 23:16
@julien-nc
Copy link
Collaborator Author

It works after emitting the viewer:trapElements:changed event to add the link picker modal content element as a trap element of the Viewer 🎉.

Copy link
Contributor

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

I was not able to reproduce the behavior described as without this fix escape would close the entire viewer for me. Maybe I'm running an outdated server or viewer version.

With this fix the behavior seems correct. Thanks for the sleuthing. 🕵️

@julien-nc julien-nc merged commit d61a069 into master Feb 7, 2023
@delete-merged-branch delete-merged-branch bot deleted the fix/noid/ref-picker-escape-pressed branch February 7, 2023 09:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants