Implement the 024-fix-hp-popover-overflow feature that switches the HP adjustment popover from absolute to fixed positioning with viewport-aware clamping so it stays fully visible and causes no horizontal scrollbar, even when the HP display is near the right edge of the viewport
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -75,6 +75,7 @@ The constitution (`.specify/memory/constitution.md`) governs all feature work:
|
||||
- TypeScript 5.8 (strict mode, verbatimModuleSyntax) + React 19, Tailwind CSS v4 + React 19, Tailwind CSS v4, Vite 6 (022-fixed-layout-bars)
|
||||
- N/A (no storage changes -- purely presentational) (022-fixed-layout-bars)
|
||||
- Browser localStorage (existing adapter, updated to handle empty encounters) (023-clear-encounter)
|
||||
- N/A (no storage changes — purely presentational fix) (024-fix-hp-popover-overflow)
|
||||
|
||||
## Recent Changes
|
||||
- 003-remove-combatant: Added TypeScript 5.x (strict mode, verbatimModuleSyntax) + React 19, Vite
|
||||
|
||||
@@ -1,5 +1,11 @@
|
||||
import { Heart, Sword } from "lucide-react";
|
||||
import { useCallback, useEffect, useRef, useState } from "react";
|
||||
import {
|
||||
useCallback,
|
||||
useEffect,
|
||||
useLayoutEffect,
|
||||
useRef,
|
||||
useState,
|
||||
} from "react";
|
||||
import { Button } from "./ui/button";
|
||||
import { Input } from "./ui/input";
|
||||
|
||||
@@ -10,9 +16,28 @@ interface HpAdjustPopoverProps {
|
||||
|
||||
export function HpAdjustPopover({ onAdjust, onClose }: HpAdjustPopoverProps) {
|
||||
const [inputValue, setInputValue] = useState("");
|
||||
const [pos, setPos] = useState<{ top: number; left: number } | null>(null);
|
||||
const ref = useRef<HTMLDivElement>(null);
|
||||
const inputRef = useRef<HTMLInputElement>(null);
|
||||
|
||||
useLayoutEffect(() => {
|
||||
const el = ref.current;
|
||||
if (!el) return;
|
||||
const parent = el.parentElement;
|
||||
if (!parent) return;
|
||||
const trigger = parent.getBoundingClientRect();
|
||||
const popover = el.getBoundingClientRect();
|
||||
const vw = document.documentElement.clientWidth;
|
||||
let left = trigger.left;
|
||||
if (left + popover.width > vw) {
|
||||
left = vw - popover.width - 8;
|
||||
}
|
||||
if (left < 8) {
|
||||
left = 8;
|
||||
}
|
||||
setPos({ top: trigger.bottom + 4, left });
|
||||
}, []);
|
||||
|
||||
useEffect(() => {
|
||||
requestAnimationFrame(() => inputRef.current?.focus());
|
||||
}, []);
|
||||
@@ -61,7 +86,12 @@ export function HpAdjustPopover({ onAdjust, onClose }: HpAdjustPopoverProps) {
|
||||
return (
|
||||
<div
|
||||
ref={ref}
|
||||
className="absolute z-10 mt-1 rounded-md border border-border bg-background p-2 shadow-lg"
|
||||
className="fixed z-10 rounded-md border border-border bg-background p-2 shadow-lg"
|
||||
style={
|
||||
pos
|
||||
? { top: pos.top, left: pos.left }
|
||||
: { visibility: "hidden" as const }
|
||||
}
|
||||
>
|
||||
<div className="flex items-center gap-1">
|
||||
<Input
|
||||
|
||||
34
specs/024-fix-hp-popover-overflow/checklists/requirements.md
Normal file
34
specs/024-fix-hp-popover-overflow/checklists/requirements.md
Normal file
@@ -0,0 +1,34 @@
|
||||
# Specification Quality Checklist: Fix HP Popover Overflow
|
||||
|
||||
**Purpose**: Validate specification completeness and quality before proceeding to planning
|
||||
**Created**: 2026-03-09
|
||||
**Feature**: [spec.md](../spec.md)
|
||||
|
||||
## Content Quality
|
||||
|
||||
- [x] No implementation details (languages, frameworks, APIs)
|
||||
- [x] Focused on user value and business needs
|
||||
- [x] Written for non-technical stakeholders
|
||||
- [x] All mandatory sections completed
|
||||
|
||||
## Requirement Completeness
|
||||
|
||||
- [x] No [NEEDS CLARIFICATION] markers remain
|
||||
- [x] Requirements are testable and unambiguous
|
||||
- [x] Success criteria are measurable
|
||||
- [x] Success criteria are technology-agnostic (no implementation details)
|
||||
- [x] All acceptance scenarios are defined
|
||||
- [x] Edge cases are identified
|
||||
- [x] Scope is clearly bounded
|
||||
- [x] Dependencies and assumptions identified
|
||||
|
||||
## Feature Readiness
|
||||
|
||||
- [x] All functional requirements have clear acceptance criteria
|
||||
- [x] User scenarios cover primary flows
|
||||
- [x] Feature meets measurable outcomes defined in Success Criteria
|
||||
- [x] No implementation details leak into specification
|
||||
|
||||
## Notes
|
||||
|
||||
- All items pass. Spec is ready for `/speckit.clarify` or `/speckit.plan`.
|
||||
96
specs/024-fix-hp-popover-overflow/plan.md
Normal file
96
specs/024-fix-hp-popover-overflow/plan.md
Normal file
@@ -0,0 +1,96 @@
|
||||
# Implementation Plan: Fix HP Popover Overflow
|
||||
|
||||
**Branch**: `024-fix-hp-popover-overflow` | **Date**: 2026-03-09 | **Spec**: [spec.md](./spec.md)
|
||||
**Input**: Feature specification from `/specs/024-fix-hp-popover-overflow/spec.md`
|
||||
|
||||
## Summary
|
||||
|
||||
The HP adjustment popover (`HpAdjustPopover`) overflows the right edge of the viewport when the trigger element (HP display) is positioned near the right side of the combatant row. This causes the popover to be partially hidden and introduces a horizontal scrollbar. The fix adds viewport-aware horizontal positioning using a `useLayoutEffect` measurement pattern, consistent with the existing `ConditionPicker` component which already handles vertical overflow.
|
||||
|
||||
## Technical Context
|
||||
|
||||
**Language/Version**: TypeScript 5.8 (strict mode, verbatimModuleSyntax)
|
||||
**Primary Dependencies**: React 19, Tailwind CSS v4, Lucide React (icons)
|
||||
**Storage**: N/A (no storage changes — purely presentational fix)
|
||||
**Testing**: Vitest
|
||||
**Target Platform**: Web browser (desktop + mobile)
|
||||
**Project Type**: Web application (monorepo: apps/web + packages/domain + packages/application)
|
||||
**Performance Goals**: N/A (simple layout fix)
|
||||
**Constraints**: No heavy positioning libraries; lightweight DOM measurement only
|
||||
**Scale/Scope**: Single component fix (~20 lines changed)
|
||||
|
||||
## Constitution Check
|
||||
|
||||
*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.*
|
||||
|
||||
| Principle | Status | Notes |
|
||||
|-----------|--------|-------|
|
||||
| I. Deterministic Domain Core | PASS | No domain changes — adapter-layer only |
|
||||
| II. Layered Architecture | PASS | Change is entirely in the adapter layer (web UI component) |
|
||||
| III. Agent Boundary | N/A | No agent involvement |
|
||||
| IV. Clarification-First | PASS | Bug fix with clear spec, no ambiguity |
|
||||
| V. Escalation Gates | PASS | Scope matches spec exactly |
|
||||
| VI. MVP Baseline Language | PASS | No scope restrictions involved |
|
||||
| VII. No Gameplay Rules | PASS | No gameplay mechanics involved |
|
||||
|
||||
All gates pass. No violations.
|
||||
|
||||
## Project Structure
|
||||
|
||||
### Documentation (this feature)
|
||||
|
||||
```text
|
||||
specs/024-fix-hp-popover-overflow/
|
||||
├── plan.md # This file
|
||||
├── research.md # Phase 0 output
|
||||
└── tasks.md # Phase 2 output (/speckit.tasks command)
|
||||
```
|
||||
|
||||
### Source Code (repository root)
|
||||
|
||||
```text
|
||||
apps/web/src/components/
|
||||
├── hp-adjust-popover.tsx # FIX: Add viewport-aware horizontal positioning
|
||||
└── combatant-row.tsx # Context: ClickableHp renders popover (relative parent)
|
||||
```
|
||||
|
||||
**Structure Decision**: This is a single-component bug fix within the existing `apps/web` adapter layer. No new files, packages, or architectural changes needed.
|
||||
|
||||
## Design
|
||||
|
||||
### Root Cause
|
||||
|
||||
The `HpAdjustPopover` uses `absolute` positioning relative to its parent `ClickableHp` container (which has `relative` positioning). The popover renders to the right of the HP value display. When the HP display is near the right edge of the viewport (common since HP is in one of the rightmost grid columns), the popover extends beyond the viewport boundary.
|
||||
|
||||
The combatant list container has `overflow-y-auto` but no `overflow-x: hidden`, so the popover overflow manifests as a horizontal scrollbar on the page.
|
||||
|
||||
### Approach: position: fixed with useLayoutEffect viewport clamping
|
||||
|
||||
Switch the popover from `position: absolute` to `position: fixed` and use a `useLayoutEffect` hook to:
|
||||
1. Measure the trigger element's viewport rect via `parentElement.getBoundingClientRect()`
|
||||
2. Measure the popover's own width via `getBoundingClientRect()`
|
||||
3. Position the popover just below the trigger (`trigger.bottom + 4`)
|
||||
4. Clamp the `left` coordinate so the popover stays within viewport bounds (8px padding)
|
||||
5. Render with `visibility: hidden` until positioned to avoid a flash
|
||||
|
||||
This approach was chosen because the popover's scroll container ancestor (`overflow-y-auto`) clips absolutely-positioned children, making `position: absolute` with `right-0` or `translateX` ineffective — the popover gets clipped rather than repositioned.
|
||||
|
||||
### Implementation Detail
|
||||
|
||||
- Replace `absolute` with `fixed` positioning on the popover container
|
||||
- Add `useLayoutEffect` that reads `parentElement.getBoundingClientRect()` for the trigger position and `el.getBoundingClientRect()` for the popover width
|
||||
- Compute `left = trigger.left`, clamped to `[8, vw - popover.width - 8]` using `document.documentElement.clientWidth`
|
||||
- Set `top = trigger.bottom + 4`
|
||||
- Store computed `{ top, left }` in state; render `visibility: hidden` until positioned
|
||||
- Click-outside detection continues to work since `document.addEventListener("mousedown")` is not affected by positioning scheme
|
||||
|
||||
### Alternatives Considered
|
||||
|
||||
- **CSS-only `overflow-x: hidden`** on ancestor: Would clip the popover rather than reposition it, and could hide other legitimate content.
|
||||
- **Heavy positioning libraries (Floating UI, Popper)**: Overkill for a small popover with a simple overflow case.
|
||||
- **`position: absolute` with `right-0` flip** (ConditionPicker pattern): The popover's relative parent is too small (just the HP number), so `right-0` barely shifts the popover. Also, the scroll container's overflow clipping still clips the popover.
|
||||
- **`position: absolute` with `translateX`**: Same clipping issue — the scroll container clips the popover regardless of transform.
|
||||
|
||||
### Post-Design Constitution Re-check
|
||||
|
||||
All gates still pass. The fix is entirely in the adapter layer, uses no domain logic, and stays within spec scope.
|
||||
26
specs/024-fix-hp-popover-overflow/research.md
Normal file
26
specs/024-fix-hp-popover-overflow/research.md
Normal file
@@ -0,0 +1,26 @@
|
||||
# Research: Fix HP Popover Overflow
|
||||
|
||||
## R-001: Positioning strategy for popover inside scroll container
|
||||
|
||||
**Decision**: Use `position: fixed` with `useLayoutEffect` + `getBoundingClientRect` to position the popover relative to the viewport, escaping the scroll container's overflow clipping.
|
||||
|
||||
**Rationale**: The `ConditionPicker` component uses `position: absolute` with a `flipped` state for vertical overflow, but that pattern does not work for the HP popover's horizontal overflow. The popover sits inside a scroll container (`flex-1 overflow-y-auto`) which clips absolutely-positioned children. Both `right-0` alignment and `translateX` shifts are ineffective because the scroll container clips the overflowing content regardless. `position: fixed` removes the popover from the container's clipping context entirely.
|
||||
|
||||
**Alternatives considered**:
|
||||
- `position: absolute` with `right-0` flip (ConditionPicker pattern): The relative parent (`ClickableHp`) is too small, so `right-0` barely shifts the popover. The scroll container still clips it. Rejected.
|
||||
- `position: absolute` with `translateX(-Npx)`: Same clipping issue. Rejected.
|
||||
- `overflow-x: hidden` on scroll container: Clips the popover instead of showing a scrollbar — same visual problem. Rejected.
|
||||
- Floating UI / Popper.js: Heavy dependency for a simple case. Rejected.
|
||||
- CSS `anchor-positioning`: Not yet widely supported across browsers. Rejected.
|
||||
|
||||
## R-002: Viewport measurement for overflow detection
|
||||
|
||||
**Decision**: Use `document.documentElement.clientWidth` instead of `window.innerWidth` for viewport width measurement.
|
||||
|
||||
**Rationale**: When the popover overflows the viewport, `window.innerWidth` expands to include the scrollbar area, causing overflow detection to report no overflow. `document.documentElement.clientWidth` returns the CSS viewport width excluding scrollbars, giving an accurate measurement even when overflow has already occurred.
|
||||
|
||||
## R-003: Horizontal scrollbar cause
|
||||
|
||||
**Decision**: The fix addresses the root cause (popover overflow) rather than masking the symptom (scrollbar).
|
||||
|
||||
**Rationale**: The scrollbar appeared because the combatant list container (`flex-1 overflow-y-auto min-h-0`) has default `overflow-x: visible`. The absolutely-positioned popover extended beyond the viewport, creating the scrollbar. Switching to `position: fixed` removes the popover from the document flow entirely, so it cannot cause overflow on any ancestor container.
|
||||
65
specs/024-fix-hp-popover-overflow/spec.md
Normal file
65
specs/024-fix-hp-popover-overflow/spec.md
Normal file
@@ -0,0 +1,65 @@
|
||||
# Feature Specification: Fix HP Popover Overflow
|
||||
|
||||
**Feature Branch**: `024-fix-hp-popover-overflow`
|
||||
**Created**: 2026-03-09
|
||||
**Status**: Draft
|
||||
**Input**: User description: "When I try to apply a delta to health of a combatant, I do not see the whole popover. It vanishes out of view to the right. Also a scrollbar appears to the bottom. Expected behaviour would be that the whole popover menu is visible and no scrollbar appears."
|
||||
|
||||
## User Scenarios & Testing *(mandatory)*
|
||||
|
||||
### User Story 1 - HP popover stays fully visible (Priority: P1)
|
||||
|
||||
When a user clicks the HP display on a combatant row to open the HP adjustment popover, the popover must be fully visible within the viewport regardless of where the combatant row is positioned horizontally. Currently the popover overflows to the right, getting clipped by the viewport edge and causing a horizontal scrollbar.
|
||||
|
||||
**Why this priority**: This is the core bug — the popover is unusable when it extends beyond the viewport edge.
|
||||
|
||||
**Independent Test**: Open the HP adjustment popover for any combatant and verify the entire popover (input field, damage button, healing button) is visible without scrolling.
|
||||
|
||||
**Acceptance Scenarios**:
|
||||
|
||||
1. **Given** a combatant row where the HP display is near the right edge of the viewport, **When** the user clicks the HP value to open the adjustment popover, **Then** the popover is fully visible within the viewport with no part clipped or hidden.
|
||||
2. **Given** the HP adjustment popover is open, **When** the user looks at the page, **Then** no horizontal scrollbar appears on the page or any scrollable container.
|
||||
3. **Given** a combatant row where the HP display has plenty of space to the right, **When** the user clicks the HP value, **Then** the popover opens in its natural position (no unnecessary repositioning).
|
||||
|
||||
---
|
||||
|
||||
### User Story 2 - Popover remains usable with stat block panel open (Priority: P2)
|
||||
|
||||
The HP popover must remain fully visible and functional when the stat block side panel is open, which reduces the available width for the encounter tracker.
|
||||
|
||||
**Why this priority**: The stat block panel narrows the main content area, making the overflow more likely.
|
||||
|
||||
**Independent Test**: Open the stat block panel, then open the HP popover for a combatant and verify it is fully visible.
|
||||
|
||||
**Acceptance Scenarios**:
|
||||
|
||||
1. **Given** the stat block side panel is open (reducing the encounter tracker width), **When** the user opens the HP adjustment popover, **Then** the popover is fully visible within the available space.
|
||||
2. **Given** a narrow viewport (e.g., mobile width), **When** the user opens the HP adjustment popover, **Then** the popover is fully visible and usable.
|
||||
|
||||
### Edge Cases
|
||||
|
||||
- What happens when the viewport is extremely narrow (narrower than the popover width)? The popover should still not cause a horizontal scrollbar; it should align to stay within bounds as much as possible.
|
||||
- What happens when the browser window is resized while the popover is open? The popover should not overflow. Closing the popover on resize is acceptable.
|
||||
|
||||
## Requirements *(mandatory)*
|
||||
|
||||
### Functional Requirements
|
||||
|
||||
- **FR-001**: The HP adjustment popover MUST be fully visible within the viewport when opened, regardless of the horizontal position of the triggering element.
|
||||
- **FR-002**: Opening the HP adjustment popover MUST NOT cause a horizontal scrollbar to appear on the page or any scrollable ancestor container.
|
||||
- **FR-003**: The popover MUST remain fully functional (input field, damage button, healing button all accessible and interactive) in its adjusted position.
|
||||
- **FR-004**: The popover SHOULD open in its natural/default position when there is sufficient space, only adjusting when it would otherwise overflow.
|
||||
|
||||
## Success Criteria *(mandatory)*
|
||||
|
||||
### Measurable Outcomes
|
||||
|
||||
- **SC-001**: The HP adjustment popover is 100% visible within the viewport for all combatant rows, regardless of horizontal position.
|
||||
- **SC-002**: No horizontal scrollbar appears when the HP adjustment popover is open.
|
||||
- **SC-003**: All popover controls (input, damage button, healing button) remain interactive in the adjusted position.
|
||||
|
||||
## Assumptions
|
||||
|
||||
- The popover's own width is small enough to fit within the viewport at all supported screen sizes.
|
||||
- The fix should use lightweight viewport-aware positioning — no heavy positioning library is needed for a popover of this size.
|
||||
- The existing click-outside-to-close and keyboard interactions remain unchanged.
|
||||
91
specs/024-fix-hp-popover-overflow/tasks.md
Normal file
91
specs/024-fix-hp-popover-overflow/tasks.md
Normal file
@@ -0,0 +1,91 @@
|
||||
# Tasks: Fix HP Popover Overflow
|
||||
|
||||
**Input**: Design documents from `/specs/024-fix-hp-popover-overflow/`
|
||||
**Prerequisites**: plan.md (required), spec.md (required for user stories), research.md
|
||||
|
||||
**Tests**: Not explicitly requested in spec. No test tasks generated.
|
||||
|
||||
**Organization**: Tasks are grouped by user story to enable independent implementation and testing of each story.
|
||||
|
||||
## Format: `[ID] [P?] [Story] Description`
|
||||
|
||||
- **[P]**: Can run in parallel (different files, no dependencies)
|
||||
- **[Story]**: Which user story this task belongs to (e.g., US1, US2)
|
||||
- Include exact file paths in descriptions
|
||||
|
||||
---
|
||||
|
||||
## Phase 1: User Story 1 - HP popover stays fully visible (Priority: P1) 🎯 MVP
|
||||
|
||||
**Goal**: The HP adjustment popover repositions itself to stay within the viewport when it would otherwise overflow to the right.
|
||||
|
||||
**Independent Test**: Open the HP adjustment popover for any combatant. The entire popover (input, damage button, healing button) is visible without horizontal scrolling, even when the HP display is near the right edge of the viewport.
|
||||
|
||||
### Implementation for User Story 1
|
||||
|
||||
- [x] T001 [US1] Switch `HpAdjustPopover` in `apps/web/src/components/hp-adjust-popover.tsx` from `position: absolute` to `position: fixed` with viewport-aware positioning — use a `useLayoutEffect` to measure the trigger's viewport rect via `parentElement.getBoundingClientRect()` and the popover's width, then compute `top` (trigger bottom + 4px) and `left` (trigger left, clamped to `[8, vw - popover.width - 8]` using `document.documentElement.clientWidth`). Render with `visibility: hidden` until positioned to avoid flash. This escapes the scroll container's overflow clipping that made `position: absolute` approaches ineffective.
|
||||
- [x] T002 [US1] **Manual QA**: Verify that no horizontal scrollbar appears when the popover is open by ensuring the repositioned popover does not extend beyond `window.innerWidth`. Test with a combatant whose HP display is near the right viewport edge.
|
||||
|
||||
**Checkpoint**: At this point, the HP popover should be fully visible for all combatant rows. No horizontal scrollbar should appear.
|
||||
|
||||
---
|
||||
|
||||
## Phase 2: User Story 2 - Popover remains usable with stat block panel open (Priority: P2)
|
||||
|
||||
**Goal**: The HP popover stays fully visible even when the stat block side panel is open, which narrows the available width.
|
||||
|
||||
**Independent Test**: Open the stat block panel, then open the HP popover for a combatant. The popover is fully visible within the encounter tracker area.
|
||||
|
||||
### Implementation for User Story 2
|
||||
|
||||
- [x] T003 [US2] **Manual QA**: Verify that the viewport-aware positioning from T001 correctly handles the reduced-width scenario when the stat block panel is open in `apps/web/src/App.tsx`. The `getBoundingClientRect()` measurement should already account for the narrowed layout since it measures against the actual viewport. If the popover still overflows (e.g., because the stat block panel is an overlay and the popover's `right` edge is under it), adjust the overflow detection threshold to account for the panel width. Test at narrow viewport widths (e.g., 375px mobile).
|
||||
|
||||
**Checkpoint**: HP popover is fully visible at all viewport widths, including with stat block panel open.
|
||||
|
||||
---
|
||||
|
||||
## Phase 3: Polish & Cross-Cutting Concerns
|
||||
|
||||
- [x] T004 Run `pnpm check` to ensure all linting, formatting, type checks, and tests pass in the repository
|
||||
|
||||
---
|
||||
|
||||
## Dependencies & Execution Order
|
||||
|
||||
### Phase Dependencies
|
||||
|
||||
- **Phase 1 (US1)**: No dependencies — can start immediately
|
||||
- **Phase 2 (US2)**: Depends on Phase 1 (T001) — verifies the same positioning logic in a narrower viewport context
|
||||
- **Phase 3 (Polish)**: Depends on Phase 1 and Phase 2
|
||||
|
||||
### Parallel Opportunities
|
||||
|
||||
- T001 is the only implementation task; T002 and T003 are verification tasks that depend on T001
|
||||
- No parallel implementation opportunities (single-file change)
|
||||
|
||||
---
|
||||
|
||||
## Implementation Strategy
|
||||
|
||||
### MVP First (User Story 1 Only)
|
||||
|
||||
1. Complete T001: Add viewport-aware positioning
|
||||
2. Complete T002: Verify no horizontal scrollbar
|
||||
3. **STOP and VALIDATE**: Test with combatants at various positions
|
||||
4. If MVP is sufficient, proceed to Polish
|
||||
|
||||
### Incremental Delivery
|
||||
|
||||
1. T001 → Core fix (popover repositions)
|
||||
2. T002 → Validate fix works in standard layout
|
||||
3. T003 → Validate fix works with stat block panel
|
||||
4. T004 → Ensure merge gate passes
|
||||
|
||||
---
|
||||
|
||||
## Notes
|
||||
|
||||
- This is a single-component bug fix. The entire implementation is in `apps/web/src/components/hp-adjust-popover.tsx`.
|
||||
- The `ConditionPicker` pattern (`position: absolute` + `flipped` flag) was not usable here because the popover sits inside a scroll container that clips absolutely-positioned children. `position: fixed` was used instead.
|
||||
- The existing `ref` on the popover div is reused for measurement.
|
||||
- Commit after T001 as the atomic implementation change.
|
||||
Reference in New Issue
Block a user