You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@texera.apache.org by gi...@apache.org on 2026/09/17 00:34:17 UTC
(texera) branch gh-readonly-queue/main/pr-8055-7be4636b7642e3fa99542582cb481c3f5fa0fc65 created (now 2db7db0c1e)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-8055-7be4636b7642e3fa99542582cb481c3f5fa0fc65
in repository https://gitbox.apache.org/repos/asf/texera.git
at 2db7db0c1e fix(frontend): suppress newline on Enter in title editors (#8055)
This branch includes the following new commits:
new 2db7db0c1e fix(frontend): suppress newline on Enter in title editors (#8055)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(texera) 01/01: fix(frontend): suppress newline on Enter in title editors (#8055)
Posted by gi...@apache.org.
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 <xi...@uci.edu>
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