Skip to content

Conversation

@rachel-fenichel
Copy link
Collaborator

@rachel-fenichel rachel-fenichel commented May 5, 2025

The basics

The details

Resolves

Fix for #8962

Proposed Changes

Commit 1: Delete the actual drawer, subclasses, and uses
Commit 2: Delete residual SVG elements that were rendered through the cursor/marker flow instead of through the normal block flow.
Commits 3 and 4: lint and format.

Reason for Changes

MarkerSvg and friends represented a way to draw the cursor that was outside of the standard rendering flow. These SVG elements were children of the standard block (field, workspace, etc) SVG elements, but were updated through explicit calls to setCurNode on the cursor.

Ben's focus changes updated focus rendering to use elements that are rendered/updated through the standard rendering flow. Where possible, focus is shown using CSS rather than by creating new elements.

Test Coverage

Documentation

Additional Information

  • keyboard-experimentation will need a related PR to remove explicit calls to hide().
  • The old keyboard navigation plugin will not compile after this change (if it even compiled before).
  • IASTNodeLocationSvg can be removed as a follow-up, if it hasn't already been removed during refactors of ASTNode.

@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 5, 2025
@rachel-fenichel
Copy link
Collaborator Author

Does not fix #8968

@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 5, 2025
@rachel-fenichel rachel-fenichel marked this pull request as ready for review May 7, 2025 00:22
@rachel-fenichel rachel-fenichel requested a review from a team as a code owner May 7, 2025 00:22
@rachel-fenichel rachel-fenichel requested a review from BenHenning May 7, 2025 00:22
@rachel-fenichel rachel-fenichel requested review from cpcallen and removed request for BenHenning May 7, 2025 00:22
Copy link
Collaborator

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Opinion: I would (and now have) described issue #8962 as a task rather than a bug, so I'd have titled this PR and its main commits as chore: rather than fix:.

Comment on lines -1331 to -1346
renderManagement
.finishQueuedRenders()
.then(() => void this.markerManager.updateMarkers());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should render no longer call renderManagement.finishQueuedRenders() at all?

(Maybe the queued renderers will be finished automatically, and the method call is not needed if you don't need the resulting promise?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, queued renders will finish automatically, and the method call is specifically to be able to get a promise and run after the renders have finished.

export interface IASTNodeLocationSvg extends IASTNodeLocation {

}
export interface IASTNodeLocationSvg extends IASTNodeLocation {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this interface not be deleted entirely, now that it is empty?

I suppose we could formally deprecate it first, but since anyone who was calling setFooSvg on an IASTNodeLocationSvg instance already has broken code I'm not sure it's worth it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Filed as #9006

@cpcallen cpcallen assigned rachel-fenichel and unassigned cpcallen May 7, 2025
@cpcallen
Copy link
Collaborator

cpcallen commented May 7, 2025

For some reason GitHub hasn't run build, format or lint workflows, and I can't figure out how to kick it in to action.

@rachel-fenichel
Copy link
Collaborator Author

Workflows ran and passed.

@rachel-fenichel rachel-fenichel merged commit bb76d6e into RaspberryPiFoundation:rc/v12.0.0 May 7, 2025
7 checks passed
@github-actions github-actions bot removed the PR: fix Fixes a bug label May 7, 2025
@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 breaking change Used to mark a PR or issue that changes our public APIs. labels May 7, 2025
@rachel-fenichel rachel-fenichel deleted the excise-marker branch May 7, 2025 16:37
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.

3 participants