fix(portfolio): keep the balance out of the aria labels in privacy mode - #8044
fix(portfolio): keep the balance out of the aria labels in privacy mode#8044yhabib wants to merge 4 commits into
Conversation
The Portfolio cards masked the balance on screen but kept the exact number in the `aria-label` of the cell. A screen reader announced the number, and the DOM held it. Each of the four labels now follows `isBalancePrivacyOptionStore`, the same store the visible value follows. With privacy mode on the label ends in `hidden`, so the cell keeps a usable name. The staked native cell also labelled itself with the USD number and a `D:` typo. It now names the native stake and its symbol.
The spec signs in a new user, gets ICP, stakes a neuron, and reads the accessible names on the Portfolio cards with privacy mode off and on.
|
✅ No security or compliance issues detected. Reviewed everything up to 9724708. Security Overview
Detected Code Changes
|
There was a problem hiding this comment.
🟡 Changes recommended
The updated header aria-label can still become ${title}: undefined when usdAmount is omitted (e.g., on “no tokens” cards), which is an accessibility bug that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR closes a privacy-mode accessibility leak on the Portfolio page by ensuring balance-related aria-labels mirror the masked UI state (announcing a localized “hidden” label instead of the raw numeric amount).
Changes:
- Updated Portfolio card header and balance-cell
aria-labels to respect privacy mode, and corrected the native-stake label content in the staked tokens table. - Added a new i18n key (
portfolio.hidden_balance_label) and updated types accordingly. - Added unit tests plus a new Playwright e2e spec to detect balance digits leaking via
aria-label/title/altattributes.
File summaries
| File | Description |
|---|---|
| frontend/src/tests/page-objects/StakedTokensCard.page-object.ts | Adds getters to read new/updated balance-related aria-labels for tests. |
| frontend/src/tests/page-objects/HeldTokensCard.page-object.ts | Adds getters to read USD row and header amount aria-labels for tests. |
| frontend/src/tests/lib/components/portfolio/StakedTokensCard.spec.ts | Adds unit coverage for aria-label behavior with privacy mode on/off. |
| frontend/src/tests/lib/components/portfolio/HeldTokensCard.spec.ts | Adds unit coverage for aria-label behavior with privacy mode on/off. |
| frontend/src/tests/e2e/portfolio-privacy-aria-labels.spec.ts | Adds an end-to-end digit-leak scan over accessible-name-related attributes in Portfolio cards. |
| frontend/src/lib/types/i18n.d.ts | Extends i18n typings with portfolio.hidden_balance_label. |
| frontend/src/lib/i18n/en.json | Adds the new portfolio.hidden_balance_label string. |
| frontend/src/lib/components/portfolio/TokensCardHeader.svelte | Makes the header amount aria-label privacy-aware. |
| frontend/src/lib/components/portfolio/StakedTokensCard.svelte | Makes stake cell aria-labels privacy-aware and fixes the native-stake label content. |
| frontend/src/lib/components/portfolio/HeldTokensCard.svelte | Makes the USD balance cell aria-label privacy-aware. |
| CHANGELOG-Nns-Dapp-unreleased.md | Documents the privacy/a11y fix under Security. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes user-facing accessibility behavior for a security/privacy concern and includes a known ARIA-spec compliance concern that needs manual screen-reader validation.
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approved
The changes consistently apply privacy mode to the relevant accessibility labels and are backed by targeted unit and e2e tests to prevent regressions.
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
Escape did not close the account menu Popover: gix-components' handleKeyPress only reacts to Enter and Space, not Escape. The menu button toggles Popover visibility on each click, so click it again to close it. This is what made the shard 1 CI job time out.
There was a problem hiding this comment.
🟢 Approved
The changes consistently apply privacy-mode masking to accessible labels, are backed by unit tests (and an additional e2e spec), and do not introduce any verified regressions in the reviewed code paths.
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
The account-menu button click was blocked: the Popover backdrop covers the whole screen, including the button, and intercepts pointer events. Click the backdrop (which has its own click handler) instead.
There was a problem hiding this comment.
🟢 Approved
The changes directly address the described privacy leak with targeted unit/e2e coverage, and the remaining feedback is limited to minor e2e step-await best-practice improvements.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
frontend/src/tests/e2e/portfolio-privacy-aria-labels.spec.ts:44
stepis an async helper (seefrontend/src/tests/utils/e2e.test-utils.ts:11-20) that chainstest.stepcalls via awaits. Calling it withoutawaitcan start multiple steps concurrently and leave previous step promises unresolved, making step reporting flaky.
Consider awaiting each step(...) call (here and throughout the spec) so the step boundaries are deterministic.
This issue also appears on line 104 of the same file.
frontend/src/tests/e2e/portfolio-privacy-aria-labels.spec.ts:104
- Same as above: this
step(...)call should be awaited to preserve the intended sequencing oftest.stepblocks and avoid dangling step promises.
step("Privacy mode off: every accessible name carries its amount");
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
Motivation
Privacy mode masks token and staking balances on screen, but the
aria-labelon the balance cells still carried the raw number. A screen reader announced the hidden amount even while the display showed•••.Changes
hiddeninstead of the number.D:typo instead of the native stake and its symbol.portfolio.hidden_balance_labeli18n key for the announced word.Tests
portfolio-privacy-aria-labels.spec.ts, that signs in, stakes ICP, toggles privacy mode, and sweeps everyaria-label,title, andaltin the two cards for a leaked digit. It could not run locally: the replica would not come up with NNS canisters on this machine.npm run check,CI=true npm run test, and./scripts/check-relative-importsall pass.Todos
aria-labelon the header<p>amount, since ARIA prohibits a name on theparagraphrole.CHANGELOG-Nns-Dapp-unreleased.md.