Skip to content

Fix mobile input focus loss during tooltip repositioning#3441

Open
serendipitous-syntax wants to merge 2 commits into
shipshapecode:mainfrom
serendipitous-syntax:serendipitous-syntax/shepherd-mobile-input-focus
Open

Fix mobile input focus loss during tooltip repositioning#3441
serendipitous-syntax wants to merge 2 commits into
shipshapecode:mainfrom
serendipitous-syntax:serendipitous-syntax/shepherd-mobile-input-focus

Conversation

@serendipitous-syntax
Copy link
Copy Markdown

@serendipitous-syntax serendipitous-syntax commented May 13, 2026

Fixes #1143.

This keeps the existing focus-after-render behavior for the initial step render, but skips the delayed step.el.focus() on later Floating UI auto-update/reposition calls. That prevents mobile viewport/keyboard resize updates from stealing focus back from an input attached to the active step.

The regression test covers the focus behavior directly: the step element is focused once after initial render, then a focused input remains focused after a later reposition update.

Checks run:

  • CYPRESS_INSTALL_BINARY=0 npx pnpm install --lockfile=false
  • npx pnpm -F shepherd.js exec vitest run test/unit/utils/floating-ui.spec.js
  • npx pnpm -F shepherd.js test:unit:ci
  • npx pnpm -F shepherd.js lint:js
  • npx pnpm -F shepherd.js lint:prettier
  • npx pnpm -F shepherd.js types:check
  • npx pnpm -F shepherd.js build
  • git diff --check

Notes:

  • CYPRESS_INSTALL_BINARY=0 npx pnpm install --frozen-lockfile is currently blocked by a duplicate @vitest/pretty-format@4.1.5 mapping in pnpm-lock.yaml.
  • Cypress browser tests were not run.

Summary by CodeRabbit

  • Bug Fixes

    • Tooltip focus now runs only once after positioning, preventing repeated focus steals during repositioning.
  • Tests

    • Added unit tests verifying tooltip positioning and that focus is applied only once.
  • Chores

    • Pinned a development dependency version to stabilize linting tool behavior.

Review Change Stack

@vercel
Copy link
Copy Markdown

vercel Bot commented May 13, 2026

Someone is attempting to deploy a commit to the shipshapecode Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 13, 2026

📝 Walkthrough

Walkthrough

The pull request gates tooltip focus behavior to occur only after the first render. A shouldFocusAfterRender flag is introduced to setupTooltip, passed through setPosition, and reset after the first invocation so that subsequent autoUpdate callbacks do not steal focus. Tests validate focus is set exactly once after initial positioning. The package.json pins eslint-plugin-cypress to 5.3.0.

Changes

Focus-gate for tooltip positioning

Layer / File(s) Summary
Focus-gate flag and positioning integration
shepherd.js/src/utils/floating-ui.ts
Introduces shouldFocusAfterRender boolean flag initialized to true in setupTooltip. The flag is passed to setPosition and then set to false after the first call, ensuring the focus sequence (setting tabIndex and calling focus()) runs only on the initial render, not on subsequent positioning updates.
Test setup and focus behavior validation
shepherd.js/test/unit/utils/floating-ui.spec.js
Adds Vitest test suite with hoisted mocks for @floating-ui/dom (autoUpdate and computePosition). Sets up fake timers, DOM fixtures (target, input, stepElement), and a test that asserts focus is called exactly once after initial setupTooltip invocation and remains unchanged when autoUpdate callbacks trigger additional positioning flushes.

Dev dependency pin

Layer / File(s) Summary
Pin eslint-plugin-cypress
shepherd.js/package.json
Pins devDependencies.eslint-plugin-cypress from ^5.3.0 to 5.3.0.

Sequence Diagram

sequenceDiagram
  participant setupTooltip
  participant setPosition
  participant computePosition
  participant stepElement

  setupTooltip->>setPosition: first call (shouldFocusAfterRender=true)
  setPosition->>computePosition: computePosition(target, stepElement)
  computePosition-->>setPosition: placement & coords
  setPosition->>stepElement: set tabIndex & focus()
  setupTooltip->>setupTooltip: shouldFocusAfterRender = false

  Note over setupTooltip: autoUpdate callback fires

  setupTooltip->>setPosition: subsequent call (shouldFocusAfterRender=false)
  setPosition->>computePosition: computePosition(target, stepElement)
  computePosition-->>setPosition: placement & coords
  Note over setPosition: skip focus sequence
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A rabbit nudges the tooltip bright,
Focus lands once on first sunlight,
Auto-updates buzz and hum,
But the tab stays still—no stolen thumb,
Hooray for quiet typing night.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The eslint-plugin-cypress version pin is unrelated to the primary objective of fixing mobile input focus loss and appears to be an incidental change. Remove the eslint-plugin-cypress version change or provide a clear justification for this unrelated dependency modification in the PR description.
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing mobile input focus loss that occurs during tooltip repositioning by introducing a shouldFocusAfterRender flag.
Linked Issues check ✅ Passed The code changes address the core objective from issue #1143 by implementing a conditional focus behavior that prevents Floating UI reposition calls from stealing focus after the initial render.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
shepherd.js/package.json (1)

96-96: ⚡ Quick win

Clarify the rationale for pinning eslint-plugin-cypress to version 5.3.0.

This change pins eslint-plugin-cypress to exactly 5.3.0, removing the caret range (^) used by other dev dependencies. While version 5.3.0 has no known security advisories, it is 2 major versions behind the current release (6.4.1). If this pin is intentional—such as to resolve the pnpm install conflicts mentioned in the PR summary—document the reason in the commit message or add a comment explaining why this package uses a different versioning strategy than peers.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shepherd.js/package.json` at line 96, The package.json change pins
eslint-plugin-cypress to exactly 5.3.0; either document why (e.g., to resolve
pnpm install conflicts) or revert to caret semantics to match peers. Update the
commit message and PR description to state the rationale for pinning
eslint-plugin-cypress@5.3.0 (and any follow-up plan to upgrade), or change the
package.json entry for eslint-plugin-cypress back to using ^5.3.0 if the pin was
accidental; ensure the explanation references eslint-plugin-cypress and
package.json so reviewers can find the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@shepherd.js/package.json`:
- Line 96: The package.json change pins eslint-plugin-cypress to exactly 5.3.0;
either document why (e.g., to resolve pnpm install conflicts) or revert to caret
semantics to match peers. Update the commit message and PR description to state
the rationale for pinning eslint-plugin-cypress@5.3.0 (and any follow-up plan to
upgrade), or change the package.json entry for eslint-plugin-cypress back to
using ^5.3.0 if the pin was accidental; ensure the explanation references
eslint-plugin-cypress and package.json so reviewers can find the intent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7b013fa0-e732-4b70-bbad-3f8dce2673c0

📥 Commits

Reviewing files that changed from the base of the PR and between 63b7bef and 2a23169.

📒 Files selected for processing (1)
  • shepherd.js/package.json

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.

Targeted input loses focus on android and ios devices

1 participant