Fix/scroll lock propagation target

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## 2.3.0
     BREAKING: a modal dialog now freezes the page behind it by default; pass backgroundScroll: true to restore the old behaviour (non-modal dialogs are unchanged)
     fix: locking the page no longer shifts fixed elements sideways, because the scrollbar gutter is reserved instead of padded away
+    fix: the lock now lands on whichever element the viewport takes its overflow from, so a page whose <body> carries an overflow and a definite height is no longer thrown back to the top when a modal opens
     fix: an anchored popover now follows its anchor out of the viewport instead of sticking to the viewport edge
     feat: a dark palette behind data-theme="dark", with data-theme="auto" following prefers-color-scheme; opt-in, so an existing site is unchanged on upgrade
     feat: every colour in the stylesheet is now a --dialogify-* custom property, so a page can retheme the dialog without overriding rules

README.md

@@ -144,4 +144,3 @@ npm test
 
 Fork the repo, branch off master, and open a pull request. Contributions are
 welcome.
-

README.zh-TW.md

@@ -140,4 +140,3 @@ npm test
 ```
 
 Fork 專案、從 master 開新 branch，然後發 pull request。歡迎一起參與。
-

src/js/core.js

@@ -1293,35 +1293,56 @@ const supportsScrollbarGutter =
     typeof window.CSS.supports === 'function' &&
     window.CSS.supports('scrollbar-gutter', 'stable');
 
+// The viewport takes its overflow from the root element, unless that computes
+// to `visible`, in which case the value is taken from <body> instead. Writing
+// the lock onto the other element makes the one that was being propagated from
+// a scroll container in its own right, which collapses the viewport's scrollable
+// area and throws the page back to the top -- so the lock has to land on
+// whichever element the viewport is already listening to.
+function scrollLockTarget() {
+    const html = window.document.documentElement;
+    const body = window.document.body;
+
+    if (!body) {
+        return html;
+    }
+
+    const overflow = window.getComputedStyle(html).overflow;
+
+    return !overflow || overflow === 'visible' ? body : html;
+}
+
 // Hiding the overflow takes the classic scrollbar away, and the page jumps
 // sideways to fill the gap it leaves behind. `scrollbar-gutter` keeps that gap
-// reserved, which — unlike padding compensation — also holds `position: fixed`
-// headers still. It is only worth applying when a classic scrollbar is really
-// there: overlay scrollbars leave no gap, and reserving one would shift the
-// page the other way.
+// reserved, which -- unlike padding compensation -- also holds `position: fixed`
+// headers still. It belongs on the root element whichever element carries the
+// lock, and is only worth applying when a classic scrollbar is really there:
+// overlay scrollbars leave no gap, and reserving one would shift the page the
+// other way.
 function lockScroll() {
     if (scrollLocks++) {
         return;
     }
 
     const html = window.document.documentElement;
-    const style = html.style;
+    const target = scrollLockTarget();
     scrollLockStyles = {
-        overflow: style.overflow,
-        gutter: style.getPropertyValue('scrollbar-gutter'),
-        paddingRight: style.paddingRight
+        target,
+        overflow: target.style.overflow,
+        gutter: html.style.getPropertyValue('scrollbar-gutter'),
+        paddingRight: html.style.paddingRight
     };
 
     const scrollbarWidth = window.innerWidth - html.clientWidth;
     if (scrollbarWidth > 0) {
         if (supportsScrollbarGutter) {
-            style.setProperty('scrollbar-gutter', 'stable');
+            html.style.setProperty('scrollbar-gutter', 'stable');
         } else {
-            style.paddingRight = `${scrollbarWidth}px`;
+            html.style.paddingRight = `${scrollbarWidth}px`;
         }
     }
 
-    style.overflow = 'hidden';
+    target.style.overflow = 'hidden';
 }
 
 function unlockScroll() {
@@ -1330,7 +1351,7 @@ function unlockScroll() {
     }
 
     const style = window.document.documentElement.style;
-    style.overflow = scrollLockStyles.overflow;
+    scrollLockStyles.target.style.overflow = scrollLockStyles.overflow;
     style.paddingRight = scrollLockStyles.paddingRight;
     if (scrollLockStyles.gutter) {
         style.setProperty('scrollbar-gutter', scrollLockStyles.gutter);

test/features.test.js

@@ -1013,20 +1013,24 @@ describe('exit animation', () => {
 
 describe('background scroll lock', () => {
     const html = document.documentElement;
+    const body = document.body;
 
-    // jsdom reports no scrollbar (innerWidth === clientWidth), so the gutter
-    // compensation stays out of the way of these assertions.
+    // The viewport reads its overflow from <html> only when that is not
+    // `visible`; by default the value comes from <body>, which is where the
+    // lock has to land. jsdom reports no scrollbar (innerWidth === clientWidth),
+    // so the gutter compensation stays out of the way of these assertions.
     afterEach(() => {
         html.style.overflow = '';
         html.style.removeProperty('scrollbar-gutter');
         html.style.paddingRight = '';
+        body.style.overflow = '';
     });
 
     it('freezes the page behind a modal dialog by default', () => {
         const d = new Dialogify('content');
         d.showModal();
 
-        expect(html.style.overflow).toBe('hidden');
+        expect(body.style.overflow).toBe('hidden');
         d.close();
     });
 
@@ -1035,35 +1039,35 @@ describe('background scroll lock', () => {
         d.showModal();
         d.close();
 
-        expect(html.style.overflow).toBe('');
+        expect(body.style.overflow).toBe('');
     });
 
     it('honours backgroundScroll: true on a modal dialog', () => {
         const d = new Dialogify('content', { backgroundScroll: true });
         d.showModal();
 
-        expect(html.style.overflow).toBe('');
+        expect(body.style.overflow).toBe('');
     });
 
     it('leaves the page alone for a non-modal dialog', () => {
         const d = new Dialogify('content');
         d.show();
 
-        expect(html.style.overflow).toBe('');
+        expect(body.style.overflow).toBe('');
     });
 
     it('freezes the page for a non-modal dialog that opts in', () => {
         const d = new Dialogify('content', { backgroundScroll: false });
         d.show();
 
-        expect(html.style.overflow).toBe('hidden');
+        expect(body.style.overflow).toBe('hidden');
         d.close();
     });
 
     it('does not lock toasts out of the page', () => {
         Dialogify.toast('hi');
 
-        expect(html.style.overflow).toBe('');
+        expect(body.style.overflow).toBe('');
     });
 
     it('keeps the page frozen until the last stacked dialog closes', () => {
@@ -1073,18 +1077,33 @@ describe('background scroll lock', () => {
         second.showModal();
 
         second.close();
-        expect(html.style.overflow).toBe('hidden');
+        expect(body.style.overflow).toBe('hidden');
 
         first.close();
-        expect(html.style.overflow).toBe('');
+        expect(body.style.overflow).toBe('');
     });
 
     it('restores whatever the page had set itself', () => {
+        body.style.overflow = 'scroll';
+
+        const d = new Dialogify('content');
+        d.showModal();
+        expect(body.style.overflow).toBe('hidden');
+
+        d.close();
+        expect(body.style.overflow).toBe('scroll');
+    });
+
+    it('locks the root element when the viewport reads its overflow from it', () => {
         html.style.overflow = 'scroll';
 
         const d = new Dialogify('content');
         d.showModal();
+
+        // Locking <body> here would leave the viewport scrollable, and would
+        // turn <body> into a scroll container of its own.
         expect(html.style.overflow).toBe('hidden');
+        expect(body.style.overflow).toBe('');
 
         d.close();
         expect(html.style.overflow).toBe('scroll');