Overlay clipping: _isScrollable should be root- and display-aware, and treat overflow: hidden as cropping

[chuckcarpenter]

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; clientHeight is 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.documentElement as a clipper;
• treat document.body as a clipper only when getComputedStyle(document.documentElement).overflowY !== 'visible', i.e. nothing propagated up from it;
• skip ancestors whose computed display is inline or contents;
• then accept hidden alongside auto, scroll, and clip.

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.