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!: add getContents to IFlyout #8064

Merged
merged 1 commit into from
May 8, 2024
Merged

Conversation

maribethb
Copy link
Contributor

@maribethb maribethb commented May 3, 2024

The basics

The details

Resolves

Fixes #8063

Proposed Changes

  • Adds getContents method to the IFlyout interface (breaking change)
  • Removes casts for Flyout and FlyoutItem[]
  • Adjusts the getContents method in flyout_base to return an actual FlyoutItem[] instead of saying it does that, but is actually Array<Block | FlyoutItem> (note that this method exists in develop but hasn't been released yet so this isn't the breaking change)

Reason for Changes

  • In Blockly v10, flyouts never store a list of their contents. Whenever show is called, the flyout definition is passed to the method, that definition is parsed, and the blocks, buttons, and gaps are laid out in the flyout. The actual blocks and buttons created are never stored anywhere.
  • For keyboard navigation to be able to navigate flyouts including buttons, we need to know what is currently in the flyout. Simply inspecting the workspace is not enough, as buttons are not a first class object stored on the workspace like blocks are. You can't get a list of buttons on the workspace. Therefore, we need to store what is currently being shown in the flyout.
  • This was initially implemented in feat: support keyboard navigation of flyout buttons #7852. Initially, the getContents method was only added to the abstract Flyout class, and not the IFlyout interface. However, custom flyouts are only required to implement the IFlyout interface and are not required to extend our base class.
  • Also, in this PR, the getContents method was typed as returning an array of FlyoutItems, but it didn't actually return those, and used casts to fake it, both when setting the contents and using them. The reason I'm fixing this by actually using FlyoutItem instead of just changing the type is because using FlyoutItem is more future-compatible. If we ever extend the flyout definition to include a third type of object that might be keyboard-navigable, we won't need to change the type or implementation of getContents (just the implementation of ASTNode to handle the new item).

Test Coverage

I haven't been able to link this to samples to test keyboard navigation, but I'll try that again before submitting this PR.

Documentation

N/A, we don't document how to create a custom flyout at this time.

Breaking changes / To fix

  • getContents is now required to be implemented in the IFlyout interface. This is important to support our goal of improving accessibility in Blockly.
  • If you do not use a custom flyout, you do not need to take any action.
  • If you use a custom flyout that extends Blockly.Flyout, Blockly.HorizontalFlyout, Blockly.VerticalFlyout, or the ContinuousFlyout from the continuous-toolbox plugin, you may not need to take any action. The getContents method has been implemented in the abstract base class.
    • If your subclass overrides the show method, ensure that it sets the contents property of the flyout as the base class implementation does.
  • If you use a custom flyout that implements the IFlyout interface and does not extend the abstract Flyout class, you will need to add an implementation for getContents to your flyout. This method should return a list of all FlyoutItems (at this time, that includes Blocks and FlyoutButtons) that are present in the current flyout.

@maribethb maribethb requested a review from a team as a code owner May 3, 2024 02:31
@maribethb maribethb requested a review from NeilFraser May 3, 2024 02:31
@github-actions github-actions bot added breaking change Used to mark a PR or issue that changes our public APIs. PR: fix Fixes a bug labels May 3, 2024
@BeksOmega
Copy link
Collaborator

Not directly related to this PR, but from a design + architecture perspective I think this is a great example of why it's best to have /either/ an abstract class /or/ an interface, and not both. Having both makes it really easy to make errors like this where you modify the abstract class without modifying the interface. And it also makes it easy to depend on side effects or other implementation details in the abstract class, without properly documenting them in the interface.

@github-actions github-actions bot added breaking change Used to mark a PR or issue that changes our public APIs. PR: fix Fixes a bug and removed PR: fix Fixes a bug breaking change Used to mark a PR or issue that changes our public APIs. labels May 6, 2024
@maribethb
Copy link
Contributor Author

Verified the fix in google/blockly-samples#2342

@maribethb maribethb merged commit 54eeb85 into google:develop May 8, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Used to mark a PR or issue that changes our public APIs. PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyboard navigation should only use methods available on IFlyout
3 participants