Skip to content

chore: scroll should not trigger press - #3695

Open
stevekaplan123 wants to merge 1 commit into
masterfrom
bug/sc-46595/mobile-web-search-result-card-clicks
Open

chore: scroll should not trigger press#3695
stevekaplan123 wants to merge 1 commit into
masterfrom
bug/sc-46595/mobile-web-search-result-card-clicks

Conversation

@stevekaplan123

@stevekaplan123 stevekaplan123 commented Sep 6, 2026

Copy link
Copy Markdown
Member

Description

This PR introduces several UI improvements to the search results page in mobile web. The main one is that when users scroll, they first have to click, but we don't want any click behavior (i.e. the card gets highlighted upon being clicked) to trigger if they're scrolling.

Code Changes

The solution uses timers:
Two timers in [SearchResultCard.jsx:19-84]:

showTimer — delays turning the highlight on (100ms). Starts on touchstart. If the finger moves >10px first, the timer is cancelled and the card never lights up. That's what stops scrolls from highlighting cards.

hideTimer — delays turning the highlight off (150ms floor). Set on touchend. Without it, a fast tap would show and hide the highlight in a few milliseconds — it'd be invisible. This timer keeps the highlight on screen for at least 150ms from when it appeared.

The edge case they cover together: if the finger lifts before 100ms, then showTimer hasn't fired yet, so onEnd shows the highlight immediately, then hideTimer holds it 150ms.

The two timers pull in opposite directions on purpose: showTimer says don't light up too eagerly (that's the scroll fix), and hideTimer says once you have lit up, stay long enough to be seen (that's the fast-tap fix).

@stevekaplan123
stevekaplan123 requested review from YishaiGlasner and yitzhakc and a lite review from Copilot September 6, 2026 11:17
@stevekaplan123
stevekaplan123 marked this pull request as ready for review September 6, 2026 11:17
@gitvelocity-reviewer

Copy link
Copy Markdown

📊 Code Quality Score: 24/100

40 × 0.6 (Medium ESF) = 24

Category Score Factors
🔭 Scope 7/20 Three files are touched: static/css/s2.css (hover and pressed rules), static/js/SearchResultCard.jsx (the usePressState hook body), and the new test file static/js/tests/searchResultCardPressState.test.js. All changes are contained within the search result card subsystem.
🏗️ Architecture 5/20 usePressState is refactored in place: four closure-local variables (startPos, showTimer, hideTimer, shownAt) replace the single startPos ref, and onCancel is split from onEnd. No new module boundary is introduced and no dependency changes.
⚙️ Implementation 11/20 usePressState now runs a four-state machine: idle → pending (after touchstart), pending → shown (after PRESS_DELAY_MS or on touchend if still pending), shown → idle (after PRESS_MIN_MS or immediately if already shown long enough). clearTimers and hide are called on onStart to handle rapid re-touch. onEnd computes remaining = PRESS_MIN_MS - (Date.now() - shownAt) to decide whether to clear immediately or schedule hideTimer.
⚠️ Risk 4/20 Affects touch interaction on every SearchResultCard render on mobile. A regression shows as a card that never lights up on tap or one that stays lit after a scroll; neither path corrupts data or breaks navigation. The change is reverted by a single file revert.
✅ Quality 12/15 searchResultCardPressState.test.js adds 154 lines covering six paths: scroll-never-shows, small-drift-is-tap, touchcancel-clears, delay-gate boundary (PRESS_DELAY_MS - 1 vs PRESS_DELAY_MS), long-press stays lit, and quick-tap flashes for PRESS_MIN_MS. Date.now is mocked alongside jest.advanceTimersByTime to keep the timing assertions deterministic. The rapid double-tap path (new touchstart while hideTimer is running) is not covered.
🔒 Perf / Security 1/5 All four touch listeners are registered with { passive: true }, which allows the browser to begin scrolling without waiting for the JS handler. No security surface is touched.

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

Scored by GitVelocity · How are scores calculated?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The behavior change is localized, matches the stated requirements, and is supported by focused tests covering key timing and gesture edge cases.

Pull request overview

This PR refines mobile-web interaction on the search results page by preventing “pressed/highlight” feedback from appearing during scroll gestures, while still ensuring quick taps get visible feedback.

Changes:

  • Reworked SearchResultCard touch handling to delay showing the pressed state, cancel it on scroll-like movement, and enforce a minimum visible pressed duration for fast taps.
  • Updated styling so the pressed visual matches the intended spec (shadow-only), gates :hover to true-hover devices, and disables the default mobile tap highlight.
  • Added Jest/jsdom coverage for tap vs scroll vs cancel behaviors around the pressed state timing.
File summaries
File Description
static/js/SearchResultCard.jsx Implements timer-based touch press detection to avoid scroll-triggered pressed state while keeping tap feedback.
static/js/tests/searchResultCardPressState.test.js Adds automated coverage for press-state timing and scroll/tap/cancel edge cases.
static/css/s2.css Aligns pressed/hover visuals and suppresses native tap highlight to match the intended UI feedback.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread static/css/s2.css
.searchResultCard:hover {
/* Hover and pressed are one and the same state in the spec: a darker shadow, no change of
background. `.is-pressed` is set by usePressState in SearchResultCard.jsx for taps only. */
.searchResultCard.is-pressed {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: the CSS changes here — worth calling out for reviewers who might read .searchResultCard.is-pressed and :hover as two different visual treatments being merged into one by accident.

They're not accidentally the same — the spec calls for hover (mouse) and pressed (touch tap) to look identical: same box-shadow, no background-color change. Previously :hover gave a shadow and .is-pressed gave a grey background — two different effects for what the design treats as one state. This PR unifies the visual style and keeps two separate triggers for it:

  • .is-pressed — set by JS (usePressState in SearchResultCard.jsx) for touch only.
  • :hover — native CSS, now wrapped in @media (hover: hover) so touch devices (which lack a real hover concept but still fire :hover and can get it stuck on the last-tapped element) never match this rule.

So mouse users get the shadow via :hover, touch users get the identical shadow via .is-pressed, and neither path can leak into the other device type.

— Claude

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants