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

feat: support keyboard navigation of flyout buttons #2200

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

mikeharv
Copy link
Contributor

@mikeharv mikeharv commented Feb 13, 2024

The basics

The details

Requires:

(Tests will continue to fail as this requires a new flyout.getContents() method, defined in the above PR.)

Resolves

Makes it possible to support keyboard navigation of flyout buttons, which is listed as a limitation in the documentation:
https://developers.google.com/blockly/guides/configure/web/keyboard-nav

Fixes

Proposed Changes

  • modify navigation.js to find top button or block based on contents
    -modify navigation_controller to trigger button callbacks when pressing "mark" (enter key)

Requires button-type AST Nodes and flyout.getContents(), which are proposed here: google/blockly#7852

Reason for Changes

These changes will make it possible for users to interact with flyout buttons using keyboard navigation. They also ensure that the top flyout item is selected when focusing a flyout, whether that be a block (stack) or a button. Code.org specifically needs this functionality for variables as well its modal editor for functions and behaviors.

The changes described above are working locally. The following screen recording demonstrates some simple flyout navigation including using various buttons. Note that the keyboard monitoring in the middle of the workspace is a separate application and just used here for illustration purposes.
https://github.com/google/blockly/assets/43474485/899bf8e3-5a76-4c5c-8e3a-3645e42de323

Test Coverage

I have updated the plugin test toolbox to include more buttons as well as nested categories in order to easily test some edge cases. No new tests have been created.

Documentation

The following sections would need to be updated:

Inserting A Block From The Toolbox
Limitations

Additional Information

@mikeharv mikeharv marked this pull request as ready for review February 14, 2024 16:17
@mikeharv mikeharv requested a review from a team as a code owner February 14, 2024 16:17
@mikeharv mikeharv requested review from NeilFraser and removed request for a team February 14, 2024 16:17
@maribethb
Copy link
Contributor

@NeilFraser Will you be able to review this PR this week?

The tests fail because this PR depends on changes in core. I think this PR needs to be against the v11 branch, and then when the core PR is merged and eventually lands in a beta release, they should {theoretically} pass. Once this PR is reviewed I can facilitate that process.

Copy link
Contributor

@NeilFraser NeilFraser left a comment

Choose a reason for hiding this comment

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

Looks good. The linting errors are ignorable in my opinion.

@rachel-fenichel
Copy link
Collaborator

The lint errors are not ignorable since they cause lint to fail and are easily fixed.

@maribethb maribethb self-requested a review April 5, 2024 17:47
@maribethb
Copy link
Contributor

OK, the latest beta of Blockly has been published which contains your changes from core! So next steps:

  1. Rebase these commits onto the rc/11.0.0 branch
  2. Update this PR to be merged against that branch
  3. In the keyboard-nav plugin's package.json file, update both the devDependency and peerDependency on blockly to be ^11.0.0-beta.7
  4. Run npm install from the root of blockly-samples
  5. Fix the lint errors/warnings you get when you run npm run lint
  6. Push your changes and make sure CI passes
  7. I'll merge this PR!

Later, after we release v11 (the next release of Blockly, coming... soon) then our team will take everything in the rc/v11.0.0 branch of samples and update them to rely on the real version not the beta, test the plugins, and release them all.

Thanks for your patience & willingness to contribute despite the complicated process with two repos! We're making some changes soon that should streamline a lot of this extra process.

@mikeharv mikeharv changed the base branch from master to rc/v11.0.0 April 8, 2024 17:32
@maribethb maribethb merged commit c2abe4d into google:rc/v11.0.0 Apr 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants