fix: add missing mat-label to form fields and remove positive tabinde…#1609
fix: add missing mat-label to form fields and remove positive tabinde…#1609SIDHARTH20K4 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves Angular Material form UI consistency and accessibility by adding missing <mat-label> elements, wrapping some selects in <mat-form-field>, and removing explicit tabindex usage across several modals and components.
Changes:
- Added
<mat-label>to multiplemat-select/matInputcontrols for clearer field labeling. - Removed many explicit
tabindexattributes, relying on natural DOM tab order. - Adjusted markup structure (e.g.,
mat-form-fieldwrapper around “Scroll Range” select) for consistent Material form-field presentation.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/shared/components/settings/app-settings/app-settings.component.html | Adds a visible Material label for the default node selector and removes explicit tab order. |
| src/app/shared/components/horizontal-scroller/horizontal-scroller.component.html | Wraps scroll-range select in mat-form-field, adds labels to selects/date inputs, removes tabindex. |
| src/app/shared/components/data-modal/show-pubkey/show-pubkey.component.html | Adds a Material label to the info-type selector and removes tabindex. |
| src/app/lnd/transactions/create-invoice-modal/create-invoice.component.html | Removes explicit tabindex from dialog controls and adds missing label for the Time Unit select. |
| src/app/lnd/peers-channels/channels/open-channel-modal/open-channel.component.html | Adds label for Transaction Type select and removes explicit tabindex from inputs/toggles/buttons. |
| src/app/lnd/peers-channels/channels/close-channel-modal/close-channel.component.html | Adds label for Transaction Type select and removes explicit tabindex across the form. |
| src/app/lnd/peers-channels/channels/bump-fee-modal/bump-fee.component.html | Adds label for Transaction Type select and removes explicit tabindex across the form. |
| src/app/lnd/on-chain/on-chain-send-modal/on-chain-send-modal.component.html | Adds labels for selects, removes explicit tabindex in both template-driven and reactive stepper flows. |
| src/app/eclair/transactions/create-invoice-modal/create-invoice.component.html | Removes explicit tabindex and adds missing label for the Time Unit select. |
| src/app/eclair/on-chain/on-chain-send-modal/on-chain-send-modal.component.html | Adds label for Amount Unit select and removes explicit tabindex usage. |
| src/app/cln/transactions/invoices/create-invoice-modal/create-invoice.component.html | Removes explicit tabindex and adds missing label for the Time Unit select. |
| src/app/cln/on-chain/on-chain-send-modal/on-chain-send-modal.component.html | Adds labels for selects and removes explicit tabindex across template-driven and stepper forms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </div> | ||
| <div class="mt-2" fxLayout="row"> | ||
| <button mat-button color="primary" tabindex="3" type="default" (click)="onAuthenticate()">Confirm</button> | ||
| <button mat-button color="primary" type="default" (click)="onAuthenticate()">Confirm</button> |
| </div> | ||
| <div class="mt-2" fxLayout="row" fxLayoutAlign="start center" fxFlex="100"> | ||
| <button mat-button color="primary" tabindex="8" type="default" matStepperNext>Next</button> | ||
| <button mat-button color="primary" type="default" matStepperNext>Next</button> |
| <span class="page-title">Create Invoice</span> | ||
| </div> | ||
| <button tabindex="8" fxFlex="5" fxLayoutAlign="center center" class="btn-close-x p-0" default mat-button [mat-dialog-close]="false">X</button> | ||
| <button fxFlex="5" fxLayoutAlign="center center" class="btn-close-x p-0" default mat-button [mat-dialog-close]="false">X</button> |
| <div *ngIf="information.uris && information.uris.length > 0" fxLayout="row"> | ||
| <mat-form-field fxLayout="column" fxFlex="100" fxFlex.gt-sm="40" fxLayoutAlign="start end"> | ||
| <mat-select tabindex="1" [(value)]="selInfoType"> | ||
| <mat-label>Info type</mat-label> |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request cleans up form templates across multiple components by removing explicit tabindex attributes to allow natural DOM tab ordering, adding missing <mat-label> elements, and wrapping select inputs in <mat-form-field> containers. The review feedback highlights two instances of invalid type="default" attributes on buttons inside the CLN on-chain send modal that should be changed to type="button" to avoid unintended form submissions. Additionally, it is recommended to remove obsolete fxFlex layout attributes from the mat-select element in the horizontal scroller component now that it is properly wrapped in a form field.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| </div> | ||
| <div class="mt-2" fxLayout="row"> | ||
| <button mat-button color="primary" tabindex="3" type="default" (click)="onAuthenticate()">Confirm</button> | ||
| <button mat-button color="primary" type="default" (click)="onAuthenticate()">Confirm</button> |
There was a problem hiding this comment.
The button has type="default", which is not a valid HTML button type. Valid values are submit, reset, or button. Since this button is inside a form, an invalid type defaults to submit in most browsers, which can cause unexpected form submissions. It should be changed to type="button".
| <button mat-button color="primary" type="default" (click)="onAuthenticate()">Confirm</button> | |
| <button mat-button color="primary" type="button" (click)="onAuthenticate()">Confirm</button> |
| </div> | ||
| <div class="mt-2" fxLayout="row" fxLayoutAlign="start center" fxFlex="100"> | ||
| <button mat-button color="primary" tabindex="8" type="default" matStepperNext>Next</button> | ||
| <button mat-button color="primary" type="default" matStepperNext>Next</button> |
There was a problem hiding this comment.
The button has type="default", which is not a valid HTML button type. It should be changed to type="button" to prevent unexpected form submission behavior inside the stepper form.
| <button mat-button color="primary" type="default" matStepperNext>Next</button> | |
| <button mat-button color="primary" type="button" matStepperNext>Next</button> |
| </mat-select> | ||
| <mat-form-field fxLayout="column" fxFlex="100" fxFlex.gt-sm="40" fxLayoutAlign="start end"> | ||
| <mat-label>Scroll Range</mat-label> | ||
| <mat-select fxFlex="60" fxFlex.gt-md="30" name="selScrlRange" class="font-bold-700" [(ngModel)]="selScrollRange" (selectionChange)="onRangeChanged($event)"> |
There was a problem hiding this comment.
Now that the mat-select is wrapped inside a mat-form-field, the fxFlex attributes (fxFlex="60" fxFlex.gt-md="30") should be removed from the mat-select itself. In Angular Material, the form control inside a mat-form-field should span the full width of its container, and any flex or sizing layout should be applied to the mat-form-field container instead.
<mat-select name="selScrlRange" class="font-bold-700" [(ngModel)]="selScrollRange" (selectionChange)="onRangeChanged($event)">
…Flex from mat-select
|
/rtl review |
|
✅ Review posted: #1609 (review) 4 finding(s); 4 inline, 0 in body. 🔁 Need a re-review after pushing changes? Reply with |
![rtlreview[bot]](https://avatars.githubusercontent.com/in/3524153?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pure HTML-template accessibility pass: adds mat-label to mat-form-fields that lacked one and strips positive tabindex from form controls and buttons across the LND/CLN/Eclair on-chain-send and create-invoice modals, the LND peers-channels modals (bump-fee, close-channel, open-channel), and three shared components. The actual edits are consistent across the three implementation trees for the modals that were touched, and they don't change submit semantics, password attributes, or template bindings — no security regression.
The gap is scope, not correctness: in three of the shared components the PR explicitly edits (show-pubkey, horizontal-scroller, app-settings), the mat-label was added but additional positive-tabindex attributes on sibling buttons inside the same file were left in place — directly contradicting the second half of the PR description. And the channel modal accessibility work landed only on the LND tree; the CLN peers-channels siblings (which mirror this UX in RTL) are not in this PR, so they likely still carry the same anti-pattern. None of this blocks the change, but the description should match what's shipped.
| <span class="page-title">{{alertTitle}}</span> | ||
| </div> | ||
| <button tabindex="8" fxFlex="5" fxLayoutAlign="center center" class="btn-close-x p-0" mat-button (click)="onClose()">X</button> | ||
| <button fxFlex="5" fxLayoutAlign="center center" class="btn-close-x p-0" mat-button (click)="onClose()">X</button> |
There was a problem hiding this comment.
🟡 F1 (minor): [Multi-impl parity per rules/rtl-multi-impl-parity.md] The channel-modal accessibility fix is applied LND-only — this PR touches src/app/lnd/peers-channels/channels/{open-channel-modal,close-channel-modal,bump-fee-modal}/*.component.html but no file under src/app/cln/peers-channels/. CLN exposes equivalent open-channel / close-channel modals in RTL's standard mirrored layout and the same mat-label-missing + positive-tabindex pattern almost certainly lives there too. The CLN sibling files are not in this PR's file_contents[], so this is unverified rather than asserted drift — worth a human spot-check before merge to decide whether to widen scope or follow up. Bump-fee is LND-only (RBF surface) so that file is not a parity concern; Eclair's channel UX is different so not flagged either. Raised by: parity-checker.
| <div *ngIf="information.uris && information.uris.length > 0" fxLayout="row"> | ||
| <mat-form-field fxLayout="column" fxFlex="100" fxFlex.gt-sm="40" fxLayoutAlign="start end"> | ||
| <mat-select tabindex="1" [(value)]="selInfoType"> | ||
| <mat-label>Info type</mat-label> |
There was a problem hiding this comment.
🟡 F2 (minor): The PR adds <mat-label>Info type</mat-label> to the info-type select but leaves two other positive-tabindex attributes in the same file untouched: the close <button tabindex="3" ...>X</button> at line 11, and the copy <button ... tabindex="2" type="submit" ...> at line 36. The PR body says positive tabindex is being removed because it causes unpredictable keyboard navigation — leaving them in a file the PR otherwise edits leaves the dialog's tab order still broken (the X jumps over the form), and a reviewer trusting the description would not notice. Either drop the two tabindex values here or narrow the PR description to call out which leftover values are intentionally out of scope. Minor labeling nit: the rest of the codebase capitalises every mat-label word (e.g. "Amount Unit", "Time Unit", "Transaction Type"); "Info type" is the only lowercase one — make it "Info Type" for consistency with the surrounding labels. Raised by: code-reviewer.
| {{scrollRange | titlecase}} | ||
| </mat-option> | ||
| </mat-select> | ||
| <mat-form-field fxLayout="column" fxFlex="100" fxFlex.gt-sm="40" fxLayoutAlign="start end"> |
There was a problem hiding this comment.
🟡 F3 (minor): The PR wraps the scroll-range mat-select in a mat-form-field with a <mat-label> and adds labels to the two datepicker inputs — good. But the four navigation buttons at lines 3–6 (FIRST / PREVIOUS / NEXT / LAST mat-icon-buttons) still carry tabindex="1", tabindex="2", tabindex="5", tabindex="6". Those are positive tabindex values on the file's most-used focus path, and the PR description explicitly says these are being removed. Since this is a shared component used across every implementation, the inconsistency is more visible than in any single modal. Drop them or note the exception in the description. Raised by: code-reviewer.
| <div fxLayout="row" fxLayoutAlign="start start"> | ||
| <mat-form-field fxLayout="column" fxLayoutAlign="start start"> | ||
| <mat-select autoFocus tabindex="1" name="defaultNode" [(ngModel)]="appConfig.defaultNodeIndex"> | ||
| <mat-label>Default Node</mat-label> |
There was a problem hiding this comment.
🟡 F4 (minor): The PR strips tabindex="1" from the Default Node mat-select and adds its mat-label — but the Reset (tabindex="2") and Update (tabindex="3") buttons immediately below (lines 19–20 in the post-change file) still carry positive tabindex values. The Settings page is the only one the author manually verified per the PR body, so the missed half is conspicuous: with the mat-select no longer pinned to tabindex="1", the natural DOM order now puts the buttons before the select via the explicit tabs, which is exactly the unpredictable keyboard navigation the PR set out to fix. Either remove these two tabindex values for a consistent fix here, or update the description. Raised by: code-reviewer.
|
🤖 rtlreviewbot audit metadata for this PR — auto-generated, please don't edit. |
…ues and add CLN parity fixes
|
@saubyk |
|
/rtl re-review |
|
✅ Re-review posted: #1609 (review) Prior findings: 2 addressed, 2 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
Substantial response since the last review: CLN siblings for open-channel, bump-fee, create-invoice, and on-chain-send are now included, Eclair siblings for on-chain-send and create-invoice are in, and every leftover positive tabindex in the three shared components (show-pubkey, horizontal-scroller, app-settings) has been stripped. F3 and F4 are fully fixed, F1 is largely resolved bar a residual close-channel parity question, and F2 is fixed except for a one-word capitalization nit.
No new defects emerged: pure {{...}} interpolations, no unsafe template binding, no auth-gate change. A few touched files lost their trailing newline and one type="button " attribute has a trailing space, but neither rises to a finding.
Status of prior findings
- F3 addressed: Fixed — all four navigation
mat-icon-buttons insrc/app/shared/components/horizontal-scroller/horizontal-scroller.component.html(FIRST/PREVIOUS/NEXT/LAST, formerlytabindex="1|2|5|6") now carry no positivetabindex. The scroll-rangemat-selectwas additionally wrapped in amat-form-fieldwith aScroll Rangelabel, and the monthly / yearly datepicker inputs receivedMonthly Date/Yearly Datelabels. - F4 addressed: Fixed —
tabindex="2"on the Reset button andtabindex="3"on the Update button insrc/app/shared/components/settings/app-settings/app-settings.component.htmlare both removed; the Default Nodemat-selectnow has itsDefault Nodelabel and natural DOM focus order is restored.
| <span class="page-title">{{channelToClose.active ? 'Close Channel' : 'Force Close Channel'}}</span> | ||
| </div> | ||
| <button tabindex="8" fxFlex="5" fxLayoutAlign="center center" class="btn-close-x p-0" mat-button (click)="onClose()">X</button> | ||
| <button fxFlex="5" fxLayoutAlign="center center" class="btn-close-x p-0" mat-button (click)="onClose()">X</button> |
There was a problem hiding this comment.
🟡 F1 (minor) — partially_addressed: CLN open-channel-modal and bump-fee siblings are now in the PR (src/app/cln/peers-channels/channels/open-channel-modal/open-channel.component.html and .../bump-fee-modal/bump-fee.component.html), and on-chain-send / create-invoice are mirrored across CLN and Eclair too — the bulk of the parity gap is closed. What remains unresolved is the close-channel surface: LND's close-channel-modal got mat-label additions and tabindex stripping, but no CLN/Eclair sibling appears in files[]. Channel close in CLN/Eclair may be implemented as an inline confirmation rather than a dedicated modal with mat-form-field controls, so this is plausibly LND-only and not an actual drift; the sibling paths are not in file_contents[] so this is unverified rather than asserted. Worth a quick author confirmation that CLN/Eclair expose no equivalent mat-form-field close-channel UI that should receive the same fix. Raised by: parity-checker.
| <div *ngIf="information.uris && information.uris.length > 0" fxLayout="row"> | ||
| <mat-form-field fxLayout="column" fxFlex="100" fxFlex.gt-sm="40" fxLayoutAlign="start end"> | ||
| <mat-select tabindex="1" [(value)]="selInfoType"> | ||
| <mat-label>Info type</mat-label> |
There was a problem hiding this comment.
🟡 F2 (minor) — partially_addressed: Both leftover positive tabindex values flagged previously are removed — the X close button no longer carries tabindex="3" (line 11 in post-change) and the Copy button no longer carries tabindex="2" (line 34 in post-change). What remains is the labeling nit: the newly added <mat-label>Info type</mat-label> uses sentence case while every other label added in this PR uses Title Case (Bitcoin Address, Amount Unit, Default Node, Scroll Range, Time Unit, Transaction Type, Number of Blocks). Prefer Info Type for visual consistency. Raised by: code-reviewer.
|
@SIDHARTH20K4 can you look at the F1 and F2 findings which the bot is flagging as partially addressed? Also wondering about the feedback posted by Gemini and Copilot? Worth addressing? |
|
@saubyk Neither has a Also fixed F2... "Info Type" is now Title Case. |
Fixes #1566
Several mat-form-field elements across the app were missing
mat-label, which meant screen readers had no way to announce what each field was for. This makes the app harder to use for blind users.Changes made:
-Added mat-label to all affected form fields with descriptive label text
-Removed positive
tabindexvalues which cause unpredictable keyboard navigationTesting:
-Verified the Settings page visually before and after (screenshots attached)
-Could not test invoice/send modals as they require a live Lightning node connection
-Remaining files were verified by code review