Copilot commented on code in PR #8318: URL: https://github.com/apache/texera/pull/8318#discussion_r3919482684
########## frontend/src/app/common/formly/editable-label-wrapper/editable-label-wrapper.component.html: ########## @@ -0,0 +1,55 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +<!-- Authoring: the label is the input, so what you type is what the reader reads. --> +<div + class="lbl-row" + *ngIf="props.authoring"> + <input + class="lbl-input" + [value]="props.authorName" + [placeholder]="props.schemaLabel" + (change)="onRename($event)" + [title]="'Shown above this box. Empty keeps ' + props.schemaLabel" /> + <!-- An input the reader can only lose by removing it altogether has no eye: offering + one here would be a second place to decide the same thing. --> + <button + *ngIf="props.canHide !== false" + type="button" + class="lbl-eye" + [class.off]="props.authorHidden" + (click)="onToggleHidden()" + [title]="props.authorHidden ? 'Hidden from the form' : 'Shown on the form'"> + <i + nz-icon + [nzType]="props.authorHidden ? 'eye-invisible' : 'eye'" + nzTheme="outline"></i> + </button> +</div> + +<!-- Everyone else just reads it. --> +<label + class="lbl-static" + *ngIf="!props.authoring && (props.authorName || props.schemaLabel)"> + {{ props.authorName || props.schemaLabel }} Review Comment: Blanking Formly's normal label and rendering this standalone `<label>` removes the control association because it has no `for` attribute. In reader mode, assistive technology will therefore announce the generated input without its author-defined/schema label. Associate this label with Formly's generated field id. ########## frontend/src/app/common/formly/formly-config.ts: ########## @@ -92,6 +94,8 @@ export const TEXERA_FORMLY_CONFIG = { wrappers: [ { name: "preset-wrapper", component: PresetWrapperComponent }, { name: "collab-wrapper", component: CollabWrapperComponent }, + { name: "expose-property-wrapper", component: ExposePropertyWrapperComponent }, + { name: "editable-label-wrapper", component: EditableLabelWrapperComponent }, Review Comment: Registering this wrapper only makes its name available to Formly; it does not apply it to any field. There is no production call to `EditableLabelWrapperComponent.decorate` (the only calls are in its spec), so the PR's advertised rename/hide UI can never appear. Wire the wrapper into the Form View field-generation path with callbacks to `setFieldOverride`, or defer the claimed behavior and component to the PR that does so. ########## frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts: ########## @@ -1341,13 +1353,28 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On }; } + // A tick box beside each TOP-LEVEL property only. The map callback runs for every + // field at every depth, so without this an array-of-objects property sprouted one on + // the array, each item and each nested field. Membership in the schema's own + // `properties` distinguishes a top-level field from a same-named nested one. + const isTopLevel = typeof mappedField.key === "string" && rootPropertyNames.has(mappedField.key); Review Comment: The new frame test covers only a flat, single-property schema, so it does not verify the key behavior introduced here: nested array/object fields must not receive expose toggles. Add a regression schema with nested fields (including a nested key that matches a root property name), assert only root fields are decorated, and adjust the depth check if that case fails. ########## frontend/src/app/workspace/component/property-editor/property-editor.component.ts: ########## @@ -74,40 +83,169 @@ import { NzButtonComponent } from "ng-zorro-antd/button"; NzResizeHandlesComponent, ], }) -export class PropertyEditorComponent implements OnInit, OnDestroy { +export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { @ViewChild("contentWrapper") contentWrapperRef!: ElementRef; protected readonly window = window; id = -1; - width = 260; + width = MIN_PANEL_WIDTH; height = Math.max(300, window.innerHeight * 0.6); currentComponent: Type<any> | null = null; + /** + * Set while an author is choosing which properties the Form View offers. + * Forwarded to the operator frame, which puts a tick box beside each property. + */ + @Input() exposeChoosing = false; + /** + * Whether this panel owns the canvas panel's saved size/position. The Form View mounts + * this same component inline in a preview box; only the docked canvas panel persists, so + * the preview copy must not overwrite the shared geometry keys. + */ + @Input() persistPlacement = true; + /** Set from the toolbar toggle on the operator canvas; the input covers the form view. */ + private choosingFromToolbar = false; + + /** The choose-what-to-expose affordance appears wherever the feature flag is on: any + * workflow can expose inputs to its Form View, independent of the default-view bit. */ + public get offersFormView(): boolean { + return this.config.env.formViewEnabled; + } + + public toggleChoosing(): void { + this.formBindingService.setChoosing(!this.formBindingService.isChoosing()); + } + + public get choosing(): boolean { + return this.exposeChoosing || this.choosingFromToolbar; + } componentInputs = {}; dragPosition = { x: 0, y: 0 }; - returnPosition = { x: 0, y: 0 }; constructor( public workflowActionService: WorkflowActionService, private changeDetectorRef: ChangeDetectorRef, - private panelService: PanelService + private panelService: PanelService, + private formBindingService: FormBindingService, + private config: GuiConfigService ) { - const width = localStorage.getItem("right-panel-width"); - if (width) this.width = Number(width); + // A stored "0" is a truthy string, so a panel that was closed before a reload used + // to come back closed on every load afterwards -- and the button that reopens it is + // itself hidden until an operator is selected, so the panel simply looked broken. + // Anything narrower than the resize minimum is treated as no stored width at all. + const storedWidth = Number(localStorage.getItem("right-panel-width")); + if (storedWidth >= MIN_PANEL_WIDTH) this.width = storedWidth; this.height = Number(localStorage.getItem("right-panel-height")) || this.height; Review Comment: A closed panel persists `height = 65` in `ngOnDestroy`. Although the new width check rejects the corresponding stored width of zero, this line still restores 65px, so the panel reopens below its declared 300px minimum. Validate the stored height before applying it (and add the height assertion to the closed-panel regression test). ########## frontend/src/app/workspace/component/property-editor/property-editor.component.html: ########## @@ -60,6 +60,23 @@ nz-menu id="property-buttons" [ngClass]="{'shadow': !width}"> + <!-- Choosing which settings the form offers happens by ticking them in this very + panel, so the switch that turns those tick boxes on belongs here rather than in + a row of file and delete icons across the toolbar. Only offered on a workflow + whose author turned the Form View on. --> + <button + nz-button + [nzType]="choosing ? 'primary' : 'text'" + class="choose-fields" + (click)="toggleChoosing()" + *ngIf="width && offersFormView" Review Comment: This new icon-only toggle has no accessible name; an `nz-tooltip` provides visual help but is not a button label for assistive technology. Add the same state-dependent text as an `aria-label`. ########## frontend/src/app/common/formly/expose-property-wrapper/expose-property-wrapper.component.html: ########## @@ -0,0 +1,32 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +<div class="expose-row choosing"> + <label + class="expose-box" + title="Show this on the Form View"> + <input + type="checkbox" + [checked]="props.exposed" + (change)="onToggle($event)" /> Review Comment: The checkbox has no accessible name. Its wrapping label contains no text, and the label's `title` is not used as the nested input's label, so screen-reader users cannot tell which property this checkbox exposes. Give the input a property-specific `aria-label`. ########## frontend/src/app/workspace/component/property-editor/property-editor.component.ts: ########## @@ -74,40 +83,169 @@ import { NzButtonComponent } from "ng-zorro-antd/button"; NzResizeHandlesComponent, ], }) -export class PropertyEditorComponent implements OnInit, OnDestroy { +export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { @ViewChild("contentWrapper") contentWrapperRef!: ElementRef; protected readonly window = window; id = -1; - width = 260; + width = MIN_PANEL_WIDTH; height = Math.max(300, window.innerHeight * 0.6); currentComponent: Type<any> | null = null; + /** + * Set while an author is choosing which properties the Form View offers. + * Forwarded to the operator frame, which puts a tick box beside each property. + */ + @Input() exposeChoosing = false; + /** + * Whether this panel owns the canvas panel's saved size/position. The Form View mounts + * this same component inline in a preview box; only the docked canvas panel persists, so + * the preview copy must not overwrite the shared geometry keys. + */ + @Input() persistPlacement = true; + /** Set from the toolbar toggle on the operator canvas; the input covers the form view. */ + private choosingFromToolbar = false; + + /** The choose-what-to-expose affordance appears wherever the feature flag is on: any + * workflow can expose inputs to its Form View, independent of the default-view bit. */ + public get offersFormView(): boolean { + return this.config.env.formViewEnabled; + } + + public toggleChoosing(): void { + this.formBindingService.setChoosing(!this.formBindingService.isChoosing()); + } + + public get choosing(): boolean { + return this.exposeChoosing || this.choosingFromToolbar; + } componentInputs = {}; dragPosition = { x: 0, y: 0 }; - returnPosition = { x: 0, y: 0 }; constructor( public workflowActionService: WorkflowActionService, private changeDetectorRef: ChangeDetectorRef, - private panelService: PanelService + private panelService: PanelService, + private formBindingService: FormBindingService, + private config: GuiConfigService ) { - const width = localStorage.getItem("right-panel-width"); - if (width) this.width = Number(width); + // A stored "0" is a truthy string, so a panel that was closed before a reload used + // to come back closed on every load afterwards -- and the button that reopens it is + // itself hidden until an operator is selected, so the panel simply looked broken. + // Anything narrower than the resize minimum is treated as no stored width at all. + const storedWidth = Number(localStorage.getItem("right-panel-width")); + if (storedWidth >= MIN_PANEL_WIDTH) this.width = storedWidth; this.height = Number(localStorage.getItem("right-panel-height")) || this.height; } + /** + * The Form View turns tick boxes on by setting this input, and it flips + * whenever the author enters or leaves edit mode. The frame builds its formly fields + * once, so without remounting here the boxes only appeared if the mode was already on + * when the panel opened -- entering edit mode with a step already selected showed none. + */ + ngOnChanges(changes: SimpleChanges): void { + if (changes["exposeChoosing"] && !changes["exposeChoosing"].firstChange) { + this.remountOperatorFrame(); + } + } + ngOnInit(): void { - const style = localStorage.getItem("right-panel-style"); - if (style) document.getElementById("right-container")!.style.cssText = style; - const translates = document.getElementById("right-container")!.style.transform; - const [xOffset, yOffset, _] = calculateTotalTranslate3d(translates); - this.returnPosition = { x: -xOffset, y: -yOffset }; + if (this.persistPlacement) { + this.restoreSavedPlacement(); + } this.registerHighlightEventsHandler(); + // The toolbar's "choose fields" toggle lives in the service so both the canvas + // toolbar and this panel see the same state. Re-emit the frame's inputs when it + // changes, so tick boxes appear and disappear without needing a re-selection. + this.formBindingService.choosing$.pipe(distinctUntilChanged(), untilDestroyed(this)).subscribe(choosing => { + const wasChoosing = this.choosingFromToolbar; + this.choosingFromToolbar = choosing; + // Only an actual change needs the frame rebuilt. The stream is a BehaviorSubject, + // so it replays its current value on subscribe; remounting for that would tear + // the panel down during the page's first change-detection pass. + if (wasChoosing === choosing || this.currentComponent !== OperatorPropertyEditFrameComponent) { + return; + } + // The frame builds its formly fields once, when it is created, so a new input + // alone would not add or remove the tick boxes -- it has to be remounted. + // + // The restore is in a `finally` and the teardown is not followed by a synchronous + // detectChanges: an exception from an unrelated component (the workspace throws + // NG0100 in dev mode) used to abort this method between the two assignments, + // leaving currentComponent null forever -- and the template hides the whole panel + // on `!currentComponent`, so the property editor silently disappeared. + this.remountOperatorFrame(); + }); this.panelService.closePanelStream.pipe(untilDestroyed(this)).subscribe(() => this.closePanel()); this.panelService.resetPanelStream.pipe(untilDestroyed(this)).subscribe(() => { this.resetPanelPosition(); this.openPanel(); }); } + /** + * Put the panel back where it was last left, unless that is somewhere unreachable. + * + * The saved value is the container's raw cssText, which carries the drag transform + * with it. A panel dragged past the edge of the window therefore came back off-screen + * on every load, and could not be rescued: "reset panels" moved the panel to a home + * position that was itself derived from that very transform, so it put the panel + * straight back where it already was. An out-of-bounds placement is dropped instead. + */ + private restoreSavedPlacement(): void { + const container = document.getElementById("right-container"); + if (!container) { + return; + } + const saved = localStorage.getItem("right-panel-style"); + if (!saved) { + return; + } + // Restore the drag offset and nothing else. The saved value is the container's whole + // style attribute, so it also carried layout properties: the Form View's + // copy of this panel sits inline in a preview box, and it used to save its own + // `position: relative` here, which on the operator canvas dropped the docked panel + // out of the viewport entirely. Width and height have their own keys, and any style + // already poisoned this way is discarded by being ignored. + const transform = /transform:\s*([^;]+)/.exec(saved)?.[1]?.trim(); + if (!transform) { + localStorage.removeItem("right-panel-style"); + return; + } + const [xOffset, yOffset, _] = calculateTotalTranslate3d(transform); + if (this.isOutOfReach(xOffset, yOffset)) { + localStorage.removeItem("right-panel-style"); + return; + } + container.style.transform = transform; + } + + /** True once a drag offset would leave too little of the panel on screen to grab. */ + private isOutOfReach(xOffset: number, yOffset: number): boolean { + const keepVisible = 80; + return ( + Math.abs(xOffset) > Math.max(0, this.window.innerWidth - keepVisible) || + Math.abs(yOffset) > Math.max(0, this.window.innerHeight - keepVisible) + ); Review Comment: These symmetric offset bounds do not match a panel anchored at `right: 0; top: 10vh`. For example, a 260px panel restored with `xOffset = 500` on a 1024px viewport is entirely beyond the right edge, but this check accepts it because `500 < 944`. Account for the panel's home position and dimensions so resized viewports cannot restore an unreachable panel. -- 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]
