feat(consent): require scrolling the terms before enabling "I agree"

[jasoneplumb]

Summary

The first-run consent dialog now disables "I agree — continue" until the user scrolls to the bottom of the terms — so acceptance follows actually viewing the full Privacy Policy + Terms of Use.

Changes

src/consent.ts:

• Accept button renders disabled; a hint ("Please scroll to the bottom to continue.") shows beneath the actions while it's locked.
• Unlocks when #consent-body is scrolled to the bottom (scrollTop + clientHeight >= scrollHeight - 4), hiding the hint and removing the listeners.
• Fits-without-scrolling fallback: if the terms don't overflow (tall viewport), it enables immediately — checked right after mount.
• Resize re-check: rotating to a layout where the content fits also unlocks it.
• Terms region is tabindex=0 and gets initial focus (keyboard users can scroll to read/unlock instead of landing on a disabled button). Decline stays enabled.
• resize listener cleaned up on close, including decline-before-scroll.

src/style.css:

• #consent-accept:disabled — dimmed + not-allowed cursor.
• #consent-scroll-hint — hidden by default; shown via JS only while locked.

Test plan

• type-check (app + worker), lint, test (96), build — all green; markup/logic verified in the bundle
• Manual browser check (vitest is pure-function-only here, so the DOM behavior needs a real browser): on a short viewport the button is disabled + hint visible → scroll to bottom → button enables + hint hides; on a tall viewport where the terms fit, the button is enabled immediately; declining before scrolling still works.

Closes #225

🤖 Generated with Claude Code

[claude]

Code Review

PR: feat(consent): require scrolling the terms before enabling "I agree"

Overview

This PR adds a scroll-to-accept gate on the consent dialog so users must scroll to the bottom of the Privacy Policy + Terms before the "I agree" button becomes active. The implementation is clean and well-thought-out — fallback for tall viewports, resize re-check, and keyboard accessibility are all handled. Two issues worth addressing before merge.

---

Bug: scroll listener not removed on early decline

src/consent.ts — cleanup()

When the user clicks Decline (or presses Escape) before scrolling to the bottom, cleanup(false) only removes the resize listener. The scroll listener on the terms body div is never removed:

function cleanup(accepted: boolean): void {
  window.removeEventListener('resize', onScrollOrResize);
  // body scroll listener is still attached here
  overlay.remove();
  ...
}

After overlay.remove() the entire DOM subtree is detached, but because the scroll listener still holds a reference to both body and the onScrollOrResize closure, the detached element is kept alive in memory. Since consent is shown only once per session this won't cause observable problems, but it's a minor leak and worth being consistent about.

Fix:

function cleanup(accepted: boolean): void {
  window.removeEventListener('resize', onScrollOrResize);
  body.removeEventListener('scroll', onScrollOrResize);   // add this
  overlay.remove();
  if (accepted) recordConsent();
  resolve(accepted);
}

This also makes cleanup and unlock symmetrical — both paths tear down both listeners.

---

Minor UX: focus doesn't move to the accept button after unlock

After unlock() fires (scroll reaches the bottom), focus stays on the terms div. A keyboard user who just scrolled down to unlock the button now has to Tab forward to reach it. Moving focus programmatically on unlock would make the flow feel complete:

const unlock = (): void => {
  acceptBtn.disabled = false;
  hint.style.display = 'none';
  body.removeEventListener('scroll', onScrollOrResize);
  window.removeEventListener('resize', onScrollOrResize);
  acceptBtn.focus();   // move focus now that the button is interactive
};

This is a soft suggestion rather than a blocker — the current behavior is functional — but it improves keyboard flow noticeably.

---

Everything else looks good

• Fallback for tall viewports — calling atBottom() synchronously after mount is correct; browsers resolve layout before returning from scrollHeight/clientHeight queries.
• Resize re-check — catching orientation changes that make content fit is a nice touch.
• { passive: true } on the scroll listener — correct.
• tabindex="0" + aria-label on the terms div — correct; keyboard users can scroll to read and unlock.
• CSS changes — minimal and appropriate; display: none default for #consent-scroll-hint is the right pattern for a JS-shown element.
• The test plan clearly identifies what needs manual browser verification.

