Skip to content

fix: visit all connection candidates in move mode#9641

Merged
mikeharv merged 5 commits intoRaspberryPiFoundation:v13from
mikeharv:connection-pairs
Mar 20, 2026
Merged

fix: visit all connection candidates in move mode#9641
mikeharv merged 5 commits intoRaspberryPiFoundation:v13from
mikeharv:connection-pairs

Conversation

@mikeharv
Copy link
Contributor

The basics

The details

Resolves

Fixes #9631


Proposed Changes

  • Fixes keyboard navigation skipping valid connection candidates (now matches mouse behavior)
  • Replaces incremental traversal with a precomputed list of valid connection pairs
  • Ensures deterministic, complete, and cyclic traversal
  • Improves traversal order by reordering local connections for more natural right/down movement (configurable with a natural flag)
  • Adds initialConnectionPair so traversal starts from the block’s original connection
  • Refines constrained-move positioning for more consistent visual alignment

Reason for Changes

Keyboard navigation previously missed valid connection targets.

This PR makes traversal complete, predictable, and easier to reason about.


Test Coverage

  • Updated tests to include previously skipped candidates (now expected).
  • Manually tested keyboard navigation on complex block structures.

Documentation

Additional Information

@mikeharv mikeharv requested a review from a team as a code owner March 19, 2026 16:14
@mikeharv mikeharv requested review from gonfunko and removed request for a team March 19, 2026 16:14
@github-actions github-actions bot added the PR: fix Fixes a bug label Mar 19, 2026

/** Where a constrained movement should start when traversing the tree. */
private searchNode: RenderedConnection | null = null;
private localConnections: RenderedConnection[] | null = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd kind of prefer to just fetch this dynamically in the two places it's used; I'm not adverse to instance variables in general, but we have a lot of different connection permutations right now that make it difficult to reason about things, vs just one list of pairs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. My original intent with caching localConnections was to avoid recomputing them on every drag frame, since getConnectionCandidate can be hit pretty frequently during pointer moves. Happy to fetch dynamically if the readability outweighs any slight performance dips. Presumably it's worked this way for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point, I think since it's just the immediate connections on the dragging block it should be fine, but worth keeping in mind if we do run into any performance issues.

private getLocalConnections(draggingBlock: BlockSvg): RenderedConnection[] {
private getLocalConnections(
draggingBlock: BlockSvg,
natural: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we just always want the natural behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I wasn't sure if we might want to make it configurable or A/B testable later. For example, the natural behavior could be good for sighted users but less intuitive for screen reader users. Easy enough to add in again if we ever want it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense! I'd ask Maribeth for her thoughts as well, but typically I prefer to shy away from configuration options without a clear and present need as anything we add to the API surface is something we're committing to maintain ~forever. Granted this method is private so that's not directly a concern here, but as a general principle that's how I tend to approach things. We also don't have any mechanism for A/B testing since we're operating solely at the library level and intentionally don't have any sort of metrics/analytics since that would affect every partner/team/product that uses Blockly and their actual end users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points! Thanks for explaining.

@mikeharv mikeharv requested a review from gonfunko March 20, 2026 15:08
@mikeharv mikeharv merged commit 8e6798a into RaspberryPiFoundation:v13 Mar 20, 2026
5 of 6 checks passed
@mikeharv mikeharv deleted the connection-pairs branch March 20, 2026 15:25
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.

2 participants