Conversation
Add regression test that demonstrates issue ing-bank#2349 - when multiple overlays with preventsScroll:true are shown, body margins should not accumulate. Currently this test fails, showing the bug exists. Related to ing-bank#2349
Move body margin management from OverlayController to OverlaysManager singleton to prevent margin accumulation when multiple overlays with preventsScroll:true are shown. The manager now tracks how many overlays are preventing scroll via a counter. Only the first overlay captures and compensates body margin; subsequent overlays increment the counter without modifying margin. When hiding, only the last overlay restores the original margin. This aligns with existing scroll-lock class management and follows the pattern recommended in issue ing-bank#2349. Fixes ing-bank#2349
SebayK
requested review from
Knorgias,
gerjanvangeest,
narzac,
okadurin,
ryubro and
tlouisse
as code owners
|
There was a problem hiding this comment.
Pull request overview
This PR addresses Issue #2349 by moving body margin compensation for preventsScroll: true overlays from OverlayController into the OverlaysManager singleton, aiming to prevent margin accumulation when multiple scroll-preventing overlays are nested.
Changes:
- Centralize body size/margin compensation logic in
OverlaysManagerand introduce a counter to avoid repeated margin adjustments for nestedpreventsScrolloverlays. - Update
OverlayControllerto delegate body size keeping to the manager (including during teardown). - Add/adjust tests to verify non-accumulating margins for nested scroll-preventing overlays and to validate teardown behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/ui/components/overlays/test/OverlayController.test.js | Adds regression coverage for nested preventsScroll margin behavior and updates tests to reflect manager-owned margin state. |
| packages/ui/components/overlays/src/OverlaysManager.js | Introduces counter-based centralized body margin compensation via requestToKeepBodySize(). |
| packages/ui/components/overlays/src/OverlayController.js | Removes per-controller margin handling and delegates to OverlaysManager; adds teardown-phase delegation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback to fix edge cases where teardown() could cause
negative counter values and break margin restoration:
1. Add guard to only decrement __preventScrollCount when > 0
2. Gate _keepBodySize('teardown') behind isShown check to prevent
unbalanced decrements when teardown() is called on hidden overlays
3. Add tests for teardown() and updateConfig() on hidden overlays
These changes ensure the counter stays balanced even when:
- teardown() is called on never-shown overlays
- updateConfig() internally calls teardown() on hidden overlays
- teardown() is called after hide() has already run
Addresses review comments in ing-bank#2701
#2349
singleton to prevent margin accumulation when multiple overlays with
preventsScroll:true are shown.
The manager now tracks how many overlays are preventing scroll via a
counter. Only the first overlay captures and compensates body margin;
subsequent overlays increment the counter without modifying margin.
When hiding, only the last overlay restores the original margin.