Skip to content

Blockly 12 + keyboard nav plugin update#10596

Merged
riknoll merged 20 commits intomicrosoft:masterfrom
microbit-matt-hillsdon:blockly-12
May 29, 2025
Merged

Blockly 12 + keyboard nav plugin update#10596
riknoll merged 20 commits intomicrosoft:masterfrom
microbit-matt-hillsdon:blockly-12

Conversation

@microbit-matt-hillsdon
Copy link
Contributor

@microbit-matt-hillsdon microbit-matt-hillsdon commented May 21, 2025

Blockly 12 and the plugin will definitely need a further upgrade to raise quality (there are useful fixes in core that aren't yet released, e.g. keyboard nav issues after delete, and the plugin is expected to get a few fixes too).

Demo: https://blockly-12-keyboard-exp.review-pxt.pages.dev/#editor

For comparison to see if identified issues are fixed in develop branch of Blockly / latest plugin you can check this deployment.

We'll follow up with a PR to use this plugin version to reinstate cross-tab copy/paste when accessible blocks is on.


override addConnectionHighlight(connection: Blockly.RenderedConnection, connectionPath: string, offset: Blockly.utils.Coordinate, rtl: boolean): void {
super.addConnectionHighlight(connection, connectionPath, offset, rtl);
override addConnectionHighlight(connection: Blockly.RenderedConnection, connectionPath: string, offset: Blockly.utils.Coordinate, rtl: boolean): SVGElement {
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 is a bit unfortunate. Matt/Rob to discuss and see if there's something we can feed back to Blockly here.

Copy link
Contributor Author

@microbit-matt-hillsdon microbit-matt-hillsdon May 21, 2025

Choose a reason for hiding this comment

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

We looked at this and don't have a straightforward suggestion here.

Blockly added a return type and moved to proactively calling addConnectionHighlight, controlling visibility with CSS display and only removing the highlight in dispose.

By default this meant our override always showed the custom part.

So we changed our override to return a group so that both parts are controlled by the same CSS display change Blockly makes. But that requires a bit of copy/paste and access to the connectionHighlights map in super.

Suggestions for how this could be improved very welcome.

Copy link
Contributor Author

@microbit-matt-hillsdon microbit-matt-hillsdon May 29, 2025

Choose a reason for hiding this comment

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

853f4ea removes the duplication and use of internals.

We were pushed to revisit this by a future plugin version adding keyboard nav to empty value connections which unintentionally showed the static connection indicator in MakeCode. But this change applies here and addresses this ugly workaround.

div.blocklyTreeRoot > div > div[role="tree"]:focus-visible {
outline: none;

html {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CSS as per plugin test page. Probably not its ultimate home but a copy is needed here for now.

@microbit-matt-hillsdon microbit-matt-hillsdon marked this pull request as ready for review May 21, 2025 15:37
@microbit-matt-hillsdon
Copy link
Contributor Author

microbit-matt-hillsdon commented May 22, 2025

Clicking on the input fields for certain blocks cause them to be in drag mode.

Repro steps

  1. Open https://blockly-12.review-pxt.pages.dev/#editor.
  2. Create new project and drag in one of the following blocks (non-exhaustive of all that have issues):
Image Image Image Image Image Image Image Image Image Image
  1. Click on the text field to try and edit.
  2. See the block go into drag mode. Repeated clicking does not stop the block from being dragged.

This is a serious impact even when keyboard nav isn't enabled. Fix likely needs RaspberryPiFoundation/blockly#9078 - we suspect workarounds are too messy.

@microbit-matt-hillsdon
Copy link
Contributor Author

Clicking on the input fields for certain blocks cause them to be in drag mode.

This is fixed in 2367994 by updating to 12.0.1-beta.1 and using the new API for the editors that pop something in the dropdown div but focus starts in the widget div.

I'd like to give this a bit more review before merge.

@microbit-matt-hillsdon
Copy link
Contributor Author

microbit-matt-hillsdon commented May 23, 2025

@microbit-robert spotted this logged error clicking on comments, likely due to the upgrade. Looking into it:

main.js:366 Uncaught TypeError: r.getBubble is not a function
    at Vd.lookUpFocusableNode (main.js:366:211539)
    at Js.findFocusableNodeFor (main.js:361:173463)
    at t (main.js:361:174171)
    at main.js:361:174523

Cause: MakeCode's comment icon has the pre-v12 methods of IHasBubble but doesn't implement the interface in a TypeScript-sense or the new-in-v12 method getBubble. So we didn't notice the need to add the new method and the typeguard now misleads calling code into thinking getBubble will always be present.

I guess we might have got a different bug if the type guard had been updated though.

Fixed by 30e06a2

@microbit-matt-hillsdon
Copy link
Contributor Author

I think this is in an OK place now @riknoll.

@microbit-matt-hillsdon
Copy link
Contributor Author

Looking at the conflicts...

microbit-matt-hillsdon and others added 3 commits May 23, 2025 21:51
Updates for:
- Awkward change in addConnectionHighlight that means Blockly now manages
  visibility and creates the highlights sooner.
- setEnabled removal
- dropdowns HTMLElement support
- plugin CSS
- toolbox integration with the plugin
- Reinstate outline around showLeds 'field' when focussed
- Active focus styling for flyout buttons and labels
- Fix focus handling for showLeds field editor
  The escape key now works as expected.
- Correct background color for play sound effect field
- We can now remove the patch script as isFullBlockField is public.
- Dropdown focus issue where dropdown div is used but widget div is focused
Explictly implement IBubble and add an implementation of the new-in-v12 method.
This hides the flyout if open fixing the global nav focusing the workspace when
the flyout is open.
@microbit-matt-hillsdon
Copy link
Contributor Author

Looking at the conflicts...

Fixed the conflicts. I've rebased, as it was easier for me to review the changes that way.

Blockly are intending to release an updated plugin later today that could be incorporated in this PR or as a follow-up depending on when it gets review.

Previously there was a separate but invisible position for the field.
This field should be full block, like Blockly colour fields.
Reduce impact of missing/changed action names.
The keybindings are now available as an array due to RaspberryPiFoundation/blockly#9
047
The Alt bindings were removed so an undefined entry was causing a conflict.
@microbit-matt-hillsdon
Copy link
Contributor Author

microbit-matt-hillsdon commented May 26, 2025

"show leds" mouse interaction glitch

With or without Accessible Blocks enabled:

  1. Click an LED, note it is not activated and top-left LED (regardless of target) has keyboard focus outline
  2. Keep clicking LEDs for logged error:
main.js:112 Uncaught TypeError: Cannot read properties of undefined (reading '2')
    at Object.unbind$$module$build$src$core$browser_events [as unbind] (main.js:112:79)
    at FieldLedMatrix.removeKeyboardFocusHandlers (main.js:13294:35)
    at SVGRectElement.<anonymous> (main.js:12979:18)
unbind$$module$build$src$core$browser_events @ main.js:112
removeKeyboardFocusHandlers @ main.js:13294
(anonymous) @ main.js:12979
  1. In this state, focus/block selection seems broken and various knock-on errors are possible.

Need to disentangle whether this is caused by the upgrade or perhaps the work on #10591 (though not easy to do as there were conflicts along the way, maybe easier just to fix).

Make sure that event listeners are added and removed at the right
points, and that fields are cleared properly.

Return ephemeral focus on pointer events as well as "Escape".
@microbit-matt-hillsdon
Copy link
Contributor Author

"show leds" mouse interaction glitch

This is now fixed so the PR is back to being reviewable/mergeable. We'll continue testing today too as this has certainly repaid testing to date with bugs!

Reinstate the blocklyFieldRect so it can get a rect-based outline because we
can't do a CSS outline on a group in Safari. That outline also had the wrong
size on Firefox for unclear reasons.

Needs some ugly CSS to deal with the hover effect from Blockly but that seems
an OK trade for the same DOM structure as other fields.

There's still an issue with the individual LEDs in Safari only.
Copy link
Member

@riknoll riknoll left a comment

Choose a reason for hiding this comment

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

one small change, otherwise LGTM

function initTooltip() {
const renderTip = (el: any) => {
if (el.disabled)
if (el.hasDisabledReason?.("disabled"))
Copy link
Member

Choose a reason for hiding this comment

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

use the disabled reason constant i defined in #10602

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, I fixed the others via conflicts but missed this one. 7159c20

This becomes more necessary in plugin versions where Blockly have restored
navigating to empty value connections but is an improvement anyway.

Behaviour change: the connection indicators now work for and/or blocks with
empty value connections.
@riknoll riknoll enabled auto-merge (squash) May 29, 2025 16:06
@riknoll riknoll merged commit ea8fc61 into microsoft:master May 29, 2025
12 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.

3 participants