feat(plan): tap/focus disclosure for the "why" reason, not just hover - #4349
feat(plan): tap/focus disclosure for the "why" reason, not just hover#4349chalfontchubby wants to merge 2 commits into
Conversation
fdb79df to
b37ba9c
Compare
title= only works for mouse hover - there's no hover state on a touchscreen to trigger it, and even on desktop it's a known accessibility gap (poor/inconsistent screen-reader support, and a plain <td> isn't keyboard-focusable). Reuses the dropdown-toggle mechanism already used for the time/rate cell manual-override menus (toggleForceDropdown/.dropdown-content/closeDropdowns, including the existing outside-click dismissal) rather than inventing a new pattern. Kept title= alongside it rather than replacing it: a touch interaction can't trigger :hover/title in the first place, so the two mechanisms never fire together and there's nothing to reconcile - desktop mouse users keep the free instant hover, touch/keyboard users get the new tap/focus panel. Added tabindex, onkeydown (Enter/Space), role="button" and a focus-visible outline so it's keyboard-operable too, not just tappable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
b37ba9c to
f0eaa64
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The new reason dropdown is likely to close immediately due to an existing document-level outside-click handler unless event propagation is stopped (see stored PR comment).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR improves the plan-table “why” explanation UX by making the per-slot reason text accessible on touch devices and keyboard navigation (not hover-only), while keeping the existing hover title= tooltip for desktop users. It does this entirely in the client-side plan renderer (web_helper.py), and adds/updates tests to cover the new wiring.
Changes:
- Adds a clickable/focusable “reason” disclosure panel for state cells using the existing dropdown mechanism (
toggleForceDropdown+.dropdown-content). - Adds new CSS for clickable state cells, keyboard focus outline, and reason text styling (including dark mode adjustments).
- Updates the “why reason” tests to validate both title tooltip behavior and the new tap/focus disclosure wiring.
File summaries
| File | Description |
|---|---|
| apps/predbat/web_helper.py | Adds CSS + JS rendering changes to make “why” reasons operable via tap and keyboard, not only hover. |
| apps/predbat/tests/test_plan_why_reason.py | Extends renderer tests to assert the new disclosure mechanism and retains tooltip coverage. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| const keydown = `if(event.key==='Enter'||event.key===' '){event.preventDefault();toggleForceDropdown('${dropdownId}')}`; | ||
| return { | ||
| clickAttrs: ` onclick="toggleForceDropdown('${dropdownId}')" onkeydown="${keydown}" tabindex="0" role="button" aria-label="Why this slot" class="${classAttr}"`, | ||
| panel: `<div class="dropdown"><div id="${dropdownId}" class="dropdown-content"><div class="reason-text">${escapeAttr(reasonText)}</div></div></div>`, | ||
| }; |
…oser The document-level "click outside closes the dropdown" handler only matched .clickable-time-cell, so tapping the new reason-cell disclosure (.clickable-state-cell) opened the panel and then immediately closed it again via the same click event bubbling to the document listener - Copilot review finding on springfall2008#4349. Added the missing class to the whitelist, plus a regression test asserting it (checks get_plan_css(), a separate function from get_plan_renderer_js() where the earlier tests in this file look) - verified it fails without the fix and passes with it.
|
Fixed - the click-outside handler only whitelisted 🤖 Generated with Claude Code |
Summary
Fast-follow to #4311 (merged) - addresses the mobile/accessibility gap flagged in review on that PR (a hover-only
title=attribute doesn't serve touch devices at all, since a tap can't trigger:hover)..dropdown-content/toggleForceDropdown/closeDropdownstap-to-toggle mechanism already used for the time/rate cell manual-override menus, kept alongsidetitle=rather than replacing it (a touch interaction can't trigger:hoverin the first place, so the two never fire together).tabindex,onkeydown,role="button"and a focus-visible outline so it's keyboard-operable too, not just tap.output.py) - purely aweb_helper.pyrendering change, since the data and rendering were already cleanly decoupled in feat(plan): "why" tooltips on the plan table (#4310) #4311.Test plan
./run_all --test plan_why_reason- all pass (including two new cases for the tap/focus disclosure wiring)pre-commit(ruff, black, cspell) run against the changed files, passed🤖 Generated with Claude Code