Overlay clipping: _isScrollable should be root- and display-aware, and treat overflow: hidden as cropping
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.clientHeightTwo 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.
Source: shipshapecode/shepherd