Skip to content

fix(editor): only fire change event on non-programmatic changes #433

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.

Sign up for GitHub

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

Merged
aomarks merged 1 commit into from
Oct 2, 2025
Merged

fix(editor): only fire change event on non-programmatic changes #433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,9 +861,9 @@ A pure text editor based on CodeMirror with syntax highlighting for HTML, CSS, J

### Events

| Event | Description |
| -------- | ------------------------------------ |
| `change` | User made an edit to the active file |
| Event | Description |
| -------- | -------------------------------------------------------------------------------------------------------------------------------------------------- |
| `change` | User made an edit to the active file (note: this event is not fired for programmatic changes to the value property nor for the user changing tabs) |

### Keyboard shortcuts

Expand Down
25 changes: 15 additions & 10 deletions src/playground-code-editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ interface TypedMap<T> extends Map<keyof T, unknown> {

const unreachable = (n: never) => n;

// Annotation to mark programmatic changes (not user edits)
const programmaticChangeAnnotation = Annotation.define<boolean>();

/**
* A basic text editor with syntax highlighting for HTML, CSS, and JavaScript.
*/
Expand Down Expand Up @@ -339,7 +342,6 @@ export class PlaygroundCodeEditor extends LitElement {
@queryAssignedElements({slot: 'extensions', flatten: true})
private _extensionElements!: HTMLElement[];

private _valueChangingFromOutside = false;
private _diagnosticDecorations: DecorationSet = Decoration.none;
private _diagnosticsMouseoverListenerActive = false;
private _lastTransactions: Transaction[] = [];
Expand Down Expand Up @@ -424,6 +426,7 @@ export class PlaygroundCodeEditor extends LitElement {
insert: this.value ?? '',
},
],
annotations: programmaticChangeAnnotation.of(true),
});
}
}
Expand All @@ -447,14 +450,13 @@ export class PlaygroundCodeEditor extends LitElement {
insert: this.value ?? '',
},
],
annotations: programmaticChangeAnnotation.of(true),
});
docState = tempView.state;
this._docCache.set(docKey, docState);
tempView.destroy();
}

this._valueChangingFromOutside = true;

// Replace the entire view with the new editor state. Unlike CM5, CM6
// is modular, and the history stays on the state object rather than
// the editor / view.
Expand All @@ -465,8 +467,6 @@ export class PlaygroundCodeEditor extends LitElement {
if (needsHideAndFold) {
void this._applyHideAndFoldRegions();
}

this._valueChangingFromOutside = false;
break;
}
case 'value':
Expand All @@ -476,8 +476,6 @@ export class PlaygroundCodeEditor extends LitElement {
}

if (this.value !== view?.state.doc.toString()) {
this._valueChangingFromOutside = true;

// The 'value' property was changed externally and it differs from
// the editor's current document content, so we need to update the
// view model to match.
Expand All @@ -492,9 +490,8 @@ export class PlaygroundCodeEditor extends LitElement {
insert: this.value ?? '',
},
],
annotations: programmaticChangeAnnotation.of(true),
});

this._valueChangingFromOutside = false;
}
break;
case 'lineNumbers':
Expand Down Expand Up @@ -720,9 +717,16 @@ export class PlaygroundCodeEditor extends LitElement {
tr.annotation(Transaction.userEvent) === 'redo'
);

// Check if ALL transactions are programmatic (not user edits).
// We fire the change event if ANY transaction is a user edit, even if
// there are also programmatic transactions in the same update.
const allProgrammatic = update.transactions.every(
(tr) => tr.annotation(programmaticChangeAnnotation) === true
);

// External changes are usually things like the editor switching which
// file it is displaying.
if (this._valueChangingFromOutside) {
if (allProgrammatic) {
// Apply hide/fold regions when value changes from outside
void this._applyHideAndFoldRegions();
this._showDiagnostics();
Expand All @@ -731,6 +735,7 @@ export class PlaygroundCodeEditor extends LitElement {
// Always reapply hide/fold regions after undo/redo
void this._applyHideAndFoldRegions();
}
// Only fire change event for user-initiated edits
this.dispatchEvent(new Event('change'));
}
}
Expand Down
123 changes: 123 additions & 0 deletions src/test/playground-code-editor_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,129 @@ suite('playground-code-editor', () => {
});
});

