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: Detect if deleting shadow block affects selection highlight #8533

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

johnnesky
Copy link
Member

@johnnesky johnnesky commented Aug 16, 2024

The basics

The details

Resolves

Partially addresses: google/blockly-samples#2426

Proposed Changes

Detects whether deleting a block would delete the currently selected block, and if so, deselects it before severing connections.

Reason for Changes

When a shadow block is selected, an ancestor of that block receives a visual highlight. If the shadow block is then deleted (e.g. because it was replaced with a real block via the shadow block converter plugin), then the highlighted block no longer has any relationship to any selected block. That highlight should go away, but currently it persists and may continue to persist while making further changes to the selection.

Note that Block.dispose() severs a connection, and Block.disposeInternal() also severs connections, so we need to determine whether any parent block needs to be updated before this happens. The subclass BlockSvg overrides these methods, so I chose to perform the detection and resolution in BlockSvg.dispose().

Test Coverage

Existing tests pass. I added new tests for the new features.

Documentation

N/A

Additional Information

I think it feels unintuitive that, when using the shadow block converter plugin and the user edits a shadow block, the current selection gets cleared. As far as the user is concerned, nothing appears to have been deleted (since the real clone of the shadow block seamlessly replaces the shadow block) so it's weird that editing a field of a shadow block deselects the selection.

I propose that we fix this in the shadow block converter plugin by transferring the selection to the new real clone of the selected shadow block after replacing it. Since that's in the samples repo, it would have to be a separate PR. In the meantime, the shadow block converter plugin is already broken and I don't think this PR makes it worse.

@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Aug 16, 2024
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Aug 17, 2024
@johnnesky johnnesky marked this pull request as ready for review August 17, 2024 02:40
@johnnesky johnnesky requested a review from a team as a code owner August 17, 2024 02:40
@johnnesky johnnesky requested a review from BeksOmega August 17, 2024 02:40
Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Looks lovely, just one docs addition =)

Thank you for the beautiful tests!

core/block.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Aug 24, 2024
@johnnesky johnnesky merged commit 095f29e into google:develop Aug 26, 2024
8 checks passed
@johnnesky johnnesky deleted the nesky_shadow_selection branch September 14, 2024 01:49
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