Skip to content

Added new modifier to Focus plugin#4578

Open
tuimz wants to merge 4 commits intoalpinejs:mainfrom
tuimz:add-new-modifier-focus-plugin
Open

Added new modifier to Focus plugin#4578
tuimz wants to merge 4 commits intoalpinejs:mainfrom
tuimz:add-new-modifier-focus-plugin

Conversation

@tuimz
Copy link

@tuimz tuimz commented Mar 17, 2025

Underlaying focus-trap library supports disabling the scroll to currently focussed element.

This specific usecase is required in my case for Filament which uses Livewire/Alpine under the hood for its modals.

If you have a very lenghty modal with a vertical scrollbar for its content, and you have a focussed element outside your viewport and you click anywhere in the modal (thus blurring the element triggering some Livewire changes) the x-trap gets re-initialized and automatically scrolls you to the focussed element.

This PR adds a new modifier (incl its docs) which effectively sets preventScroll in the options of the underlaying focus-trap instance.

See also: https://github.com/focus-trap/focus-trap#:~:text=and%20selector%20strings.-,preventScroll,-%7Bboolean%7D%3A%20By

Tim Aerdts and others added 4 commits March 17, 2025 15:30
Added new modifier to Focus plugin
f4c4c25 
Underlaying focus-trap library supports disabling the scroll to currently focussed element.
Reverted whitespace change
9db5209
Prevent scroll should infact be true
b16f711
@calebporzio @claude
Review fixes: add test, fix merge conflicts, improve docs
b7b3bd0 
- Added Cypress test for preventscroll modifier
- Resolved merge conflict with inert feature from main
- Removed reference to focus-trap internals in docs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@calebporzio
Copy link
Collaborator

calebporzio commented Feb 9, 2026

PR Review: #4578 — Added new modifier to Focus plugin

Type: Feature
Verdict: Needs discussion

What's happening (plain English)

  1. When you use x-trap on a dialog/modal, the underlying focus-trap library calls .focus() on the first focusable element when the trap activates.
  2. The browser's native .focus() auto-scrolls to bring the focused element into view.
  3. If you have a long, scrollable modal and the focused element is off-screen (say, below the fold), activating the trap scrolls the modal content to that element — which is disorienting.
  4. This PR adds .preventscroll modifier to x-trap which passes { preventScroll: true } to focus-trap's options, which in turn passes it to the native .focus() call — preventing the auto-scroll.

The code change itself is 4 lines — just a passthrough of a focus-trap option. It works correctly.

The naming problem

Here's what concerns me. Alpine already has two scroll-related modifiers/methods in the focus plugin:

API What it does
x-trap.noscroll Prevents page scrolling behind the modal (sets overflow: hidden on <html>)
$focus.noscroll() Prevents browser from scrolling to the focused element
x-trap.preventscroll (this PR) Prevents browser from scrolling to the focused element inside the trap

So .noscroll and .preventscroll on the same directive do completely different things, but their names suggest they're related. A user seeing x-trap.noscroll.preventscroll would be very confused about what each does.

The $focus magic uses .noscroll() for the same concept this PR calls .preventscroll. Ideally they'd share a name, but .noscroll is already taken on x-trap for a different purpose.

Other approaches considered

  1. Rename to .nofocusscroll — More descriptive: "don't scroll when focusing." Avoids confusion with .noscroll. This is probably the best name if you want to merge this.
  2. Make .noscroll do both — Have .noscroll prevent page scrolling AND focus scrolling. Users wanting to prevent scroll probably want both. Simpler API, but changes existing behavior (currently .noscroll doesn't prevent focus scrolling).
  3. Don't merge — The use case is niche (Livewire re-renders in long modals). Zero community engagement (no reactions, no comments). Could be solved in userland by the Filament/Livewire layer.

Changes Made

  • Added Cypress test for the .preventscroll modifier (verified it fails without the fix, passes with it)
  • Fixed merge conflict with the inert feature from main (the PR was based on stale code — the old inert handling was duplicated with main's new onPostActivate approach)
  • Improved docs: removed reference to "focus-trap" internals, fixed grammar

Test Results

24/24 focus tests passing (23 existing + 1 new). CI was passing on the original PR. Build succeeds.

Code Review

  • packages/focus/src/index.js:119-121: The implementation is clean — 3 lines, checks modifier, sets option. No concerns.
  • The PR had no tests. I added one that creates a scrollable container with a button below the fold and verifies scrollTop stays at 0 when the trap activates.
  • Docs referenced "focus-trap" by name — fixed to abstract away the implementation detail.

Security

No security concerns identified.

Verdict

Needs discussion. The implementation is correct and minimal, but the naming needs a decision before merging. Having .noscroll and .preventscroll on the same directive doing different things is confusing and will generate issues/questions.

My recommendation: if you want this feature, rename it to .nofocusscroll to clearly distinguish it from .noscroll. Or consider making .noscroll handle both concerns (preventing page scroll AND focus scroll), since users who prevent scroll likely want both behaviors.

Zero community engagement (no reactions/comments in ~11 months) suggests this isn't a widely-needed feature, so "no for now" is also reasonable.

Reviewed by Claude

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tuimz @calebporzio