test('does NOT dispatch change event when switching documentKey and value together', async () => {
const editor = document.createElement('playground-code-editor');
const DOCUMENT_KEY1 = {doc: 1};
const DOCUMENT_KEY2 = {doc: 2};

editor.value = 'document 1';
editor.documentKey = DOCUMENT_KEY1;
container.appendChild(editor);
await editor.updateComplete;
await raf();

let changeCount = 0;
editor.addEventListener('change', () => {
changeCount++;
});

// This simulates what playground-file-editor does when switching files
// Both value AND documentKey change together in a single update
editor.value = 'document 2';
editor.documentKey = DOCUMENT_KEY2;

// Wait for Lit's update cycle
await editor.updateComplete;
await raf();

// Wait for any potential async change events
await wait(100);

// Switch back - this is where the bug manifests in lit.dev
editor.value = 'document 1 modified';
editor.documentKey = DOCUMENT_KEY1;
await editor.updateComplete;
await raf();
await wait(100);

assert.equal(
changeCount,
0,
'Switching documentKey and value together should not fire change event'
);
});

test('does NOT dispatch change event when switching documentKey', async () => {
const editor = document.createElement('playground-code-editor');
const DOCUMENT_KEY1 = {dockey: 1};
const DOCUMENT_KEY2 = {dockey: 2};

editor.value = 'document 1';
editor.documentKey = DOCUMENT_KEY1;
container.appendChild(editor);
await editor.updateComplete;
await raf();

let changeCount = 0;
editor.addEventListener('change', () => changeCount++);

// Switch to a different document - should NOT fire change event
editor.value = 'document 2';
editor.documentKey = DOCUMENT_KEY2;
await editor.updateComplete;
await raf();

assert.equal(
changeCount,
0,
'Switching documentKey should not fire change event'
);

// Switch back to first document - should NOT fire change event
editor.value = 'document 1';
editor.documentKey = DOCUMENT_KEY1;
await editor.updateComplete;
await raf();

assert.equal(
changeCount,
0,
'Switching back to original documentKey should not fire change event'
);
});

test('dispatches change event only for user edits, not programmatic changes', async () => {
const editor = document.createElement('playground-code-editor');
editor.value = 'foo';
container.appendChild(editor);
await editor.updateComplete;
await raf();

const editorInternals = editor as unknown as {
_editorView: EditorView;
};
Comment on lines +164 to +166
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

Accessing private members through type assertion is fragile and could break if the internal structure changes. Consider exposing a test-only public method or using a more robust testing approach.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@e111077 e111077 Oct 2, 2025

Choose a reason for hiding this comment

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

ehhhhhh i agree but I'm hoping CM7 aint coming soon

Copy link
Contributor

@justinfagnani justinfagnani Oct 2, 2025

Choose a reason for hiding this comment

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

Seems fine for this test, but could you also have fired a keyboard event into the widget? Just asking, no change needed.


let changeCount = 0;
editor.addEventListener('change', () => changeCount++);

// Programmatic change - should NOT fire
editor.value = 'bar';
await editor.updateComplete;
await raf();
assert.equal(changeCount, 0, 'No change event after programmatic update');

// User edit via CodeMirror - SHOULD fire
editorInternals._editorView.dispatch({
changes: {
from: 0,
to: editorInternals._editorView.state.doc.length,
insert: 'baz',
},
});
await raf();
assert.equal(changeCount, 1, 'Change event fired after user edit');

// Another programmatic change - should NOT fire
editor.value = 'qux';
await editor.updateComplete;
await raf();
assert.equal(
changeCount,
1,
'No additional change event after another programmatic update'
);
});

suite('history', () => {
let editor: PlaygroundCodeEditor;
let editorInternals: {
Expand Down