---

Summary: The listener cleanup in cleanup() is the one concrete change I'd make before merging. The focus-after-unlock point is worth considering. Otherwise this is solid.

🤖 Generated with Claude Code

[jasoneplumb]

Both addressed: cleanup() now removes the body scroll listener too (symmetric teardown, covers decline-before-scroll), and unlock() focuses the accept button so keyboard users land on it right after scrolling to the bottom. Thanks.

[claude]

Code Review — feat(consent): require scrolling the terms before enabling "I agree"

Overall: This is a solid, well-thought-out feature. The symmetric listener teardown, passive scroll listener, fits-without-scrolling fallback, and resize re-check all show good attention to edge cases. One confirmed focus-order bug and two minor accessibility gaps are noted below.

---

Bug: body.focus() overwrites acceptBtn.focus() on tall viewports

src/consent.ts — end of the setup block

When the terms fit without scrolling, unlock() is called immediately — which correctly calls acceptBtn.focus(). But body.focus() is unconditionally called afterward, silently moving focus back to the scrollable div even though the accept button is already enabled.

// Current code (problematic order):
if (atBottom()) {
  unlock();       // ← calls acceptBtn.focus()
} else {
  ...
}
// ... event listeners ...
body.focus();     // ← overwrites acceptBtn.focus() in the tall-viewport path

On a tall viewport the user ends up tabbing from a focused-but-unnecessary scroll region to the accept button, instead of landing directly on the already-enabled button.

Suggested fix — move body.focus() into the else branch so the two code paths have distinct focus targets:

if (atBottom()) {
  unlock(); // unlock() already calls acceptBtn.focus()
} else {
  hint.style.display = 'block';
  body.addEventListener('scroll', onScrollOrResize, { passive: true });
  window.addEventListener('resize', onScrollOrResize);
  body.focus(); // Only needed when the button is locked
}

---

Minor: aria-live missing on scroll hint

When unlock() runs (either via scroll or immediately), the hint disappears and the button becomes interactive — but screen readers get no announcement of that state change. Adding aria-live="polite" to #consent-scroll-hint (or to #consent-actions) would let assistive technology announce when the button becomes available.

<p id="consent-scroll-hint" aria-live="polite">Please scroll to the bottom to continue.</p>

---

Nit: window resize listener could be passive

The scroll listener correctly uses { passive: true }. The resize listener doesn't need it (resize has no cancelable default action), but marking it passive is a good consistency habit and future-proofs the code:

window.addEventListener('resize', onScrollOrResize, { passive: true });

---

What's done well

• Symmetric cleanup in cleanup() — explicitly removes both listeners regardless of which code path closes the modal (accept, decline, overlay-click, or Escape). No leaks.
• No-op safety — removeEventListener is called in unlock() even in the immediate path before listeners are added; that's harmless and keeps the logic simple.
• 4 px scroll tolerance — correctly handles sub-pixel rounding that would otherwise leave the button permanently locked a fraction of a pixel from the bottom.
• CSS-first display: none — the hint starts hidden via CSS so it's in the right state before JS runs; JS only ever turns it on then off.
• tabindex="0" on #consent-body — keyboard users can scroll to read without a mouse; focus then moves to the accept button on unlock. The intent is right even if the tall-viewport path currently misdirects focus (see bug above).
• { passive: true } on scroll — correct for a pure read-only scroll handler.

---

Test coverage

The PR description acknowledges no automated DOM test is possible given the project's pure-function vitest setup, and the manual test plan is thorough and well-documented. That's appropriate given the project's testing conventions.

---

Summary: One real bug (focus misdirection on tall viewports) that's a one-line fix, and two low-severity accessibility improvements. The core logic is correct and well-structured.

🤖 Generated with Claude Code