KofTwentyTwo/gclo
2
0 Fork 0
Code Issues 28 Pull Requests 1 Actions Packages Projects Releases 22 Wiki Activity

UI/UX review: 22 adversarially-verified findings (accessibility, theming, workflow, microcopy) #22

New Issue
Closed
opened 2026-07-03 20:29:32 +00:00 by KofTwentyTwo · 3 comments
KofTwentyTwo commented 2026-07-03 20:29:32 +00:00 (Migrated from github.com)
Copy link
Copy source

Five specialist review agents (workflow, Fluent compliance, accessibility, feedback/state, microcopy) reviewed the shipped v0.1.0-beta.4 XAML and view models at the code level; every finding survived two adversarial verifiers (factual + design-value). 51 raw → 26 confirmed → 22 after consolidating duplicates. Overall: structure and workflow reviewed well; the systemic gaps are theme-hostile hardcoded status colors and a near-silent screen-reader experience.

P1 — accessibility & theming blockers (~1 focused day)

  • Status colors are hardcoded literals and the only status differentiator (StatusFormat.cs:12–25): Gray/DodgerBlue/ForestGreen/Crimson/Orange ignore dark theme and High Contrast, and there are no icons — Done vs Failed is color-alone. Fix: WinUI semantic brushes (SystemFillColorSuccessBrush, SystemFillColorCriticalBrush, SystemFillColorCautionBrush, SystemFillColorAttentionBrush, neutral for Queued) resolved per-call from Application.Current.Resources, plus a per-status FontIcon glyph. (Found independently by 3 of 5 reviewers.)
  • Inline row error uses Foreground="Crimson" (MainWindow.xaml:321) → {ThemeResource SystemFillColorCriticalBrush}.
  • No live regions — sync progress/summary/errors never announced (MainWindow.xaml:168–171): AutomationProperties.LiveSetting="Polite" on StatusText (Assertive for errors) + RaiseNotificationEvent for summary/failures.
  • Row selection checkboxes have no accessible name (MainWindow.xaml:272–278): announce as bare "checkbox" — add AutomationProperties.Name with the repo name.

