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: change which element keydown is bound to from document to injection div #8188

Merged
merged 4 commits into from
May 31, 2024

Conversation

mark-friedman
Copy link
Contributor

The basics

The details

Resolves

Fixes #8187

Proposed Changes

Bind the keydown event to the workspace's injection div rather than the HTML document

Reason for Changes

See #8187 for details.

Test Coverage

I modified the unit tests which deal with keydown events. I also tested it manually in Chrome, Firefox and Safari on a Mac.

Documentation

The docstring for bindDocumentEvents was changed to reflect the change in the code. I don't believe that that docstring is exposed in the reference documentation.

Additional Information

In theory, this might be considered a breaking change, in that a developer might be depending on the current behavior, even though it is undocumented and possibly confusing to their users. It is also possible that a developer might have some code which, for some reason, is expecting the event to be bound to the document.

@mark-friedman mark-friedman requested a review from a team as a code owner May 30, 2024 20:19
@github-actions github-actions bot added the PR: fix Fixes a bug label May 30, 2024
Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

This all seems pretty reasonable to me, but I'm soliciting additional views from the team as I'm not the most familiar with Blockly's keyboard event handling or indeed the DOM itself.

@cpcallen
Copy link
Contributor

Additional discussion questions for the team:

  • On the topic of whether this is a breaking change: is it intended (i.e. a feature) that shortcuts work even if the focus is elsewhere in the document? In particular…
  • Does it present any usability issues if keyboard navigation depends on the user being able to focus on the div first?

@BeksOmega
Copy link
Collaborator

Does it present any usability issues if keyboard navigation depends on the user being able to focus on the div first?

I think we probably want this behavior. In my mind, keyboard nav should only apply when the user is focused on the div, so that the other parts of the site can also be navigable.

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

The consensus of the team is that this is a good change, and that it is indeed a fix and not a breaking change (even though it's possible that some developers could have been depending on the previous, undesirable behaviour).

@cpcallen cpcallen merged commit 5881ce3 into google:develop May 31, 2024
10 checks passed
@mark-friedman mark-friedman deleted the change-keydown-binding branch May 31, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keydown events should be bound to a workspace rather than a document
3 participants