Copilot commented on code in PR #8442:
URL: https://github.com/apache/texera/pull/8442#discussion_r3981542548
##########
frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts:
##########
@@ -631,10 +636,19 @@ export class OperatorPropertyEditFrameComponent
implements OnInit, OnChanges, On
this.currentOperatorSchema =
this.dynamicSchemaService.getDynamicSchema(this.currentOperatorId);
this.currentOperatorStatus =
this.workflowStatusSerivce.getCurrentStatus()[this.currentOperatorId];
-
this.workflowActionService.getTexeraGraph().updateSharedModelAwareness("currentlyEditing",
this.currentOperatorId);
+ if (this.broadcastEditing) {
+ this.workflowActionService
+ .getTexeraGraph()
+ .updateSharedModelAwareness("currentlyEditing",
this.currentOperatorId);
+ }
const operator =
this.workflowActionService.getTexeraGraph().getOperator(this.currentOperatorId);
- // set the operator data needed
- this.workflowActionService.setOperatorVersion(operator.operatorID,
this.currentOperatorSchema.operatorVersion);
+ // Syncing the operator to the current schema version writes the new
version into the Yjs shared
+ // model (changeOperatorVersion), which broadcasts and persists. That is
right on the canvas, but
+ // a read-only inspect (broadcastEditing=false) must not mutate the
workflow just by opening a
+ // step, so skip the sync there and show the version as stored.
+ if (this.broadcastEditing) {
+ this.workflowActionService.setOperatorVersion(operator.operatorID,
this.currentOperatorSchema.operatorVersion);
Review Comment:
The read-only guard still allows opening this frame to mutate operator
properties. `rerenderEditorForm()` applies AJV defaults and unconditionally
calls `onFormChanges(this.formData)` at lines 705–714; after the debounce,
`registerOnFormChangeHandler()` writes any added defaults through
`setOperatorProperty()`. Thus inspecting an older operator with a newly
defaulted field changes the shared workflow even though `broadcastEditing` is
false. Please gate every write-producing initialization path (including the
form-change and UI-parameter sync handlers), ideally with a dedicated
read-only/allow-writes input, and add a fake-timer regression test asserting
neither version nor properties are written.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -236,8 +243,48 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
// Give the result tables a realistic height to page against, so they show
a screenful of rows
// instead of one. (~7 rows; the card scrolls for the rest.)
this.panelResizeService.changePanelSize(900, 560);
+ // Highlighting is off by default; turning it on is what makes a click on
a step select it,
+ // which is how a reader opens that step's panel to inspect it (and,
later, an author to expose).
+ this.workflowActionService.setHighlightingEnabled(true);
this.load(wid);
+ // Selecting a step on the embedded (read-only) canvas opens its property
panel read-only. The
+ // canvas is not editable, but highlighting still works, so reuse it
rather than teach the editor
+ // a second click mode.
+ this.workflowActionService
+ .getJointGraphWrapper()
+ .getJointOperatorHighlightStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(() => {
+ // The stream emits only the newly-highlighted ids, not the whole
selection, so read the
+ // current selection to decide -- the same source the property panel
uses. Exactly one
+ // highlighted step opens the panel; a shift-click multi-select opens
nothing (the panel
+ // shows no single operator either), rather than opening whichever
step was clicked last.
+ const selected =
this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs();
+ if (selected.length === 1) {
+ this.onOperatorClicked(selected[0]);
+ } else {
+ this.clearSelection();
+ }
+ // The panel is mounted with [broadcastEditing]="false", so opening a
step here never
+ // announces "currently editing this operator" on the shared co-editor
channel -- a reader
+ // inspecting a step is not editing the graph, and broadcasting would
print the reader's own
+ // name in colour over that operator on everyone else's canvas.
Suppressed at the frame (the
+ // only place that writes it), not here, so it cannot be re-set after
this handler runs.
+ });
+
+ // Clicking empty canvas clears the highlight; the panel should go with it.
+ this.workflowActionService
+ .getJointGraphWrapper()
+ .getJointOperatorUnhighlightStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(() => {
+ if
(this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs().length
=== 0) {
+ this.clearSelection();
+ this.cdr.detectChanges();
+ }
+ });
Review Comment:
This only handles the zero-selection case. After a shift multi-select closes
the panel, shift-clicking one selected operator off leaves exactly one operator
highlighted; `PropertyEditorComponent` then renders that remaining operator,
but `selectedOperatorId` stays `undefined`, so `[hidden]` keeps the panel
closed. Recompute the selected ID on unhighlight using the same exact-one rule
as the highlight handler.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.html:
##########
@@ -221,6 +221,40 @@ <h2>{{ instructionTitle || "How to use this" }}</h2>
<texera-mini-map
*ngIf="workflowEverOpened"
class="box"></texera-mini-map>
+
+ <!-- Sibling of the panel, not nested inside it: the property editor
opens its own stacking
+ context, and a button inside that context paints under its
content -- visible but
+ unclickable. As a sibling the close button is simply above. -->
+ <button
+ class="panel-close"
+ *ngIf="selectedOperatorId"
+ type="button"
+ aria-label="Close step details"
+ (click)="closeOperatorPanel()">
+ <i
+ nz-icon
+ nzType="close"
+ aria-hidden="true"></i>
+ </button>
+
+ <!-- A reader can open a step to view its settings, read-only. The
panel itself is the
+ scroll container (so a long panel can still be read); the
property editor inside carries
+ the `inert` attribute, which blocks pointer AND keyboard AND
focus -- so nothing in it
+ can be edited or tabbed into. The graph is modification-disabled
too. Turning the panel
+ live for choosing what to expose is the authoring PR.
+ [hidden], not *ngIf: the property editor shows its operator by
REACTING to the highlight
+ stream (no initial pull), so it must already be mounted and
subscribed when the click
+ highlights a step. Mounting it on selection (*ngIf) subscribes
too late, misses that
+ emission, and the panel opens empty. So keep it mounted and just
hide it. -->
+ <div
+ class="panel"
+ [hidden]="!selectedOperatorId">
+ <texera-property-editor
+ [exposeChoosing]="false"
+ [persistPlacement]="false"
+ [broadcastEditing]="false"
+ [attr.inert]="''"></texera-property-editor>
Review Comment:
Applying `inert` to the entire property editor removes all of its settings
from the accessibility tree, so screen-reader users cannot inspect the
read-only values at all; it also leaves no focusable region for keyboard users
to scroll a long inner panel. Please implement a read-only presentation that
disables mutation controls while preserving semantic text/labels (and keyboard
scrolling) instead of making the content subtree inert.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -1043,6 +1090,34 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
return this.rendered.some(r => hasRequiredError(r.form));
}
+ //
---------------------------------------------------------------------------
+ // Inspecting a step: its property panel, opened read-only from the preview
+ //
---------------------------------------------------------------------------
+
+ /** Clicking a step opens the workflow's own property panel for it,
read-only. */
+ public onOperatorClicked(operatorID: string): void {
+ const graph = this.workflowActionService.getTexeraGraph();
+ if (!graph.hasOperator(operatorID)) {
+ this.clearSelection();
+ return;
+ }
+ this.selectedOperatorId = operatorID;
+ this.cdr.detectChanges();
+ }
+
+ /** Dismiss the panel: the selection is what holds it open, so drop the
highlight and the selection. */
+ public closeOperatorPanel(): void {
+ const wrapper = this.workflowActionService.getJointGraphWrapper();
+
wrapper.unhighlightOperators(...wrapper.getCurrentHighlightedOperatorIDs());
Review Comment:
Selection is created through `WorkflowActionService.highlightOperators()`,
which also publishes the `highlighted` awareness field, but this direct wrapper
call only changes local JointJS state. Closing the panel therefore leaves
co-editors showing a stale highlight for this reader. Use the service-level
unhighlight method so local selection and shared awareness are cleared together.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts:
##########
@@ -75,6 +77,19 @@ import { GuiConfigService } from
"../../../common/service/gui-config.service";
* name/avatar row, the Canvas switch actually firing, the loading/body swap,
and the co-editor
* row -- which is the review's evidence of the rendered page in place of a
screenshot.
*/
+// A stand-in for the always-mounted property panel. The real one is heavy --
its ngOnInit
+// subscribes to the full JointJS highlight-stream set and the panel service
-- and it has its
+// own spec. This page only needs the panel present (it lives behind [hidden],
not *ngIf, so it
+// is mounted from the start to catch the highlight that opens it), so swap in
a stub carrying the
+// two inputs the template binds and nothing else. The swap is on a child of
the page, so the
+// page's own template still renders as shipped and stays covered.
+@Component({ selector: "texera-property-editor", template: "", standalone:
true })
+class MockPropertyEditorComponent {
+ @Input() exposeChoosing = false;
+ @Input() persistPlacement = true;
+ @Input() broadcastEditing = true;
Review Comment:
This stub and override only allow the template to compile; no assertion in
this rendered spec references the panel, its `[hidden]` behavior, `inert`, or
the three false input bindings. A regression that removes the read-only marker
or mounts the editor conditionally would still pass. Add rendered assertions
for initial mounting/hidden state, selection visibility, inertness, and all
bound read-only inputs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