P2 — visible friction

  • Overall progress bar dishonest on subset syncs: Maximum stays at full loaded count; scope TotalCount/completed counting to the captured run set (MainViewModel.cs).
  • Inputs editable mid-run; path preview can contradict the running job: add CanEditInputs (!IsRunning && !IsLoadingRepos) bound to the input region's IsEnabled.
  • One StatusText multiplexes org-lookup/load/sync/errors: a token keystroke mid-sync clobbers "Syncing…"; give org-lookup inline feedback by the ComboBox, or at minimum gate its writes on !IsRunning.
  • No minimum window size — action row clips off-screen: OverlappedPresenter minimum (~660×500); optionally wrap the action row.
  • First-run token guidance is reactive only: "How to create a token" HyperlinkButton + required scopes (repo, optional read:org) near the field.
  • Table on raw Mica: wrap header+ListView in LayerFillColorDefaultBrush with CornerRadius="8".
  • No table semantics for screen readers (L effort): near-term composed per-row AutomationProperties.Name ("name, status, branch, archived"); long-term a real grid control.
  • Sort headers announce the glyph ("Name black up-pointing triangle"): visual-only arrow + proper names ("Name, sorted ascending") + polite notification on sort.
  • Unlabeled controls: identical "Resolve…" links (add repo name), select-all checkbox (tooltip isn't a UIA name), PathRecoveryDialog rename fields/Skip checkboxes (LabeledBy).
  • No access keys/accelerators: menu mnemonics + Ctrl+R Load, Ctrl+Enter Sync, Esc Cancel.
  • "Parallel" NumberBox header names no noun: "Parallel clones" (also in Settings) + tooltip.
  • Path preview dangles when empty and says "created" for updates: collapse when empty; "Each repository will be placed at:".

P3 — polish

  • "Retry failed" stays visible (inert) after the last failure is Resolved: dedicated AnyFailed observable instead of piggy-backing on CompletedCount.
  • No busy state while a path recovery applies: set the row active during ApplyRecoveryAsync so the per-row bar shows.
  • Progress bar/spinners lack automation names ("Overall sync progress", "Loading organizations/repositories").
  • "Browse..." (three periods) vs "Settings…" (glyph): use "…" everywhere.
  • "{n} path(s)" pluralization in the user-facing invalid-path row error: explicit singular/plural.
  • Status colors item also noted by the feedback reviewer (consolidated into the P1 above).

Method note: code-level review (headless machine — pixel/spacing judgments deliberately excluded); 25 findings were refuted during verification and discarded. Full narrative report delivered to the owner as a private artifact page.

Five specialist review agents (workflow, Fluent compliance, accessibility, feedback/state, microcopy) reviewed the shipped v0.1.0-beta.4 XAML and view models at the code level; every finding survived two adversarial verifiers (factual + design-value). 51 raw → 26 confirmed → 22 after consolidating duplicates. **Overall: structure and workflow reviewed well; the systemic gaps are theme-hostile hardcoded status colors and a near-silent screen-reader experience.** ## P1 — accessibility & theming blockers (~1 focused day) - [ ] **Status colors are hardcoded literals and the only status differentiator** (`StatusFormat.cs:12–25`): `Gray/DodgerBlue/ForestGreen/Crimson/Orange` ignore dark theme and High Contrast, and there are no icons — Done vs Failed is color-alone. Fix: WinUI semantic brushes (`SystemFillColorSuccessBrush`, `SystemFillColorCriticalBrush`, `SystemFillColorCautionBrush`, `SystemFillColorAttentionBrush`, neutral for Queued) resolved per-call from `Application.Current.Resources`, plus a per-status `FontIcon` glyph. *(Found independently by 3 of 5 reviewers.)* - [ ] **Inline row error uses `Foreground="Crimson"`** (`MainWindow.xaml:321`) → `{ThemeResource SystemFillColorCriticalBrush}`. - [ ] **No live regions — sync progress/summary/errors never announced** (`MainWindow.xaml:168–171`): `AutomationProperties.LiveSetting="Polite"` on StatusText (Assertive for errors) + `RaiseNotificationEvent` for summary/failures. - [ ] **Row selection checkboxes have no accessible name** (`MainWindow.xaml:272–278`): announce as bare "checkbox" — add `AutomationProperties.Name` with the repo name. ## P2 — visible friction - [ ] **Overall progress bar dishonest on subset syncs**: Maximum stays at full loaded count; scope `TotalCount`/completed counting to the captured run set (`MainViewModel.cs`). - [ ] **Inputs editable mid-run; path preview can contradict the running job**: add `CanEditInputs` (`!IsRunning && !IsLoadingRepos`) bound to the input region's `IsEnabled`. - [ ] **One StatusText multiplexes org-lookup/load/sync/errors**: a token keystroke mid-sync clobbers "Syncing…"; give org-lookup inline feedback by the ComboBox, or at minimum gate its writes on `!IsRunning`. - [ ] **No minimum window size — action row clips off-screen**: `OverlappedPresenter` minimum (~660×500); optionally wrap the action row. - [ ] **First-run token guidance is reactive only**: "How to create a token" HyperlinkButton + required scopes (`repo`, optional `read:org`) near the field. - [ ] **Table on raw Mica**: wrap header+ListView in `LayerFillColorDefaultBrush` with `CornerRadius="8"`. - [ ] **No table semantics for screen readers** (`L` effort): near-term composed per-row `AutomationProperties.Name` ("name, status, branch, archived"); long-term a real grid control. - [ ] **Sort headers announce the glyph** ("Name black up-pointing triangle"): visual-only arrow + proper names ("Name, sorted ascending") + polite notification on sort. - [ ] **Unlabeled controls**: identical "Resolve…" links (add repo name), select-all checkbox (tooltip isn't a UIA name), PathRecoveryDialog rename fields/Skip checkboxes (`LabeledBy`). - [ ] **No access keys/accelerators**: menu mnemonics + Ctrl+R Load, Ctrl+Enter Sync, Esc Cancel. - [ ] **"Parallel" NumberBox header names no noun**: "Parallel clones" (also in Settings) + tooltip. - [ ] **Path preview dangles when empty and says "created" for updates**: collapse when empty; "Each repository will be placed at:". ## P3 — polish - [ ] "Retry failed" stays visible (inert) after the last failure is Resolved: dedicated `AnyFailed` observable instead of piggy-backing on `CompletedCount`. - [ ] No busy state while a path recovery applies: set the row active during `ApplyRecoveryAsync` so the per-row bar shows. - [ ] Progress bar/spinners lack automation names ("Overall sync progress", "Loading organizations/repositories"). - [ ] "Browse..." (three periods) vs "Settings…" (glyph): use "…" everywhere. - [ ] "{n} path(s)" pluralization in the user-facing invalid-path row error: explicit singular/plural. - [ ] Status colors item also noted by the feedback reviewer (consolidated into the P1 above). Method note: code-level review (headless machine — pixel/spacing judgments deliberately excluded); 25 findings were refuted during verification and discarded. Full narrative report delivered to the owner as a private artifact page.
KofTwentyTwo commented 2026-07-03 20:36:46 +00:00 (Migrated from github.com)
Copy link
Copy source

Owner follow-up: composition-level critique (layout hierarchy, IA) was out of this review's defect-verification scope — addressed as a redesign direction posted on #16, which the P2/P3 items here should be reconciled against before implementation (several dissolve in the new layout: the action-row clipping, status multiplexing, and path-preview items are all restructured away).

Owner follow-up: composition-level critique (layout hierarchy, IA) was out of this review's defect-verification scope — addressed as a redesign direction posted on #16, which the P2/P3 items here should be reconciled against before implementation (several dissolve in the new layout: the action-row clipping, status multiplexing, and path-preview items are all restructured away).
KofTwentyTwo commented 2026-07-04 18:06:08 +00:00 (Migrated from github.com)
Copy link
Copy source

All four P1 items shipped in v0.1.0-beta.5 (PR #23): theme/HC-aware semantic status brushes + status glyphs (never color-alone), SystemFillColorCriticalBrush errors, a WORKING live region for StatusText (doc-verification caught that LiveSetting alone never announces — LiveRegionChanged is now raised per update), UIA failure notifications, and AutomationProperties.Name on all previously anonymous controls. Owner checkpoint: verify visually in light/dark/High Contrast. P2/P3 items proceed per the plan's Phase 3 reconciliation.

All four P1 items shipped in v0.1.0-beta.5 (PR #23): theme/HC-aware semantic status brushes + status glyphs (never color-alone), SystemFillColorCriticalBrush errors, a WORKING live region for StatusText (doc-verification caught that LiveSetting alone never announces — LiveRegionChanged is now raised per update), UIA failure notifications, and AutomationProperties.Name on all previously anonymous controls. Owner checkpoint: verify visually in light/dark/High Contrast. P2/P3 items proceed per the plan's Phase 3 reconciliation.
KofTwentyTwo commented 2026-07-05 19:12:41 +00:00 (Migrated from github.com)
Copy link
Copy source

All findings addressed across the accessibility wave (beta.5), the workspace redesign (beta.6–8), and this session's responsive-layout work (beta.11).

  • P1 (accessibility & theming): status now uses theme-/HC-aware SystemFillColor* brushes plus per-status glyphs (color is never the sole differentiator); inline errors use SystemFillColorCriticalBrush; StatusText is a live region with a manual LiveRegionChanged raise and RaiseNotificationEvent for failures; selection checkboxes and Resolve links carry AutomationProperties.Name.
  • P2: run-scoped progress (TotalCount/completed over the captured set); CanEditInputs gates inputs mid-run; minimum window size via OverlappedPresenter plus a responsive two-row toolbar so actions never clip; "How to create a token" guidance; the table sits on a LayerFillColorDefaultBrush card; sort headers announce composed names ("Name, sorted ascending") with the glyph in the Raw view; access keys + Ctrl+R / Ctrl+Enter / Esc.
  • P3: HasFailedRepos flag (Retry hides when nothing's failed); per-row busy state during recovery; automation names on progress bars/rings; "Browse…" uses the ellipsis everywhere; explicit singular/plural in path messages.

The one long-term item — a real grid control for full table semantics — is tracked separately as #33 (adopt WinUI.TableView once its accessibility release ships). Closing this review issue.

**All findings addressed** across the accessibility wave (beta.5), the workspace redesign (beta.6–8), and this session's responsive-layout work (beta.11). - **P1 (accessibility & theming):** status now uses theme-/HC-aware `SystemFillColor*` brushes plus per-status glyphs (color is never the sole differentiator); inline errors use `SystemFillColorCriticalBrush`; StatusText is a live region with a manual `LiveRegionChanged` raise and `RaiseNotificationEvent` for failures; selection checkboxes and Resolve links carry `AutomationProperties.Name`. - **P2:** run-scoped progress (`TotalCount`/completed over the captured set); `CanEditInputs` gates inputs mid-run; minimum window size via `OverlappedPresenter` plus a responsive two-row toolbar so actions never clip; "How to create a token" guidance; the table sits on a `LayerFillColorDefaultBrush` card; sort headers announce composed names ("Name, sorted ascending") with the glyph in the Raw view; access keys + Ctrl+R / Ctrl+Enter / Esc. - **P3:** `HasFailedRepos` flag (Retry hides when nothing's failed); per-row busy state during recovery; automation names on progress bars/rings; "Browse…" uses the ellipsis everywhere; explicit singular/plural in path messages. The one long-term item — a **real grid control** for full table semantics — is tracked separately as #33 (adopt WinUI.TableView once its accessibility release ships). Closing this review issue.
Sign in to join this conversation.
No Branch/Tag Specified
Labels
Clear labels
.NET
Pull requests that update .NET code
 bug
Something isn't working
 deferred
Agreed follow-up, not scheduled
 dependencies
Pull requests that update a dependency file
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 github_actions
Pull requests that update GitHub Actions code
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 needs-owner
Blocked on an action only the owner can take
 priority
Owner-flagged: work this first
 quality
Standards, simplicity, enforcement
 question
Further information is requested
 refactor
Restructuring without behavior change
 release
Release pipeline / distribution
 wontfix
This will not be worked on
No labels enhancement quality
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
forge-recovery (Forge Recovery) james.maes (James Maes)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: KofTwentyTwo/gclo#22