This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8055-7be4636b7642e3fa99542582cb481c3f5fa0fc65 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 2db7db0c1e5ddf168cd89ef5875d2f91b84600a2 Author: Xinyuan Lin <[email protected]> AuthorDate: Thu Sep 17 00:34:02 2026 +0000 fix(frontend): suppress newline on Enter in title editors (#8055) ### What changes were proposed in this PR? The operator and port title editors bind Enter to "commit and close" so that a display name stays a single-line value. Since the move to Quill 2 (#6418) that suppression has been dead: Enter inserted a literal newline into the shared `Y.Text`, which is published to every co-editor and persisted with the workflow. Root cause — Quill 2 resolves a keydown against two buckets, `event.key` first: ``` bindings[evt.key] ++ bindings[evt.which] // "Enter" bucket, then 13 bucket matches.some(handler) // first non-`true` return wins, loop stops ``` Quill's own `handleEnter` is registered under `"Enter"`; the editors registered theirs under the legacy `13` keycode. So Quill's handler ran first, inserted the newline, and short-circuited the loop before the editors' handler was reached. The template's separate `(keyup.enter)` still closed the editor on key *release*, so the rename looked like it worked. ``` Before: Enter -> Quill handleEnter -> "\n" into shared text -> keyup closes editor After: Enter -> editor handler -> commit and close, text untouched ``` Keying both bindings `"Enter"` puts them in the same bucket, registered ahead of `handleEnter` (user options are added before Quill's built-ins). | Rename typed | Enter pressed at | Stored name before | Stored name after | | --- | --- | --- | --- | | `Sentiment Analysis` | end | `"Sentiment Analysis\n"` | `"Sentiment Analysis"` | | `Sentiment Analysis` | mid-word | `"Sen\ntiment Analysis"` | `"Sentiment Analysis"` | | *(untouched)* | start | `"\n"` | `""` | No visual change: the editor renders identically before and after, which is why this went unnoticed — the defect is only in the value that gets stored. `CollabWrapperComponent` carries the same `key: 13`, but it is unreachable (its call site is commented out) and #7351 deletes it, so it is left alone here. ### Any related issues, documentation, discussions? Closes #8053. Regression from #6418 (Quill 1 -> 2). Related: #7351. ### How was this PR tested? New regression tests in both title editors' specs mount the real Quill instance and the real y-quill binding, open the editor through the template's edit button, and press Enter as a **keydown only** — the template's `(keyup.enter)` fallback is never dispatched, so it cannot mask a broken binding. Each asserts the shared `Y.Text` and `editingTitle`. Coverage: caret at the end, mid-word, an empty name, Shift+Enter, and an ordinary keystroke that must still reach the editor (`defaultPrevented === false`, editor stays open). The four positive tests fail on `main` and pass here; the negative one passes both ways, as a control: ``` AssertionError: expected 'renamed\n' to be 'renamed' AssertionError: expected 'ren\named' to be 'renamed' AssertionError: expected '\n' to be '' ``` ```bash cd frontend && yarn install --frozen-lockfile ``` ```bash cd frontend && npx ng test --include "src/app/workspace/component/property-editor/**/*.spec.ts" --watch=false ``` `316 passed | 1 skipped (317)`. Full frontend suite: `204 files, 5213 passed`. `yarn build` (production) succeeds. `npx prettier --check` and `npx eslint` clean. jsdom has no layout engine, so `Range#getBoundingClientRect` is stubbed next to the existing test-env polyfills — Quill calls it to place the caret and quill-cursors calls it per remote cursor. Manually verified in Chromium against Quill 2.0.3 with both binding forms side by side and a real Enter keystroke: `key: 13` stored `"Sentiment Analysis\n"` and left the editor open; `key: "Enter"` stored `"Sentiment Analysis"` and closed it. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code, Opus 5 --- .../operator-property-edit-frame.component.spec.ts | 163 +++++++++++++++++++++ .../operator-property-edit-frame.component.ts | 4 +- .../port-property-edit-frame.component.spec.ts | 88 ++++++++++- .../port-property-edit-frame.component.ts | 4 +- frontend/src/jsdom-svg-polyfill.ts | 12 ++ 5 files changed, 266 insertions(+), 5 deletions(-) diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts index fa551d1402..9cf755d575 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts @@ -66,6 +66,7 @@ import { of, Subject, throwError } from "rxjs"; import { WorkflowVersionService } from "../../../../dashboard/service/user/workflow-version/workflow-version.service"; import { GuiConfigService } from "../../../../common/service/gui-config.service"; import { PresetWrapperComponent } from "src/app/common/formly/preset-wrapper/preset-wrapper.component"; +import * as Y from "yjs"; const { marbles } = configure({ run: false }); @@ -3130,4 +3131,166 @@ describe("OperatorPropertyEditFrameComponent", () => { expect(decorated).toEqual(["limit"]); }); }); + + /** + * The spec's default TestBed swaps the template for a stub, so the collaborative title editor — + * the Quill instance the component mounts on `#customName` and the keyboard bindings it configures + * — is never built by anything above. This block renders the real template and drives that editor. + * It comes last in the file deliberately: Quill 2 has no `destroy()`, so each instance leaves a + * MutationObserver and document listeners behind for `fixture.destroy()` to detach, and it must not + * be combined with `fakeAsync` (zone.js patches MutationObserver). + */ + describe("quill title editing", () => { + let quillFixture: ComponentFixture<OperatorPropertyEditFrameComponent>; + let quillComponent: OperatorPropertyEditFrameComponent; + let texeraGraph: WorkflowGraph; + + beforeEach(async () => { + TestBed.resetTestingModule(); + await TestBed.configureTestingModule({ + providers: [ + WorkflowActionService, + { provide: OperatorMetadataService, useClass: StubOperatorMetadataService }, + { provide: ComputingUnitStatusService, useClass: MockComputingUnitStatusService }, + DatePipe, + ...commonTestProviders, + ], + imports: [ + OperatorPropertyEditFrameComponent, + BrowserAnimationsModule, + FormsModule, + FormlyModule.forRoot(TEXERA_FORMLY_CONFIG), + FormlyNgZorroAntdModule, + ReactiveFormsModule, + HttpClientTestingModule, + ], + }).compileComponents(); + + quillFixture = TestBed.createComponent(OperatorPropertyEditFrameComponent); + quillComponent = quillFixture.componentInstance; + // Downcast to the concrete graph: `getSharedOperatorType` is not on the readonly view. + texeraGraph = TestBed.inject(WorkflowActionService).getTexeraGraph() as WorkflowGraph; + quillFixture.detectChanges(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + quillFixture.destroy(); + }); + + /** Opens the editor the only way a user can — the title section's edit button. */ + function openEditorFromButton(): Y.Text { + const sharedOperator = new Y.Doc().getMap("operator"); + quillComponent.currentOperatorId = mockScanPredicate.operatorID; + // `interactive` defaults to false, which renders the edit button disabled — a disabled button + // swallows the click silently and the editor would never mount. + quillComponent.interactive = true; + vi.spyOn(texeraGraph, "getSharedOperatorType").mockReturnValue(sharedOperator as any); + quillFixture.detectChanges(); + + quillFixture.debugElement.query(By.css("#formly-title button")).nativeElement.click(); + quillFixture.detectChanges(); + + expect(quillComponent.editingTitle).toBe(true); + expect(quillComponent.quillBinding).toBeDefined(); + // The map genuinely lacked the key, so this Y.Text can only have come from the connect call. + const sharedTitle = sharedOperator.get("customDisplayName"); + expect(sharedTitle).toBeInstanceOf(Y.Text); + return sharedTitle as Y.Text; + } + + /** + * An operator display name is a single-line value, so the editor binds Enter to "commit and + * close" instead of letting Quill insert a newline. Everything typed here goes straight into the + * shared Y.Text, so a stray "\n" is published to every co-editor, persisted with the workflow, + * and read back on each later open. Quill 2 buckets keyboard bindings by `event.key` and runs its + * built-in `handleEnter` ahead of anything registered under the legacy `13` keycode, which is how + * the suppression stopped working when the frontend moved to Quill 2 (#8053). The template's + * `(keyup.enter)` closes the editor either way — a keydown-only press is what separates a working + * binding from a broken one, in both the text left behind and `editingTitle`. + */ + const ENTER: KeyboardEventInit = { key: "Enter", keyCode: 13, which: 13 }; + + /** + * Quill's keydown listener returns early unless the editor `hasFocus()` and reports a selection, + * so both have to be real here — without them no binding runs at all and every assertion below + * would pass vacuously. `keyCode` / `which` mirror what a browser sends: Quill matches a binding + * on either, and jsdom's default of 0 would make a `13`-keyed binding unreachable rather than + * merely out-prioritised, hiding the very defect these tests pin. + */ + function pressKey(caret: number, init: KeyboardEventInit): KeyboardEvent { + quillComponent.quill.root.focus(); + quillComponent.quill.setSelection(caret, 0); + const keyPress = new KeyboardEvent("keydown", { bubbles: true, cancelable: true, ...init }); + quillComponent.quill.root.dispatchEvent(keyPress); + quillFixture.detectChanges(); + return keyPress; + } + + it("should show a remote rename in the mounted editor", () => { + const sharedTitle = openEditorFromButton(); + + sharedTitle.insert(0, "renamed remotely"); + + // Without this the keyboard tests below could pass against an editor wired to nothing. + expect((document.getElementById("customName") as HTMLElement).textContent).toContain("renamed remotely"); + }); + + it("should commit the rename on Enter without appending a newline to the shared name", () => { + const sharedTitle = openEditorFromButton(); + sharedTitle.insert(0, "renamed"); + + pressKey(sharedTitle.length, ENTER); + + expect(sharedTitle.toString()).toBe("renamed"); + // `editingTitle` is what proves the editor's own binding ran: no keyup was dispatched, so the + // template's fallback cannot be the thing that closed the editor. + expect(quillComponent.editingTitle).toBe(false); + expect(quillComponent.quillBinding).toBeUndefined(); + }); + + it("should not split the shared name when Enter is pressed mid-word", () => { + const sharedTitle = openEditorFromButton(); + sharedTitle.insert(0, "renamed"); + + pressKey(3, ENTER); + + expect(sharedTitle.toString()).toBe("renamed"); + expect(quillComponent.editingTitle).toBe(false); + }); + + it("should leave an untouched name empty when Enter commits it", () => { + const sharedTitle = openEditorFromButton(); + + pressKey(0, ENTER); + + // The empty document is the case Quill's own handler turns into a lone "\n" — a display name + // that reads as blank but is no longer equal to the empty string it started as. + expect(sharedTitle.toString()).toBe(""); + expect(quillComponent.editingTitle).toBe(false); + }); + + it("should commit on Shift+Enter without appending a newline either", () => { + const sharedTitle = openEditorFromButton(); + sharedTitle.insert(0, "renamed"); + + pressKey(sharedTitle.length, { ...ENTER, shiftKey: true }); + + expect(sharedTitle.toString()).toBe("renamed"); + expect(quillComponent.editingTitle).toBe(false); + }); + + it("should leave ordinary typing alone", () => { + const sharedTitle = openEditorFromButton(); + sharedTitle.insert(0, "renamed"); + + const keyPress = pressKey(sharedTitle.length, { key: "a", keyCode: 65, which: 65 }); + + // Suppressing Enter must not become suppressing the keyboard: an ordinary character is left + // for the browser to insert, and the editor stays open so the rename can continue. + expect(keyPress.defaultPrevented).toBe(false); + expect(quillComponent.editingTitle).toBe(true); + expect(quillComponent.quillBinding).toBeDefined(); + }); + }); }); diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts index 033b75bc48..3fd8f1afcc 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts @@ -1511,11 +1511,11 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On keyboard: { bindings: { enter: { - key: 13, + key: "Enter", handler: () => this.disconnectQuillFromText(), }, shift_enter: { - key: 13, + key: "Enter", shiftKey: true, handler: () => this.disconnectQuillFromText(), }, diff --git a/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.spec.ts b/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.spec.ts index d12b316d33..3d751dffec 100644 --- a/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.spec.ts @@ -512,7 +512,7 @@ describe("PortPropertyEditFrameComponent", () => { * `(focusout)` bindings — unexercised. Without these the whole feature can be unwired from the * UI without a single test noticing. */ - function openEditorFromButton(): void { + function openEditorFromButton(): Y.Map<unknown> { const sharedPortDescription = new Y.Doc().getMap("portDescription"); component.currentPortID = inputPort; vi.spyOn(texeraGraph, "getSharedPortDescriptionType").mockReturnValue(sharedPortDescription); @@ -522,6 +522,7 @@ describe("PortPropertyEditFrameComponent", () => { expect(component.editingTitle).toBe(true); expect(component.quillBinding).toBeDefined(); + return sharedPortDescription; } it("should open the collaborative editor from the edit button and close it on Enter", () => { @@ -543,5 +544,90 @@ describe("PortPropertyEditFrameComponent", () => { expect(component.editingTitle).toBe(false); expect(component.quillBinding).toBeUndefined(); }); + + /** + * A port display name is a single-line value, so the editor binds Enter to "commit and close" + * instead of letting Quill insert a newline. Everything typed here goes straight into the shared + * Y.Text, so a stray "\n" is published to every co-editor, persisted with the workflow, and read + * back on each later open. Quill 2 buckets keyboard bindings by `event.key` and runs its built-in + * `handleEnter` ahead of anything registered under the legacy `13` keycode, which is how the + * suppression stopped working when the frontend moved to Quill 2 (#8053). The template's + * `(keyup.enter)` closes the editor either way — a keydown-only press is what separates a working + * binding from a broken one, in both the text left behind and `editingTitle`. + */ + const ENTER: KeyboardEventInit = { key: "Enter", keyCode: 13, which: 13 }; + + /** + * Quill's keydown listener returns early unless the editor `hasFocus()` and reports a selection, + * so both have to be real here — without them no binding runs at all and every assertion below + * would pass vacuously. `keyCode` / `which` mirror what a browser sends: Quill matches a binding + * on either, and jsdom's default of 0 would make a `13`-keyed binding unreachable rather than + * merely out-prioritised, hiding the very defect these tests pin. + */ + function pressKey(caret: number, init: KeyboardEventInit): KeyboardEvent { + component.quill.root.focus(); + component.quill.setSelection(caret, 0); + const keyPress = new KeyboardEvent("keydown", { bubbles: true, cancelable: true, ...init }); + component.quill.root.dispatchEvent(keyPress); + fixture.detectChanges(); + return keyPress; + } + + it("should commit the rename on Enter without appending a newline to the shared name", () => { + const sharedTitle = openEditorFromButton().get("displayName") as Y.Text; + sharedTitle.insert(0, "renamed"); + + pressKey(sharedTitle.length, ENTER); + + expect(sharedTitle.toString()).toBe("renamed"); + // `editingTitle` is what proves the editor's own binding ran: no keyup was dispatched, so the + // template's fallback cannot be the thing that closed the editor. + expect(component.editingTitle).toBe(false); + expect(component.quillBinding).toBeUndefined(); + }); + + it("should not split the shared name when Enter is pressed mid-word", () => { + const sharedTitle = openEditorFromButton().get("displayName") as Y.Text; + sharedTitle.insert(0, "renamed"); + + pressKey(3, ENTER); + + expect(sharedTitle.toString()).toBe("renamed"); + expect(component.editingTitle).toBe(false); + }); + + it("should leave an untouched name empty when Enter commits it", () => { + const sharedTitle = openEditorFromButton().get("displayName") as Y.Text; + + pressKey(0, ENTER); + + // The empty document is the case Quill's own handler turns into a lone "\n" — a display name + // that reads as blank but is no longer equal to the empty string it started as. + expect(sharedTitle.toString()).toBe(""); + expect(component.editingTitle).toBe(false); + }); + + it("should commit on Shift+Enter without appending a newline either", () => { + const sharedTitle = openEditorFromButton().get("displayName") as Y.Text; + sharedTitle.insert(0, "renamed"); + + pressKey(sharedTitle.length, { ...ENTER, shiftKey: true }); + + expect(sharedTitle.toString()).toBe("renamed"); + expect(component.editingTitle).toBe(false); + }); + + it("should leave ordinary typing alone", () => { + const sharedTitle = openEditorFromButton().get("displayName") as Y.Text; + sharedTitle.insert(0, "renamed"); + + const keyPress = pressKey(sharedTitle.length, { key: "a", keyCode: 65, which: 65 }); + + // Suppressing Enter must not become suppressing the keyboard: an ordinary character is left + // for the browser to insert, and the editor stays open so the rename can continue. + expect(keyPress.defaultPrevented).toBe(false); + expect(component.editingTitle).toBe(true); + expect(component.quillBinding).toBeDefined(); + }); }); }); diff --git a/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.ts b/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.ts index 3eeb4e3fc1..e8808c2279 100644 --- a/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.ts +++ b/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.ts @@ -248,11 +248,11 @@ export class PortPropertyEditFrameComponent implements OnInit, OnChanges { keyboard: { bindings: { enter: { - key: 13, + key: "Enter", handler: () => this.disconnectQuillFromText(), }, shift_enter: { - key: 13, + key: "Enter", shiftKey: true, handler: () => this.disconnectQuillFromText(), }, diff --git a/frontend/src/jsdom-svg-polyfill.ts b/frontend/src/jsdom-svg-polyfill.ts index 7aef063d7d..881a9c5c0f 100644 --- a/frontend/src/jsdom-svg-polyfill.ts +++ b/frontend/src/jsdom-svg-polyfill.ts @@ -111,6 +111,18 @@ installIfMissing(G.SVGGraphicsElement?.prototype, { getBBox: fakeRect as AnyFn, }); +// `Range.prototype.getBoundingClientRect` — jsdom has no layout engine and +// doesn't implement it. Quill's `Selection#getBounds` calls it on a live +// Range to place the caret, and quill-cursors calls it again for every +// remote cursor an awareness update carries, so any spec that mounts a +// collaborative editor throws `range.getBoundingClientRect is not a +// function`. A zeroed rect is enough — jsdom never paints, and specs that +// need real caret geometry belong in browser mode. `top` / `left` / `right` +// / `height` are the four fields `getBounds` reads back off it. +installIfMissing(G.Range?.prototype, { + getBoundingClientRect: (() => ({ ...fakeRect(), top: 0, right: 0, bottom: 0, left: 0 })) as AnyFn, +}); + // Constructable Stylesheets API (`new CSSStyleSheet().replaceSync(...)`) — // jsdom doesn't ship it, but @codingame/monaco-vscode-api v25 calls it at // module load. Stub with an inert constructor; specs don't visually render
