yangzhang75 commented on code in PR #8517: URL: https://github.com/apache/texera/pull/8517#discussion_r3997296068
########## frontend/src/app/common/formly/editable-label-wrapper/editable-label-wrapper.component.html: ########## @@ -0,0 +1,75 @@ +<!-- + 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. Written through on + every keystroke (input, not change): the hidden label below and the saved name follow the box as + it is typed, so nothing is lost if the page is left while the box still has the focus. --> +<div + class="lbl-row" + *ngIf="props.authoring"> + <input + class="lbl-input" + [value]="props.authorName" + [placeholder]="props.schemaLabel" + (input)="onRename($event)" + [attr.aria-label]="'Label shown above this input'" + [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. --> + <!-- A toggle button: the name stays constant and aria-pressed carries the state (a name that + changed with the state would announce the opposite of what the state says). --> + <button + *ngIf="props.canHide !== false" + type="button" + class="lbl-eye" + [class.off]="props.authorHidden" + (click)="onToggleHidden()" + [attr.aria-pressed]="props.authorHidden === true" + aria-label="Hide from the form" + [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> + +<!-- The name box above edits the label; it is not the label. The control below still needs a + programmatic name (decorate blanks formly's own), so it gets one that follows the current name, + present for assistive technology only. --> +<label + class="lbl-sr-only" + *ngIf="props.authoring" + [attr.for]="id"> + {{ props.authorName || props.schemaLabel }} +</label> + +<!-- Everyone else just reads it. This wrapper blanks formly's own label (decorate sets label to ""), + so this one has to do that label's job in full: `for` ties it to the control (formly gives the + control the field id), or the reader's input would show a name and announce none. --> +<label + class="lbl-static" + *ngIf="!props.authoring && (props.authorName || props.schemaLabel)" + [attr.for]="id"> + {{ props.authorName || props.schemaLabel }} Review Comment: Right, the array widget carries no labelable control with the field id. 0f1254be4: a repeated field is decorated with group: true, and the wrapper then renders its title as a span (id + '-label') and gives the rows role="group" with aria-labelledby, in reader mode and while authoring alike; scalar and object fields keep label[for]. Wrapper spec covers both, and the decorate flag. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -902,19 +925,59 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { * stored per-sub-field overrides (rename, hide), keyed by field path. A repeated section builds * its row template on demand, so its builder is wrapped to decorate every row formly ever makes. */ - private applyFieldOverrides(field: FormlyFieldConfig, binding: FormFieldBinding): void { + private applyFieldOverrides(field: FormlyFieldConfig, binding: FormFieldBinding, schemaLabel: string): void { const walk = (node: FormlyFieldConfig, path: string): void => { // Drop the schema's own description on every field, nested ones included: on this page the // one piece of guidance is the help text the form's author writes, rendered once by the card. node.props = { ...(node.props ?? {}), description: "" }; + // Author mode, the input itself (root path): its name is renamed in place by clicking the + // title, like every nested field. No eye here -- a whole input leaves via Remove, not a hide + // toggle. The editable label becomes the single title, so formly's own label is cleared to + // avoid printing it twice. + if (!path && this.authoring) { + EditableLabelWrapperComponent.decorate( + node, + { authoring: true, name: binding.displayName ?? "", hidden: false, fallback: schemaLabel, canHide: false }, + name => this.onBindingNamed(binding.id, name) + ); + node.props = { ...(node.props ?? {}), label: "" }; + } else if (!path && node.type === "array") { Review Comment: Confirmed: the leaf rows of a root scalar array were walked at the root's empty path and picked up the binding-level title box. 0f1254be4 passes an explicit root flag through the walk: only the input's own node is decorated as the root; rows (scalar and object) are walked as rows. Spec: a scalar array's rows carry no wrapper, the input has the one title box. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -902,19 +925,59 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { * stored per-sub-field overrides (rename, hide), keyed by field path. A repeated section builds * its row template on demand, so its builder is wrapped to decorate every row formly ever makes. */ - private applyFieldOverrides(field: FormlyFieldConfig, binding: FormFieldBinding): void { + private applyFieldOverrides(field: FormlyFieldConfig, binding: FormFieldBinding, schemaLabel: string): void { const walk = (node: FormlyFieldConfig, path: string): void => { // Drop the schema's own description on every field, nested ones included: on this page the // one piece of guidance is the help text the form's author writes, rendered once by the card. node.props = { ...(node.props ?? {}), description: "" }; + // Author mode, the input itself (root path): its name is renamed in place by clicking the + // title, like every nested field. No eye here -- a whole input leaves via Remove, not a hide + // toggle. The editable label becomes the single title, so formly's own label is cleared to + // avoid printing it twice. + if (!path && this.authoring) { + EditableLabelWrapperComponent.decorate( + node, + { authoring: true, name: binding.displayName ?? "", hidden: false, fallback: schemaLabel, canHide: false }, + name => this.onBindingNamed(binding.id, name) + ); + node.props = { ...(node.props ?? {}), label: "" }; + } else if (!path && node.type === "array") { + // Reader mode, a repeated input: the shared array widget prints its label at the BOTTOM, + // beside its add button (the canvas panel's convention), while every other widget and the + // author's editable title sit above. Left alone, the title would jump from above the rows + // in edit mode to below them on Done. Give it the same static title above instead; the + // wrapper blanks the widget's own label. + EditableLabelWrapperComponent.decorate(node, { + authoring: false, + name: binding.displayName ?? "", + hidden: false, + fallback: schemaLabel, + canHide: false, + }); + } // Apply the author's stored overrides so a reader sees each sub-field renamed and hidden as // set up. The root (path "") carries the binding's own displayName, set in renderField. if (path) { const override = binding.overrides?.[path] ?? {}; if (override.displayName) { node.props = { ...(node.props ?? {}), label: override.displayName }; } - if (override.hidden) { + if (this.authoring) { + // An author edits the sub-field's label where it appears and keeps hidden fields on + // screen (faded, via the wrapper) so they can be brought back, rather than removed from + // the DOM as they are for a reader. + EditableLabelWrapperComponent.decorate( + node, + { + authoring: true, + name: override.displayName ?? "", + hidden: override.hidden === true, + fallback: (node.props?.label as string) || path, Review Comment: Yes, the fallback was read after the override had replaced the label. 0f1254be4 captures the schema's own label before the override is applied and hands that to the wrapper as schemaLabel, so the placeholder/tooltip and what clearing the box yields agree. Spec covers an already renamed sub-field. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -1241,6 +1380,24 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { } } + /** + * Renaming the input itself, from its own title. Like the help text, a name or a hide flag is + * presentation only: the wrapper that took the edit already shows it, so the form is NOT rebuilt + * here. A rebuild would replace the very control the author is on (the name box, the eye) and drop + * the keyboard focus with it; the stored override is applied on the next full re-read (Done). + */ + private onBindingNamed(bindingId: string, value: string): void { + this.formBindingService.updateBinding(bindingId, { displayName: value }); + } + + private onSubFieldNamed(bindingId: string, path: string, value: string): void { + this.formBindingService.setFieldOverride(bindingId, path, { displayName: value }); + } + + private onSubFieldHiddenAt(bindingId: string, path: string, hidden: boolean): void { + this.formBindingService.setFieldOverride(bindingId, path, { hidden }); Review Comment: Confirmed, and the harness never emitted so the specs could not see it. 0f1254be4: presentation writes (name, hide flag, help text, and the picker/remove/reorder callers that re-read themselves) run under a self-reflected mark and the formBindingChanged$ subscriber skips the rebuild while it is set; the announcement still reaches the autosave, and structural announcements (expose from the panel) rebuild as before. The harness's form-binding mock now emits like the real service, and the spec drives the eye, the name box and help text through it: no rebuild; then a structural announcement: one rebuild. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -1241,6 +1380,24 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { } } + /** + * Renaming the input itself, from its own title. Like the help text, a name or a hide flag is + * presentation only: the wrapper that took the edit already shows it, so the form is NOT rebuilt + * here. A rebuild would replace the very control the author is on (the name box, the eye) and drop + * the keyboard focus with it; the stored override is applied on the next full re-read (Done). + */ + private onBindingNamed(bindingId: string, value: string): void { + this.formBindingService.updateBinding(bindingId, { displayName: value }); + } + + private onSubFieldNamed(bindingId: string, path: string, value: string): void { + this.formBindingService.setFieldOverride(bindingId, path, { displayName: value }); + } + + private onSubFieldHiddenAt(bindingId: string, path: string, hidden: boolean): void { Review Comment: Exactly that -- the chain was severed at the harness. 0f1254be4 takes your direction: the presentation sinks (name, hide, help text) are marked as self-reflected and the subscriber skips the rebuild while the mark is set; structural writes keep the immediate rebuild (the callers that re-read themselves, remove/reorder/result pick, also mark theirs so they rebuild once, not twice). The harness's form-binding mock now announces on formBindingChanged$ like setFormBinding does, so the spec drives the eye through the real path and asserts no rebuild, then a structural announcement and one rebuild. Deleting the skip turns six specs red. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -902,19 +925,59 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { * stored per-sub-field overrides (rename, hide), keyed by field path. A repeated section builds * its row template on demand, so its builder is wrapped to decorate every row formly ever makes. */ - private applyFieldOverrides(field: FormlyFieldConfig, binding: FormFieldBinding): void { + private applyFieldOverrides(field: FormlyFieldConfig, binding: FormFieldBinding, schemaLabel: string): void { const walk = (node: FormlyFieldConfig, path: string): void => { // Drop the schema's own description on every field, nested ones included: on this page the // one piece of guidance is the help text the form's author writes, rendered once by the card. node.props = { ...(node.props ?? {}), description: "" }; + // Author mode, the input itself (root path): its name is renamed in place by clicking the + // title, like every nested field. No eye here -- a whole input leaves via Remove, not a hide + // toggle. The editable label becomes the single title, so formly's own label is cleared to + // avoid printing it twice. + if (!path && this.authoring) { + EditableLabelWrapperComponent.decorate( + node, + { authoring: true, name: binding.displayName ?? "", hidden: false, fallback: schemaLabel, canHide: false }, + name => this.onBindingNamed(binding.id, name) + ); + node.props = { ...(node.props ?? {}), label: "" }; + } else if (!path && node.type === "array") { + // Reader mode, a repeated input: the shared array widget prints its label at the BOTTOM, + // beside its add button (the canvas panel's convention), while every other widget and the + // author's editable title sit above. Left alone, the title would jump from above the rows + // in edit mode to below them on Done. Give it the same static title above instead; the + // wrapper blanks the widget's own label. + EditableLabelWrapperComponent.decorate(node, { + authoring: false, + name: binding.displayName ?? "", + hidden: false, + fallback: schemaLabel, + canHide: false, + }); + } // Apply the author's stored overrides so a reader sees each sub-field renamed and hidden as // set up. The root (path "") carries the binding's own displayName, set in renderField. if (path) { const override = binding.overrides?.[path] ?? {}; if (override.displayName) { node.props = { ...(node.props ?? {}), label: override.displayName }; } - if (override.hidden) { + if (this.authoring) { + // An author edits the sub-field's label where it appears and keeps hidden fields on + // screen (faded, via the wrapper) so they can be brought back, rather than removed from + // the DOM as they are for a reader. + EditableLabelWrapperComponent.decorate( + node, + { + authoring: true, + name: override.displayName ?? "", + hidden: override.hidden === true, + fallback: (node.props?.label as string) || path, Review Comment: Yes: 0f1254be4 captures the schema's own label before the override replaces it and passes that as the fallback, so the placeholder and the tooltip promise what clearing the box really yields. Spec: an already renamed sub-field keeps the schema label as its fallback. -- 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]
