yangzhang75 commented on code in PR #9016: URL: https://github.com/apache/texera/pull/9016#discussion_r4309440261
########## frontend/src/app/workspace/component/workflow-form/read-only-card.directive.ts: ########## @@ -0,0 +1,144 @@ +/** + * 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. + */ + +import { AfterViewInit, Directive, ElementRef, Input, NgZone, OnChanges, OnDestroy } from "@angular/core"; + +/** What the browser lets a user reach with Tab, before this directive takes it out of the order. */ +const FOCUSABLE = + "a[href], area[href], button, input, select, textarea, iframe, object, embed, summary, " + + "audio[controls], video[controls], [contenteditable]:not([contenteditable='false']), [tabindex]"; + +/** Where an element's own tabindex is kept while it is locked, so unlocking gives it back. */ +const ORIGINAL_TABINDEX = new WeakMap<Element, string | null>(); + +/** + * Keeps the keyboard out of a read-only input card, without hiding what it shows. + * + * A read-only card is locked with `pointer-events: none`, which is hit testing only: a keyboard user + * could Tab into the controls a custom widget draws of its own -- a file picker's Browse button, the + * HuggingFace file input -- and activate them with Enter or Space, and those controls do not consult + * the form's disabled state the way the bound formly controls do (#8943). This takes every focusable + * descendant out of the tab order and marks it `aria-disabled`, so there is nothing to reach and + * assistive tech says why. + * + * `inert` would be shorter, and the step panel uses it for its read-only mount, but it also removes + * the subtree from the accessibility tree. A reader's card exists to be read -- the values are what + * a reader runs the workflow with -- so hiding it from a screen reader would trade this bug for a + * worse one. Disabled controls stay in the accessibility tree and keep being announced. + * + * Widgets render their controls after the card is built and may redraw them at any time, so the + * locking is re-applied whenever the card's content changes -- a control added, or one whose own + * tabindex is written back. + */ +@Directive({ + selector: "[texeraReadOnlyCard]", +}) +export class ReadOnlyCardDirective implements OnChanges, AfterViewInit, OnDestroy { + /** Whether this card is read-only; the directive does nothing while it is false. */ + @Input("texeraReadOnlyCard") locked = false; + + private observer?: MutationObserver; + /** The elements this directive took out of the tab order, so it only gives back what it took. */ + private readonly lockedElements = new Set<Element>(); + + constructor( + private host: ElementRef<HTMLElement>, + private zone: NgZone + ) {} + + ngOnChanges(): void { + this.apply(); + } + + ngAfterViewInit(): void { + this.apply(); + } + + ngOnDestroy(): void { + this.stopWatching(); + } + + private apply(): void { + if (this.locked) { + this.lockDescendants(); + this.startWatching(); + } else { + this.stopWatching(); + this.unlockDescendants(); + } + } + + private lockDescendants(): void { + for (const element of Array.from(this.host.nativeElement.querySelectorAll(FOCUSABLE))) { Review Comment: Right, and it is worse than a one-off: a widget redraws its own controls while the card lives on (the page keeps a card across rebuilds, by binding id), so the set only grew. `lockDescendants()` now releases first: anything the host no longer contains is given back its tabindex and its disabled state and dropped from the set. Released rather than merely dropped, so a control that leaves and comes back is remembered from how it is then, not from the locked state this directive left on it. Test as asked: five add/remove rounds, asserting the tracked count comes back to where it started. Deletion-checked -- removing the release turns it red. ########## frontend/src/app/workspace/component/workflow-form/read-only-card.directive.ts: ########## @@ -0,0 +1,144 @@ +/** + * 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. + */ + +import { AfterViewInit, Directive, ElementRef, Input, NgZone, OnChanges, OnDestroy } from "@angular/core"; + +/** What the browser lets a user reach with Tab, before this directive takes it out of the order. */ +const FOCUSABLE = + "a[href], area[href], button, input, select, textarea, iframe, object, embed, summary, " + + "audio[controls], video[controls], [contenteditable]:not([contenteditable='false']), [tabindex]"; + +/** Where an element's own tabindex is kept while it is locked, so unlocking gives it back. */ +const ORIGINAL_TABINDEX = new WeakMap<Element, string | null>(); + +/** + * Keeps the keyboard out of a read-only input card, without hiding what it shows. + * + * A read-only card is locked with `pointer-events: none`, which is hit testing only: a keyboard user + * could Tab into the controls a custom widget draws of its own -- a file picker's Browse button, the + * HuggingFace file input -- and activate them with Enter or Space, and those controls do not consult + * the form's disabled state the way the bound formly controls do (#8943). This takes every focusable + * descendant out of the tab order and marks it `aria-disabled`, so there is nothing to reach and + * assistive tech says why. + * + * `inert` would be shorter, and the step panel uses it for its read-only mount, but it also removes + * the subtree from the accessibility tree. A reader's card exists to be read -- the values are what + * a reader runs the workflow with -- so hiding it from a screen reader would trade this bug for a + * worse one. Disabled controls stay in the accessibility tree and keep being announced. + * + * Widgets render their controls after the card is built and may redraw them at any time, so the + * locking is re-applied whenever the card's content changes -- a control added, or one whose own + * tabindex is written back. + */ +@Directive({ + selector: "[texeraReadOnlyCard]", +}) +export class ReadOnlyCardDirective implements OnChanges, AfterViewInit, OnDestroy { + /** Whether this card is read-only; the directive does nothing while it is false. */ + @Input("texeraReadOnlyCard") locked = false; + + private observer?: MutationObserver; + /** The elements this directive took out of the tab order, so it only gives back what it took. */ + private readonly lockedElements = new Set<Element>(); + + constructor( + private host: ElementRef<HTMLElement>, + private zone: NgZone + ) {} + + ngOnChanges(): void { + this.apply(); + } + + ngAfterViewInit(): void { + this.apply(); + } + + ngOnDestroy(): void { + this.stopWatching(); + } + + private apply(): void { + if (this.locked) { + this.lockDescendants(); + this.startWatching(); + } else { + this.stopWatching(); + this.unlockDescendants(); + } + } + + private lockDescendants(): void { + for (const element of Array.from(this.host.nativeElement.querySelectorAll(FOCUSABLE))) { + if (!this.lockedElements.has(element)) { + ORIGINAL_TABINDEX.set(element, element.getAttribute("tabindex")); + this.lockedElements.add(element); + } + // Set only what is not already set: this runs again for every change inside the card, and a + // write of its own would be one more change. A control already locked is skipped, so the + // attribute observer below settles after one pass; one that a widget put back in the tab + // order is locked again. + if (element.getAttribute("tabindex") !== "-1") { + element.setAttribute("tabindex", "-1"); + } + if (element.getAttribute("aria-disabled") !== "true") { + element.setAttribute("aria-disabled", "true"); Review Comment: Agreed, and this is the one that matters: the HuggingFace audio input posts the chosen file to `/api/huggingface/upload-audio`, whose resource is `@RolesAllowed("REGULAR","ADMIN")` with no workflow-access check, so a reader reaching it really does write a file on the server. I reproduced that on a workflow shared read-only before fixing it. The card-level lock now disables natively too: every button/input/select/textarea under a reader's card gets `disabled`, its previous state remembered and given back when the card becomes editable, and the observer watches the `disabled` attribute as well so a widget that re-enables its own control (the audio uploader's `[disabled]="isUploading"`) is locked again. Elements that cannot be disabled natively (a link) keep the tab-order lock. Nothing is made `inert`, so the card stays readable. Tests: a click on a locked control dispatches no activation and both controls report disabled; the state is given back on unlock; a control the widget re-enables is locked again. jsdom does not model a disabled control refusing focus, so that half is asserted through the property the browser acts on rather than through `document.activeElement`. Per-widget guards inside the uploaders' own handlers, and the endpoint's missing access check, are still worth their own issues -- the card-level lock covers every widget at once, including ones added later, but it cannot close a request posted by hand. Pushed as d5eb98e0c. -- 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]
