Skip to content

fix(workflow-form): keep the keyboard out of a read-only input card, not only the pointer - #9016

Merged
mengw15 merged 1 commit into
apache:mainfrom
yangzhang75:fix/form-readonly-card-keyboard
Oct 19, 2026
Merged

mengw15 merged 1 commit into
apache:mainfrom
yangzhang75:fix/form-readonly-card-keyboard

Conversation

@yangzhang75

@yangzhang75 yangzhang75 commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Closes #8943. Part of the Form View feature (#8011); from the #8439 refinements (item 5).

A read-only input card -- what a viewer without write access sees -- is locked with pointer-events: none. That is hit testing only. The bound formly controls are disabled through props.disabled, but the controls a custom widget draws of its own are not: the HuggingFace audio and image uploaders each render a plain file input that nothing disables. A reader could Tab into one inside a locked card, open it with Enter and choose a file.

For the audio input that is a real write. The widget POSTs the chosen file to /api/huggingface/upload-audio, which authorises any signed-in user and never asks who may write this workflow, so the file lands in the server's temp store. Checked on a workflow shared read-only: two presses of Tab reach the uploader inside the locked card, Enter opens the file dialog, and the POST comes back 200 with the stored path. The image uploader is client-side, so there the reader only makes the card display a picture of their choosing -- a read-only card wires no write-back and a reader's save is refused, so the operator keeps the author's value and a run still runs it.

The card now also keeps the keyboard out, through a small directive on it (texeraReadOnlyCard, applied as [texeraReadOnlyCard]="!canEdit" beside the existing read-only class):

  • Every focusable descendant is taken out of the tab order (tabindex="-1", its own value remembered) and marked aria-disabled="true", so there is nothing for Tab to reach and assistive tech says why rather than going quiet.
  • Every control the browser will refuse outright -- button, input, select, textarea -- is also disabled natively, its previous state remembered. Out of the tab order is not out of reach: tabindex="-1" ends only sequential navigation and aria-disabled only announces, so a reader's assistive tech could still focus a widget's file input and activate it, which for the audio one posts the file (Copilot).
  • A card that becomes editable gets its controls back, each with the tabindex and the disabled state it had, and only those this directive took.
  • A control that leaves the card is let go of on the next pass: a widget redraws its own controls while the card lives on, so a set that only grew held every detached node for as long as the reader stayed on the page (Copilot).
  • Widgets render their own controls after the card is built and redraw them as they go, so the locking is re-applied on every change inside the card: a control added, one whose own tabindex is written back (ng-zorro writes one when it enables a control), and one a widget enables again (the audio uploader's [disabled]="isUploading"). The observer runs outside Angular and sets only what is not already set, so one pass settles it.

inert is what the step panel uses for its read-only mount, and the issue suggested following it; this does not, deliberately. inert also removes the subtree from the accessibility tree, and a reader's card exists to be read -- the values are what a reader runs the workflow with -- so that would trade this bug for a worse one. Disabled controls stay in the accessibility tree and keep being announced. The directive's doc comment says so, next to the code.

Nothing else changes: the pointer lock, the styles, the card's content and the author's editing path are untouched, and the directive does nothing at all while a card is editable.

Any related issues, documentation, discussions?

Closes #8943, which re-files #8758 (closed). Part of #8439 (item 5) and the Form View feature #8011. Raised by Copilot on #8437, #8438 and #8440.

Three neighbouring things are deliberately left alone. The widgets' own controls still do not consult the field's disabled state in their own handlers -- fixing that is per-widget work (the HuggingFace uploaders, the dataset picker, the resource value selector), and the card-level lock covers every widget at once, including ones added later. The upload endpoint itself authorises on being signed in and nothing else (@RolesAllowed("REGULAR","ADMIN"), no workflow-access check), so it would still accept a file posted by hand; closing the UI route does not close that one, and it is worth its own issue. And the step panel's inert mount has the same accessibility-tree problem described above; changing it is not this issue's scope, and it is worth its own look.

How was this PR tested?

Unit tests (vitest). The directive's own spec, on a host shaped like a card (a bound input, a widget's Browse button, a link, a span, a control with its own tabindex): every focusable control in a read-only card is out of the tab order; each is marked aria-disabled while the card is not inert and its text is still there; an editable card is left alone, keeping a control's own tabindex="3"; a card that becomes editable gets that 3 back, the untouched control gets no tabindex, and the aria-disabled marks go; a control a widget adds after the card was built is locked; a control whose widget writes its tabindex back, or enables again, is locked again; a click on a locked control dispatches no activation and the controls report disabled (jsdom does not model a disabled control refusing focus, so that half is asserted through the property the browser acts on); the disabled state is given back on unlock; five rounds of a control being added and removed leave the tracked set where it started, rather than holding the detached nodes; the observer is disconnected when the card goes away. The page's rendered spec wires it through the real template: a read-only viewer's card, with a button appended the way a widget renders one, has it out of the tab order and marked, and the card is not inert.

By hand, on two dev builds of the same base commit, one with this change and one without, signed in as a second account the workflow is shared read-only with. Before: from the view toggle, two presses of Tab land on the uploader inside the locked card, Enter opens the file dialog, and choosing a file POSTs it (200). After: the same two presses land on Run, the card is skipped, and no request goes out. The mouse reaches nothing in either build, and the stored workflow is unchanged in both.

Each guard was deletion-checked and turned a named test red (the directive not applied; the tabindex write; the aria-disabled write; the native disable; the editable check; the unlock; the original-tabindex restore; the original-disabled restore; the release of what has left the card; the childList and both attribute halves of the observer; the disconnect). eslint, prettier and the production (AOT) build pass; every statement, function and branch this PR adds is covered, and the full frontend suite passes.

