Overlay clipping: _isScrollable should be root- and display-aware, and treat overflow: hidden as cropping
Nobody has claimed this yet.
Assessment
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Newbie friendliness
- 52/100
- Issue type
- Bug
- Clarity
- Clearly specified
- Activity status
- Quiet
- Tech stack
- javascript, typescript
- Domain
- frontend
Research direction
Start in shepherd.js/src/components/shepherd-modal.ts at _isScrollable, then inspect the scrollTo handling in step.ts and the existing Cypress suite. Reproduce the overflow-hidden, propagated body overflow, inline, and contents cases in a browser rather than relying on happy-dom. Done means the Cypress coverage demonstrates correct clipping and the overlay opening remains visible for the described layouts.
Written by the indexing model from the issue text.
Description
Split out of a review discussion on #3483.
_isScrollable in shepherd.js/src/components/shepherd-modal.ts decides which ancestors crop a highlighted element, and it currently reads:
overflowY !== 'hidden' && overflowY !== 'visible' && el.scrollHeight >= el.clientHeight
Two things are wrong with it, in opposite directions.
It misses ancestors that really do crop
overflow-y: hidden clips its overflowing descendants — it just isn't user-scrollable. Excluding it means the overlay cuts a full-size hole over content the user cannot see: a highlight inside a collapsed overflow: hidden; height: 0 accordion, a carousel track, or a hidden pane that has been scrolled programmatically. Note the predicate already accepts clip, which crops identically and differs only in not establishing a scroll container, so the current handling is internally inconsistent.
But simply dropping the exclusion regresses real layouts
getComputedStyle(el).overflowY === 'hidden' is not the same claim as "this box clips". Reproduced in Chrome:
body { height: 100vh; overflow: hidden; margin: 0 } /* html untouched */
Because html's overflow is visible, body's overflow propagates to the viewport and body's used value becomes visible. Body does not clip, and content below its 100vh box paints normally — but getComputedStyle(body).overflowY still returns "hidden".
Our own scrollTo option (element.scrollIntoView(), step.ts) then scrolls the viewport, which still works under a propagated overflow: hidden. Measured after scrolling a static, in-flow target into view:
body rect: top -998, bottom -554 (entirely off-screen)
target rect: top 202, bottom 242 (fully visible)
excluding hidden (today): chain [] -> opening { y: 202, height: 40 }
including hidden: chain ['body'] -> opening { y: 202, height: 0 }
A fully visible target loses its opening and ends up under the dark overlay. This reaches the attachTo target too, not only extraHighlights, since _getScrollParent supplies targetScrollParent.
Two smaller cases share the root cause — computed hidden on a box that doesn't clip:
display: inline— overflow does not apply to non-replaced inlines;clientHeightis 0 and the rect is the union of line boxes.display: contents— generates no box at all;getBoundingClientRect()is 0×0 at the origin, which would zero every opening beneath it.
The scrollHeight >= clientHeight term is inert
Per CSSOM-View the scrolling area is at least the padding box, so this is true by construction for every element that has a box, and 0 >= 0 for every element that doesn't. It filters nothing and is not the safety valve it looks like. (It is >=, not >, so it doesn't even exclude non-overflowing containers.)
Suggested shape
Make the predicate root- and display-aware rather than just broadening it:
- never treat
document.documentElementas a clipper; - treat
document.bodyas a clipper only whengetComputedStyle(document.documentElement).overflowY !== 'visible', i.e. nothing propagated up from it; - skip ancestors whose computed
displayisinlineorcontents; - then accept
hiddenalongsideauto,scroll, andclip.
Filtering just html/body restores the failing case above to { y: 202, height: 40 }, so the narrowing does work.
Testing note
None of this can be unit tested as things stand. Nothing in the unit or Cypress suites sets overflow-y to hidden, the modal spec mocks getComputedStyle wholesale, and happy-dom has no layout engine — so overflow propagation, display: inline line boxes, and display: contents box generation cannot be expressed there. This needs Cypress coverage, which is a large part of why it was kept out of #3483.
- Dominant language
- JavaScript
- Stars
- 13.8k
- Forks
- 658
- Avg merge
- 7d 5h
- Merged PRs (30d)
- 18
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from shipshapecode/shepherd
-
Docs: rendering framework components as step content, and why DOM injection belongs in when.show Opendocumentation
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
shipshapecode/shepherd#3479 ·
-
Difficulty 3/5 1-2 days Newbie friendliness 52/100
shipshapecode/shepherd#3505 ·
-
bug
Difficulty 4/5 3-5 days Newbie friendliness 52/100
shipshapecode/shepherd#3478 ·
-
Difficulty 4/5 3-5 days Newbie friendliness 48/100
shipshapecode/shepherd#3442 · 1 comment · 2 reactions ·
-
Difficulty 5/5 Over a week Newbie friendliness 25/100
shipshapecode/shepherd#3146 · 2 comments ·
All issues in shipshapecode/shepherd
Similar issues
-
bug confirmed issue
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
open-webui/open-webui#30750 · 1 comment ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 70/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
-
Mend: dependency security vulnerability untriaged
Difficulty 2/5 1-3 hours Newbie friendliness 70/100