Repository navigation
fix(modal): treat overflow: hidden as cropping, skip inline and display: contents ancestors - #3557
Merged
Merged
Conversation
…lies to the viewport `_isScrollable` treated <body> as a scroll container whenever its computed overflow-y was `auto`/`scroll`. But while <html> is `overflow: visible`, body's overflow propagates to the viewport and body's used overflow is `visible`, so it crops nothing. With body sized to the viewport (`height: 100%`), scrolling a target below the initial fold into view moved body's rect off-screen and the overlay opening collapsed to zero height, covering the target it was meant to highlight. <html> is now never treated as a clipper (its overflow always applies to the viewport), and <body> only when <html>'s overflow is not `visible`. Fixes #1984 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011RqamMhESzSaWmRHLw2Vai
…ay: contents ancestors `_isScrollable` excluded `overflow-y: hidden`, so a highlight inside a collapsed accordion, carousel track or programmatically scrolled hidden pane got a full-size opening over content the user cannot see. It already accepted `clip`, which crops identically, so the handling was inconsistent. `hidden` is now accepted alongside `auto`, `scroll` and `clip`. To keep that from regressing boxes whose computed overflow does not crop: - the root elements are already handled (#1984), which covers a propagated `body { overflow: hidden }`; - non-replaced inlines (overflow does not apply) and `display: contents` (no box, 0x0 rect at the origin) are skipped. The `scrollHeight >= clientHeight` term is dropped: per CSSOM-View the scrolling area is at least the padding box, so it was true for every element and filtered nothing. Fixes #3484 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011RqamMhESzSaWmRHLw2Vai
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe modal clipping predicate now treats non-visible vertical overflow as clipping, except for elements with ChangesModal overlay clipping
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No identified issue prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (1)
🛟 Help
|
…opagation Any `contain` value on <html> or <body> stops body's overflow propagating to the viewport, leaving body a real scroll container even while <html> is `overflow: visible` (verified in Chromium for layout, paint, size, style, content, strict and inline-size). Only skip body when neither is contained. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011RqamMhESzSaWmRHLw2Vai
…n-0vsqdj-3484 # Conflicts: # shepherd.js/test/unit/components/shepherd-modal.spec.js
Body's overflow propagates to the viewport only while the root's overflow
is `visible` on both axes. With `html { overflow-x: clip }`, overflow-y
still computes to `visible` but body keeps its own scroll container
(verified in Chromium), so check overflowX too.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011RqamMhESzSaWmRHLw2Vai
…n-0vsqdj-3484 # Conflicts: # shepherd.js/test/unit/components/shepherd-modal.spec.js
…-0vsqdj-3484 # Conflicts: # shepherd.js/src/components/shepherd-modal.ts # shepherd.js/test/cypress/integration/root-overflow.cy.js # shepherd.js/test/unit/components/shepherd-modal.spec.js
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3484.
Change
_isScrollableinshepherd.js/src/components/shepherd-modal.ts:hiddenalongsideauto,scrollandclip.hiddencrops exactly like the others; it just can't be scrolled by the user. Excluding it was also inconsistent with already acceptingclip.display: inlineanddisplay: contentsancestors. Overflow doesn't apply to non-replaced inlines, whose rect is the union of their line boxes.contentsgenerates no box, so its rect is 0×0 at the origin and would zero every opening beneath it.scrollHeight >= clientHeight. Per CSSOM-View the scrolling area is at least the padding box, so the term was true for every element and filtered nothing.body { height: 100vh; overflow: hidden }case from the issue is already handled by fix(modal): don't clip highlights by root elements whose overflow applies to the viewport #3556's root-awareness.Verified in Chromium
main(with #3556)height:0; overflow:hiddenaccordionbody{height:100vh;overflow:hidden}, target scrolled into view{ y: 202, height: 40 }stays correct)overflow:hiddeninline spandisplay: contents; overflow: hiddenhtml{overflow:hidden}), target below its foldTests
hidden,clipandautocropping;visiblenot cropping; andinline/contentsnever cropping. Thehiddencase fails onmain.mockOverflowgained an optionaldisplaysmap.overflow-clipping.cy.jswithexamples/overflow-clipping.html(accordion, inline, contents) and adds the propagatedoverflow: hiddenbody case toroot-overflow.cy.js. As the issue notes, these layouts can't be expressed in happy-dom. The same example pages were also driven in Chromium through Playwright; the results are the table above.vitest run: 300/300 pass; eslint, prettier (js/ts), andtscare clean.🤖 Generated with Claude Code
https://claude.ai/code/session_011RqamMhESzSaWmRHLw2Vai
Summary by CodeRabbit