shipshapecode / shipshapecode/shepherd
Overlay clipping: _isScrollable should be root- and display-aware, and treat overflow: hidden as cropping
Nobody has claimed this yet.
- Dominant language
- JavaScript
- Stars
- 13.8k
- Forks
- 658
- Avg merge
- 4d 5h
- Merged PRs (30d)
- 15
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.
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.
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.
Assessment
- Tech stack
- javascript, typescript
- Domain
- frontend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 52/100