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

fix(web): Prevent DELETE key from clearing DateInput in modal #8846

Merged
merged 6 commits into from
Apr 29, 2024

Conversation

uniqueeest
Copy link
Contributor

This commit modifies the keydown event handler to stop the propagation of non-navigation key events in the change date modal.
So the DELETE key prevented the error of erasing the selected pictures.

@uniqueeest uniqueeest changed the title fix(web): Prevent DELETE key from clearing DateInput in modal (#8804) fix(web): Prevent DELETE key from clearing DateInput in modal Apr 16, 2024
@bo0tzz bo0tzz linked an issue Apr 16, 2024 that may be closed by this pull request
3 tasks
@ben-basten
Copy link
Member

This might be out of scope for the current PR, but I'm wondering if this event propagation fix should be pulled into the FullScreenModal level. Keyboard shortcuts affecting the content behind a modal is an issue in multiple places, so that would also fix issues like #8805.

@danieldietzler
Copy link
Member

This might be out of scope for the current PR, but I'm wondering if this event propagation fix should be pulled into the FullScreenModal level. Keyboard shortcuts affecting the content behind a modal is an issue in multiple places, so that would also fix issues like #8805.

Yeah I think you're right. Before we solve the same issue in a dozen places we should fix the actual issue at the root.

@uniqueeest
Copy link
Contributor Author

@danieldietzler I fixed the keyboard issue that occurs in modals by adding stop propagation to the full-screen-modal component. What do you think?

@danieldietzler
Copy link
Member

You don't want to essentially disable all events. Something like escape should still work there. #8846 (comment) applies here as well.

@@ -45,9 +45,13 @@

<FocusTrap>
Copy link
Member

@ben-basten ben-basten Apr 22, 2024

Choose a reason for hiding this comment

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

Looks like the FocusTrap will have to be moved, to make sure that it's still receiving the keyboard events needed to manage focus within the modal. Making it a direct child of the section could be a fix for this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ben-basten Thank you, and I'll give it a try!

Copy link
Contributor

Choose a reason for hiding this comment

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

I just changed this to just ignore events that originate from an input type that includes date and that seemed to fix the issue without needing to make any other changes.

@jrasm91
Copy link
Contributor

jrasm91 commented Apr 29, 2024

This fixes the issue for keyboard shortcuts while focus is in the input. It doesn't address the issue for shortcuts triggering events while the modal is open and the focus is not in an input field. I think that is fine for now and can be left for a follow-up PR.

@jrasm91 jrasm91 merged commit 461f259 into immich-app:main Apr 29, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Web : Change date modale / DEL key trash the selected pictures
5 participants