Video

Before

Screen.Recording.2026-10-18.at.4.10.47.PM.mov

After

Screen.Recording.2026-10-18.at.4.11.17.PM.mov

Was this PR authored or co-authored using generative AI tooling?

Yes. Generated-by: Claude Code (Anthropic). Co-authored with Claude, reviewed line by line by the author before submission.

🤖 Generated with Claude Code

@github-actions github-actions Bot added fix frontend Changes related to the frontend GUI labels Oct 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @mengw15
    You can notify them by mentioning @mengw15 in a comment.

@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. A release/* label nominates a backport target; the branch's release manager approving this PR is what sends the fix there. The required Backport Approvals check stays red until every label below is approved, so each manager either approves or removes their own label — which is why the labels left on a merged PR are exactly the branches it reached.

Release branch Analysis
⚠️ release/v1.3 Not labeled automatically — none of the files this PR modifies exist on this branch (frontend/src/app/workspace/component/workflow-form/workflow-form.component.html, frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts, frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts). The fix may target code that isn't on this release, or the files were moved/renamed after the branch was cut. Please check and add release/v1.3 by hand if this fix should be backported here — its release manager then has to approve before this PR can merge.
⚠️ release/v1.2 Not labeled automatically — none of the files this PR modifies exist on this branch (frontend/src/app/workspace/component/workflow-form/workflow-form.component.html, frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts, frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts). The fix may target code that isn't on this release, or the files were moved/renamed after the branch was cut. Please check and add release/v1.2 by hand if this fix should be backported here — its release manager then has to approve before this PR can merge.

Auto-label run.

@codecov-commenter

codecov-commenter commented Oct 17, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.33%. Comparing base (3726dd7) to head (d5eb98e).

Files with missing lines Patch % Lines
...omponent/workflow-form/read-only-card.directive.ts 98.07% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #9016   +/-   ##
=========================================
  Coverage     92.33%   92.33%           
  Complexity     5045     5045           
=========================================
  Files          1260     1261    +1     
  Lines         54067    54120   +53     
  Branches       6755     6768   +13     
=========================================
+ Hits          49921    49972   +51     
  Misses         2486     2486           
- Partials       1660     1662    +2     
Flag Coverage Δ *Carryforward flag
access-control-service 77.38% <ø> (ø) Carriedforward from 3726dd7
agent-service 99.16% <ø> (ø) Carriedforward from 3726dd7
amber 87.31% <ø> (ø) Carriedforward from 3726dd7
computing-unit-managing-service 60.48% <ø> (ø) Carriedforward from 3726dd7
config-service 87.37% <ø> (ø) Carriedforward from 3726dd7
file-service 82.00% <ø> (ø) Carriedforward from 3726dd7
frontend 96.55% <98.14%> (-0.01%) ⬇️
notebook-migration-service 83.73% <ø> (ø) Carriedforward from 3726dd7
pyamber 98.57% <ø> (ø) Carriedforward from 3726dd7
workflow-compiling-service 72.43% <ø> (ø) Carriedforward from 3726dd7

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yangzhang75
yangzhang75 force-pushed the fix/form-readonly-card-keyboard branch from a963b0f to ef10167 Compare October 19, 2026 00:01
@yangzhang75
yangzhang75 marked this pull request as ready for review October 19, 2026 00:13
@yangzhang75

Copy link
Copy Markdown
Contributor Author

/request-review: @mengw15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The lock still permits activation outside Tab navigation, and widget redraws retain detached controls.

2 open issues
What changed in this PR

This PR strengthens Form View’s read-only input cards by blocking Tab navigation without hiding their contents from assistive technology.

Changes:

  • Adds a directive that manages descendant tab order and accessibility attributes.
  • Applies the directive to read-only input cards.
  • Adds directive and rendered-template regression tests.
File Description
frontend/​src/​app/​workspace/​component/​workflow-form/​workflow-form.rendered.spec.ts Tests directive integration in the real template.
frontend/​src/​app/​workspace/​component/​workflow-form/​workflow-form.component.ts Imports the directive.
frontend/​src/​app/​workspace/​component/​workflow-form/​workflow-form.component.html Binds the lock to editing permissions.
frontend/​src/​app/​workspace/​component/​workflow-form/​read-only-card.directive.ts Manages locking, restoration, and DOM observation.
frontend/​src/​app/​workspace/​component/​workflow-form/​read-only-card.directive.spec.ts Tests locking, restoration, and observer lifecycle.

Review effort: Balanced

…not only the pointer

The 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; those controls do not consult the form's disabled state the
way the bound formly controls do, so the upload went out from a read-only card.

A directive on the card takes every focusable descendant out of the tab order
and marks it aria-disabled, re-applying when a widget renders or redraws its own
controls. `inert`, which the step panel uses for its read-only mount, would also
remove the card from the accessibility tree: a reader's card exists to be read,
so that would trade this bug for a worse one.
@yangzhang75
yangzhang75 force-pushed the fix/form-readonly-card-keyboard branch from ef10167 to d5eb98e Compare October 19, 2026 00:32
@mengw15
mengw15 requested a balanced review from Copilot October 19, 2026 00:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved blocking issues were identified, and regression coverage addresses the interaction gap and directive lifecycle.

No open issues 🎉

2 resolved since last review

Review effort: Balanced

@mengw15 mengw15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mengw15
mengw15 enabled auto-merge October 19, 2026 00:41
@mengw15
mengw15 added this pull request to the merge queue Oct 19, 2026
Merged via the queue into apache:main with commit a46ddc6 Oct 19, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix frontend Changes related to the frontend GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(workflow-form): the read-only input card blocks the pointer but not the keyboard

4 participants