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: Fixes bug where clicking on the canvas did not remove focus from other elements #539

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

ShrimpCryptid
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid commented Feb 4, 2025

Problem

Closes #535, "clicking on viewport should remove focus from other elements."

Estimated review size: tiny, <5 minutes

Solution

  • Clicking or dragging on the canvas now blurs focus from other elements.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@ShrimpCryptid ShrimpCryptid added the bug Something isn't working label Feb 4, 2025
@ShrimpCryptid ShrimpCryptid self-assigned this Feb 4, 2025
Copy link

github-actions bot commented Feb 4, 2025

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2025-02-11 17:54 UTC

Copy link

github-actions bot commented Feb 4, 2025

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 75.02% 6430 / 8570
🔵 Statements 75.02% 6430 / 8570
🔵 Functions 60.07% 170 / 283
🔵 Branches 80.97% 532 / 657
File CoverageNo changed files found.
Generated in workflow #1431

@ShrimpCryptid ShrimpCryptid force-pushed the fix/blur-inputs-on-viewport-click branch from 69d6087 to 5c590dd Compare February 4, 2025 01:21
// Prevent the default behavior for mouse clicks that would cause text
// selection, but keep the behavior where focus is removed from other
// elements.
event.preventDefault();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was what was causing the bug, but event.preventDefault is still mostly desired behavior. Simulate the focus behavior by blurring the active element in the document.

@ShrimpCryptid ShrimpCryptid marked this pull request as ready for review February 4, 2025 01:22
@ShrimpCryptid ShrimpCryptid requested a review from a team as a code owner February 4, 2025 01:22
@ShrimpCryptid ShrimpCryptid requested review from toloudis and ascibisz and removed request for a team February 4, 2025 01:22
@ShrimpCryptid ShrimpCryptid linked an issue Feb 4, 2025 that may be closed by this pull request
Base automatically changed from feature/annotation-select-whole-tracks to main February 4, 2025 17:45
Copy link

@ascibisz ascibisz left a comment

Choose a reason for hiding this comment

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

I don't know much about the world of MouseEvents, but looks good to me! I appreciate the comments in the code :)

// elements.
event.preventDefault();
if (document.activeElement instanceof HTMLElement) {
document.activeElement.blur();
Copy link
Contributor

Choose a reason for hiding this comment

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

will it blur itself too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, good question. I don't think blurring itself would do anything because it's not an element that has focus behavior or styling, but I'll add a check in and skip blurring if it's the same element. Thank you!

@ShrimpCryptid ShrimpCryptid merged commit 0fea06b into main Feb 11, 2025
3 checks passed
@ShrimpCryptid ShrimpCryptid deleted the fix/blur-inputs-on-viewport-click branch February 11, 2025 17:51
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 this pull request may close these issues.

Clicking on the viewport should remove focus from inputs
3 participants