github/excalidraw
4
0
Fork 0
mirror of https://github.com/excalidraw/excalidraw.git synced 2026-10-19 16:15:56 +00:00
Code Issues Projects Releases Wiki Activity

fix(editor): Dangling deleted bound text (#11733)

Browse Source 
---------

Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
This commit is contained in:
Márk Tolmács
2026-07-22 22:37:35 +02:00
committed by GitHub
parent a3b90897b5
commit 91b78903ab
4 changed files with 76 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/element/src/fractionalIndex.ts
+20 -4
View File
@@ -6,9 +6,11 @@ import {
} from "@excalidraw/fractional-indexing";
import { mutateElement, newElementWith } from "./mutateElement";
import { getBoundTextElement } from "./textElement";
import { getBoundTextElementId } from "./textElement";
import { hasBoundTextElement } from "./typeChecks";
import { isNonDeletedElement } from ".";
import type {
ElementsMap,
ExcalidrawElement,
@@ -65,6 +67,8 @@ export const validateFractionalIndices = (
const stringifyElement = (element: ExcalidrawElement | void) =>
`${element?.index}:${element?.id}:${element?.type}:${element?.isDeleted}:${element?.version}:${element?.versionNonce}`;
const elementsMap = includeBoundTextValidation ? arrayToMap(elements) : null;
const indices = elements.map((x) => x.index);
for (const [i, index] of indices.entries()) {
const predecessorIndex = indices[i - 1];
@@ -81,11 +85,23 @@ export const validateFractionalIndices = (
}
// disabled by default, as we don't fix it
if (includeBoundTextValidation && hasBoundTextElement(elements[i])) {
if (
includeBoundTextValidation &&
elementsMap &&
hasBoundTextElement(elements[i]) &&
isNonDeletedElement(elements[i])
) {
const container = elements[i];
const text = getBoundTextElement(container, arrayToMap(elements));
const boundTextElementId = getBoundTextElementId(container);
const text = boundTextElementId
? elementsMap.get(boundTextElementId)
: null;
if (text && text.index! <= container.index!) {
if (
text &&
isNonDeletedElement(text) &&
text.index! <= container.index!
) {
errorMessages.push(
`Fractional indices invariant for bound elements has been compromised: "${stringifyElement(
text,
packages/excalidraw/components/App.tsx
+47 -17
View File
@@ -3042,20 +3042,16 @@ class App extends React.Component<AppProps, AppState> {
// make sure editingTextElement points to latest element reference
if (actionResult.elements && editingTextElement) {
actionResult.elements.forEach((element) => {
if (
editingTextElement?.id === element.id &&
editingTextElement !== element &&
isNonDeletedElement(element) &&
isTextElement(element)
) {
editingTextElement = element;
}
});
}
if (editingTextElement?.isDeleted) {
editingTextElement = null;
const editingTextElementId = editingTextElement.id;
const nextElement = actionResult.elements.find(
(element) => element.id === editingTextElementId,
);
editingTextElement =
nextElement &&
isNonDeletedElement(nextElement) &&
isTextElement(nextElement)
? nextElement
: null;
}
this.setState((prevAppState) => {
@@ -4205,10 +4201,44 @@ class App extends React.Component<AppProps, AppState> {
});
}
// selection may only contain non-deleted elements. Resolved post-commit,
// so the elements we filter against are the latest.
const selectedElementIds = Object.keys(this.state.selectedElementIds);
if (selectedElementIds.length) {
const staleSelectedElementIds = selectedElementIds.filter((id) => {
const element = this.scene.getElement(id);
return !element || element.isDeleted;
});
// only update when actually stale, so we retain the object identity
// `selectedElementIds` is cached on (e.g. `Scene.getSelectedElements`)
if (staleSelectedElementIds.length) {
this.setState((prevState) => {
const nextSelectedElementIds = { ...prevState.selectedElementIds };
for (const id of staleSelectedElementIds) {
delete nextSelectedElementIds[id];
}
return {
selectedElementIds: makeNextSelectedElementIds(
nextSelectedElementIds,
prevState,
),
};
});
}
}
// failsafe in case the state is being updated in incorrect order resulting
// in the editingTextElement being now a deleted element
if (this.state.editingTextElement?.isDeleted) {
this.setState({ editingTextElement: null });
// in the editingTextElement being now a deleted element. Resolved against
// the scene by id, since the state holds an immutable snapshot whose
// `isDeleted` never flips once the element is deleted.
if (this.state.editingTextElement) {
const sceneElement = this.scene.getElement(
this.state.editingTextElement.id,
);
if (!sceneElement || sceneElement.isDeleted) {
this.setState({ editingTextElement: null });
}
}
// Forced false while a viewport animation runs — the scroll-back-to-content
packages/excalidraw/tests/__snapshots__/history.test.tsx.snap
+5 -5
View File
@@ -6391,7 +6391,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
exports[`history > multiplayer undo/redo > should iterate through the history when element changes relate only to remotely deleted elements > [end of test] number of elements 1`] = `3`;
exports[`history > multiplayer undo/redo > should iterate through the history when element changes relate only to remotely deleted elements > [end of test] number of renders 1`] = `17`;
exports[`history > multiplayer undo/redo > should iterate through the history when element changes relate only to remotely deleted elements > [end of test] number of renders 1`] = `18`;
exports[`history > multiplayer undo/redo > should iterate through the history when element changes relate only to remotely deleted elements > [end of test] redo stack 1`] = `[]`;
@@ -6823,7 +6823,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
exports[`history > multiplayer undo/redo > should iterate through the history when selected elements relate only to remotely deleted elements > [end of test] number of elements 1`] = `3`;
exports[`history > multiplayer undo/redo > should iterate through the history when selected elements relate only to remotely deleted elements > [end of test] number of renders 1`] = `15`;
exports[`history > multiplayer undo/redo > should iterate through the history when selected elements relate only to remotely deleted elements > [end of test] number of renders 1`] = `16`;
exports[`history > multiplayer undo/redo > should iterate through the history when selected elements relate only to remotely deleted elements > [end of test] redo stack 1`] = `[]`;
@@ -7243,7 +7243,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
exports[`history > multiplayer undo/redo > should iterate through the history when selected groups contain only remotely deleted elements > [end of test] number of elements 1`] = `4`;
exports[`history > multiplayer undo/redo > should iterate through the history when selected groups contain only remotely deleted elements > [end of test] number of renders 1`] = `15`;
exports[`history > multiplayer undo/redo > should iterate through the history when selected groups contain only remotely deleted elements > [end of test] number of renders 1`] = `16`;
exports[`history > multiplayer undo/redo > should iterate through the history when selected groups contain only remotely deleted elements > [end of test] redo stack 1`] = `[]`;
@@ -7465,7 +7465,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
exports[`history > multiplayer undo/redo > should iterate through the history when selected or editing linear element was remotely deleted > [end of test] number of elements 1`] = `1`;
exports[`history > multiplayer undo/redo > should iterate through the history when selected or editing linear element was remotely deleted > [end of test] number of renders 1`] = `11`;
exports[`history > multiplayer undo/redo > should iterate through the history when selected or editing linear element was remotely deleted > [end of test] number of renders 1`] = `12`;
exports[`history > multiplayer undo/redo > should iterate through the history when selected or editing linear element was remotely deleted > [end of test] redo stack 1`] = `[]`;
@@ -7742,7 +7742,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
exports[`history > multiplayer undo/redo > should iterate through the history when when element change relates to remotely deleted element > [end of test] number of elements 1`] = `1`;
exports[`history > multiplayer undo/redo > should iterate through the history when when element change relates to remotely deleted element > [end of test] number of renders 1`] = `9`;
exports[`history > multiplayer undo/redo > should iterate through the history when when element change relates to remotely deleted element > [end of test] number of renders 1`] = `10`;
exports[`history > multiplayer undo/redo > should iterate through the history when when element change relates to remotely deleted element > [end of test] redo stack 1`] = `[]`;
packages/excalidraw/tests/dragCreate.test.tsx
+4 -4
View File
@@ -311,9 +311,9 @@ describe("Test dragCreate", () => {
});
expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
`6`,
`7`,
);
expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`5`);
expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`6`);
expect(h.state.selectionElement).toBeNull();
expect(h.elements).toEqual([
expect.objectContaining({
@@ -345,9 +345,9 @@ describe("Test dragCreate", () => {
});
expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
`6`,
`7`,
);
expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`5`);
expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`6`);
expect(h.state.selectionElement).toBeNull();
expect(h.elements).toEqual([
expect.objectContaining({