shipshapecode / shipshapecode/shepherd

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

Open
#3,484 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
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; 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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.