refactor(web): consolidate shape tokens and improve keyboard accessibility#1112
refactor(web): consolidate shape tokens and improve keyboard accessibility#1112PavluntiyJ wants to merge 2 commits into
Conversation
Greptile SummaryThis PR consolidates ~80 stylesheets to reference a new shared radius token scale (
Confidence Score: 3/5The CSS token migration across 80+ files is safe and well-executed, but the keyboard accessibility addition introduces a real interaction defect that affects every keyboard user who interacts with action buttons inside channel rows and episode cards. The libs/ui/components/src/lib/channel-list-container/channel-list-item/channel-list-item.component.ts and both episode-card/episode-list-item blocks in libs/ui/components/src/lib/season-container/season-container.component.html Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User presses Space/Enter] --> B{Focus target?}
B -->|channel-list-item div| C[onKeydownActivate fires]
B -->|child button: favorite/info/aux| D[keydown bubbles to parent div]
D --> C
C --> E[event.preventDefault]
C --> F[clicked.emit → channel selected]
D --> G[keyup fires on child button]
G --> H[button click handler fires]
H --> I[onFavoriteClick / showProgramDescription / onAuxActionClick]
F & I --> J[Double-action: channel selected AND button action triggered]
style J fill:#ff6b6b,color:#fff
style C fill:#ffd93d
style D fill:#ff6b6b,color:#fff
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[User presses Space/Enter] --> B{Focus target?}
B -->|channel-list-item div| C[onKeydownActivate fires]
B -->|child button: favorite/info/aux| D[keydown bubbles to parent div]
D --> C
C --> E[event.preventDefault]
C --> F[clicked.emit → channel selected]
D --> G[keyup fires on child button]
G --> H[button click handler fires]
H --> I[onFavoriteClick / showProgramDescription / onAuxActionClick]
F & I --> J[Double-action: channel selected AND button action triggered]
style J fill:#ff6b6b,color:#fff
style C fill:#ffd93d
style D fill:#ff6b6b,color:#fff
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onKeydownActivate(event: Event): void { | ||
| event.preventDefault(); | ||
| this.clicked.emit(); | ||
| } |
There was a problem hiding this comment.
onKeydownActivate is bound at the outer <div> level, so a Space/Enter keydown on any inner <button> (favorite, info, aux-action) bubbles up and triggers this handler. When focus is on the favorite button and the user presses Space: clicked emits (channel selection fires), then the button's own keyup fires its click → onFavoriteClick runs. The user gets an unintended channel-select event every time they interact with an action button via keyboard.
The fix is to bail out when the event originated from a descendant element — event.target is the element that received the keypress; event.currentTarget is the .channel-list-item div, and they are only equal when the div itself has focus.
| onKeydownActivate(event: Event): void { | |
| event.preventDefault(); | |
| this.clicked.emit(); | |
| } | |
| onKeydownActivate(event: Event): void { | |
| if ((event.target as HTMLElement) !== (event.currentTarget as HTMLElement)) return; | |
| event.preventDefault(); | |
| this.clicked.emit(); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: libs/ui/components/src/lib/channel-list-container/channel-list-item/channel-list-item.component.ts
Line: 99-102
Comment:
**Keydown bubbles from child buttons, causing double-fire**
`onKeydownActivate` is bound at the outer `<div>` level, so a Space/Enter keydown on any inner `<button>` (favorite, info, aux-action) bubbles up and triggers this handler. When focus is on the favorite button and the user presses Space: `clicked` emits (channel selection fires), *then* the button's own keyup fires its click → `onFavoriteClick` runs. The user gets an unintended channel-select event every time they interact with an action button via keyboard.
The fix is to bail out when the event originated from a descendant element — `event.target` is the element that received the keypress; `event.currentTarget` is the `.channel-list-item` div, and they are only equal when the div itself has focus.
```suggestion
onKeydownActivate(event: Event): void {
if ((event.target as HTMLElement) !== (event.currentTarget as HTMLElement)) return;
event.preventDefault();
this.clicked.emit();
}
```
How can I resolve this? If you propose a fix, please make it concise.| <article | ||
| class="episode-card" | ||
| role="button" | ||
| tabindex="0" | ||
| (click)="selectEpisode(episode)" | ||
| (keydown.enter)="selectEpisode(episode)" | ||
| (keydown.space)=" | ||
| $event.preventDefault(); | ||
| selectEpisode(episode) | ||
| " |
There was a problem hiding this comment.
The download, play-local, and toggle-watched <button> elements live inside this <article role="button">. When any of those inner buttons has keyboard focus and the user presses Space or Enter, the keydown event bubbles to the article's (keydown.enter) / (keydown.space) handlers and selectEpisode fires in addition to the button's own click handler — pressing Space on "mark as watched" would simultaneously start episode playback. The same pattern applies to both the grid-view episode-card and the list-view episode-list-item. Both need a target guard or the inner buttons need (keydown.enter)="$event.stopPropagation()" / (keydown.space)="$event.stopPropagation()" guards.
Prompt To Fix With AI
This is a comment left during a code review.
Path: libs/ui/components/src/lib/season-container/season-container.component.html
Line: 96-105
Comment:
**Same event-bubbling issue for episode-card action buttons**
The download, play-local, and toggle-watched `<button>` elements live inside this `<article role="button">`. When any of those inner buttons has keyboard focus and the user presses Space or Enter, the `keydown` event bubbles to the article's `(keydown.enter)` / `(keydown.space)` handlers and `selectEpisode` fires in addition to the button's own click handler — pressing Space on "mark as watched" would simultaneously start episode playback. The same pattern applies to both the grid-view episode-card and the list-view episode-list-item. Both need a target guard or the inner buttons need `(keydown.enter)="$event.stopPropagation()"` / `(keydown.space)="$event.stopPropagation()"` guards.
How can I resolve this? If you propose a fix, please make it concise.| role="button" | ||
| tabindex="0" | ||
| [attr.aria-label]="name()" | ||
| [attr.aria-pressed]="selected()" |
There was a problem hiding this comment.
aria-pressed is the ARIA attribute for toggle buttons where the same button switches between two states. Here "selected" means "currently active channel" — the user cannot deactivate the channel by pressing the same row a second time. Screen readers will announce this as "pressed" on the active row, implying it can be un-pressed, which is misleading. aria-current="true" better matches the "active item in a navigation list" pattern without implying a toggle.
| [attr.aria-pressed]="selected()" | |
| [attr.aria-current]="selected() || null" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: libs/ui/components/src/lib/channel-list-container/channel-list-item/channel-list-item.component.html
Line: 11
Comment:
`aria-pressed` is the ARIA attribute for toggle buttons where the same button switches between two states. Here "selected" means "currently active channel" — the user cannot deactivate the channel by pressing the same row a second time. Screen readers will announce this as "pressed" on the active row, implying it can be un-pressed, which is misleading. `aria-current="true"` better matches the "active item in a navigation list" pattern without implying a toggle.
```suggestion
[attr.aria-current]="selected() || null"
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…ility - Migrated hardcoded border-radius values to --app-radius-* tokens across ~80 stylesheets in apps/web and libs/*, adding a new --app-radius-2xl tier where the existing scale had a gap - Fixed the two genuinely theme-locked hardcoded colors in season-container (thumbnail/placeholder backgrounds) to use --app-content-bg / --app-widget-header-bg - Reverted an incorrect attempt to theme remote-control via --app-* tokens: it ships inside apps/remote-control-web, a standalone bundle that never loads m3-theme.scss, so those tokens don't exist there. It keeps its original self-contained dark palette and literal px radii instead - Added keyboard accessibility: channel-list-item, episode-card and episode-list-item were clickable elements with no tabindex/role, making them unreachable via keyboard. Added role="button", tabindex, Enter/Space activation, :focus-visible rings, and aria-label on icon-only buttons that previously relied solely on matTooltip - Added :focus-visible states to remote-control's touchpad/keypad/ volume buttons - Added regression tests for the new keyboard activation and aria-label behavior on channel-list-item
Enter/Space on channel-list-item and season-container's episode cards was bound at the row/article level, so the event also fired when a descendant action button (favorite, info, aux, download, toggle-watched) had keyboard focus — pressing Space to toggle a favorite also selected/activated the row underneath it. Guard both handlers on event.target === event.currentTarget so only the row itself (not a bubbled child event) triggers activation. Also switch channel-list-item's aria-pressed to aria-current: the row isn't a toggle button, it's the current item in a list, and aria-pressed misleadingly implies it can be un-pressed. Found by Greptile review on the PR.
PavluntiyJ
force-pushed
the
feature/ui-tokens-and-accessibility
branch
from
43c4b69 to
9d641ac
Compare
1 participant
Summary
--app-radius-xs/sm/md/lg/xl/2xl/pill) inm3-theme.scssand migrated every hardcodedborder-radiusvalue acrossapps/webandlibs/*(~80 stylesheets) to reference it.channel-list-item(used everywhere channels are listed) andseason-container's episode/season cards were clickable elements with notabindex/role, making them completely unreachable via keyboard. Addedrole="button",tabindex="0", Enter/Space activation, visible:focus-visiblerings, andaria-labelon icon-only action buttons that previously relied only onmatTooltip(invisible to screen readers).season-container(thumbnail/placeholder backgrounds) to use--app-content-bg/--app-widget-header-bg.Verification
Tabnow lands on the row with a visible focus ring,Enteractivates playback, a furtherTabreaches the favorite button with its own ring and accessible label.--app-*tokens (undefined in that bundle), then confirmed it's fully restored after reverting to its self-contained palette, including visible focus rings on the touchpad/keypad/volume buttons.Why
remote-controlkeeps hardcoded colorsremote-control.component.scssrenders insideapps/remote-control-web— a separate, standalone Angular app (served to a phone on the local network) that never importsapps/web/src/m3-theme.scssand has no--app-*custom properties defined anywhere. An earlier pass in this PR tried to theme it with--app-*tokens like the rest of the app; live verification showed this broke the component entirely (backgrounds/borders resolve to nothing when the referenced custom property doesn't exist in the cascade). It was reverted to its original self-contained dark palette — the only correct option for a bundle with no theme system — while keeping the new:focus-visibleaccessibility additions, which don't depend on that assumption.What we deliberately didn't touch
box-shadow— left alone everywhere. The existing--app-shadow-soft/--app-shadow-raisedtokens are low-contrast (0.04–0.16 opacity), designed for subtle card elevation. Most realbox-shadowusages in the app are either semantic (selection glows, live-indicator glows, colored accent shadows) or much heavier modal/hero/player shadows (0.12–0.6 opacity). Forcing either category onto the two soft/raised tokens would have flattened intentional visual depth.season-container'srgba(255,255,255,…)glass overlays — these already derive their contrast from--text-primary/--accent-color, a separate backdrop-adaptive system (tuned per movie/series poster luminance) that cascades from the parent detail-view, unrelated to the app's light/dark theme toggle.@media-query gaps andprefers-reduced-motionsupport, both flagged in the initial UI audit — left for a follow-up PR to keep this change focused on tokens + keyboard a11y.Testing
pnpm nx test components— 13 suites / 72 tests pass, including 2 new regression tests (keyboard activation via Enter/Space,aria-label/role/tabindexonchannel-list-item).pnpm nx build webandpnpm nx build remote-control-web— both succeed.pnpm nx lint components/lint remote-control— no new issues.