From cdd47f94ce503112cf6a810eac0a0594de28f5ad Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 17:13:29 +0000 Subject: [PATCH] fix(modal): stop the iframe offset walk at the window the overlay renders into `_getIframeOffset` accumulated frame offsets until `window.top`. The overlay is `position: fixed` inside the document Shepherd renders into, so when Shepherd itself runs inside a same-origin frame and targets an element a frame further down, the walk also added the hosting frame's offset and the opening drifted off the target by exactly that frame's position (confirmed in Chromium: top -> host frame at 50,100 -> content frame; opening at 90,180 instead of 40,80). The walk now stops at the modal container's window, and at the top window (its own parent) if the target is not nested under it at all. The iframe unit tests now swap only the target's ownerDocument, so the modal container stays in the real window, and assert the computed offsets rather than just visibility. A Cypress spec covers both the single-frame and the nested-frame layouts on real layout. Fixes #3478 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011RqamMhESzSaWmRHLw2Vai --- shepherd.js/src/components/shepherd-modal.ts | 13 +- .../cypress/examples/iframes/content.html | 21 ++ .../test/cypress/examples/iframes/host.html | 27 +++ .../test/cypress/examples/iframes/nested.html | 22 ++ .../test/cypress/integration/iframes.cy.js | 83 +++++++ .../test/cypress/utils/overlay-openings.js | 26 +++ .../unit/components/shepherd-modal.spec.js | 204 ++++++++++-------- 7 files changed, 305 insertions(+), 91 deletions(-) create mode 100644 shepherd.js/test/cypress/examples/iframes/content.html create mode 100644 shepherd.js/test/cypress/examples/iframes/host.html create mode 100644 shepherd.js/test/cypress/examples/iframes/nested.html create mode 100644 shepherd.js/test/cypress/integration/iframes.cy.js create mode 100644 shepherd.js/test/cypress/utils/overlay-openings.js diff --git a/shepherd.js/src/components/shepherd-modal.ts b/shepherd.js/src/components/shepherd-modal.ts index b7cf55318..9f239d2ca 100644 --- a/shepherd.js/src/components/shepherd-modal.ts +++ b/shepherd.js/src/components/shepherd-modal.ts @@ -353,10 +353,21 @@ export function createShepherdModal(container: HTMLElement): ShepherdModalAPI { if (!el) return offset; + // The overlay is `position: fixed` in the document it is rendered into, so + // frame offsets only accumulate up to that document's window. Walking on + // to `window.top` would also add the offsets of the frames hosting + // Shepherd itself, pushing the opening off the target (#3478). + const modalWindow = container.ownerDocument.defaultView; let targetWindow: Window | null = el.ownerDocument.defaultView; try { - while (targetWindow && targetWindow !== window.top) { + while ( + targetWindow && + targetWindow !== modalWindow && + // The top window is its own parent; stop there if the target is not + // nested under the modal's window at all. + targetWindow !== targetWindow.parent + ) { const targetIframe = targetWindow?.frameElement; if (targetIframe) { diff --git a/shepherd.js/test/cypress/examples/iframes/content.html b/shepherd.js/test/cypress/examples/iframes/content.html new file mode 100644 index 000000000..2458f4934 --- /dev/null +++ b/shepherd.js/test/cypress/examples/iframes/content.html @@ -0,0 +1,21 @@ + + + + + + + +
Target
+ + diff --git a/shepherd.js/test/cypress/examples/iframes/host.html b/shepherd.js/test/cypress/examples/iframes/host.html new file mode 100644 index 000000000..b6ae24740 --- /dev/null +++ b/shepherd.js/test/cypress/examples/iframes/host.html @@ -0,0 +1,27 @@ + + + + + + + + + + + + + diff --git a/shepherd.js/test/cypress/examples/iframes/nested.html b/shepherd.js/test/cypress/examples/iframes/nested.html new file mode 100644 index 000000000..c4d1cd770 --- /dev/null +++ b/shepherd.js/test/cypress/examples/iframes/nested.html @@ -0,0 +1,22 @@ + + + + + + + + + + + diff --git a/shepherd.js/test/cypress/integration/iframes.cy.js b/shepherd.js/test/cypress/integration/iframes.cy.js new file mode 100644 index 000000000..7d6a45816 --- /dev/null +++ b/shepherd.js/test/cypress/integration/iframes.cy.js @@ -0,0 +1,83 @@ +import setupTour from '../utils/setup-tour'; +import overlayOpenings from '../utils/overlay-openings'; + +// End-to-end guard for #3478. The overlay opening for a target in a nested +// frame must be offset only by the frames between the target and the document +// Shepherd renders into, not by the frames hosting Shepherd itself. Unit tests +// can only mock the frame chain, so the arithmetic is checked here. +describe('modal overlay for targets inside iframes', () => { + let tour; + + afterEach(() => { + tour?.complete(); + }); + + /** Waits for a same-origin frame's window to satisfy `isReady`. */ + const frameWindow = ($frame, isReady) => + cy + .wrap($frame) + .its('0.contentWindow') + .should((win) => expect(isReady(win)).to.be.ok); + + const expectOpeningOnTarget = (shepherdWindow) => { + const doc = shepherdWindow.document; + const content = doc.getElementById('content'); + const frameRect = content.getBoundingClientRect(); + const targetRect = content.contentDocument + .querySelector('.target') + .getBoundingClientRect(); + const [opening] = overlayOpenings(doc); + + expect(opening.x).to.be.closeTo(frameRect.left + targetRect.left, 1); + expect(opening.y).to.be.closeTo(frameRect.top + targetRect.top, 1); + expect(opening.height).to.be.closeTo(40, 1); + }; + + const startTour = (shepherdWindow) => { + const target = shepherdWindow.document + .getElementById('content') + .contentDocument.querySelector('.target'); + + tour = setupTour( + shepherdWindow.Shepherd, + { scrollTo: false }, + () => [ + { + attachTo: { element: target, on: 'bottom' }, + id: 'iframe-target', + text: 'Inside an iframe' + } + ], + { useModalOverlay: true } + ); + tour.start(); + }; + + const contentReady = (win) => + win.Shepherd && + win.document + .getElementById('content') + ?.contentDocument?.querySelector('.target'); + + it('offsets the opening by the target frame when Shepherd is in the top document', () => { + cy.visit('/test/cypress/examples/iframes/host.html'); + + cy.window() + .should((win) => expect(contentReady(win)).to.be.ok) + .then((win) => { + startTour(win); + cy.wait(250).then(() => expectOpeningOnTarget(win)); + }); + }); + + it('does not add the offset of the frame hosting Shepherd', () => { + cy.visit('/test/cypress/examples/iframes/nested.html'); + + cy.get('#host').then(($host) => { + frameWindow($host, contentReady).then((win) => { + startTour(win); + cy.wait(250).then(() => expectOpeningOnTarget(win)); + }); + }); + }); +}); diff --git a/shepherd.js/test/cypress/utils/overlay-openings.js b/shepherd.js/test/cypress/utils/overlay-openings.js new file mode 100644 index 000000000..ad1a2b5b7 --- /dev/null +++ b/shepherd.js/test/cypress/utils/overlay-openings.js @@ -0,0 +1,26 @@ +/** + * Reads the cutouts of the modal overlay back out of its svg path. + * + * The path is the full-viewport rect followed by one sub-path per opening, + * each starting `M{x + radius},{y}` and reaching its bottom edge with the + * first `V{y + height}`. Only valid for openings without a corner radius. + * + * @param {Document} doc The document the overlay is rendered in + * @returns {{ x: number, y: number, height: number }[]} + */ +export default function overlayOpenings(doc) { + const d = doc + .querySelector('.shepherd-modal-overlay-container path') + .getAttribute('d'); + + return d + .split('Z') + .slice(1) + .filter(Boolean) + .map((subPath) => { + const [, x, y, bottom] = subPath.match( + /^M([-\d.e]+),([-\d.e]+).*?V([-\d.e]+)/ + ); + return { x: Number(x), y: Number(y), height: Number(bottom) - Number(y) }; + }); +} diff --git a/shepherd.js/test/unit/components/shepherd-modal.spec.js b/shepherd.js/test/unit/components/shepherd-modal.spec.js index e6c5dee88..fe9c7bbd1 100644 --- a/shepherd.js/test/unit/components/shepherd-modal.spec.js +++ b/shepherd.js/test/unit/components/shepherd-modal.spec.js @@ -1270,41 +1270,50 @@ describe('components/ShepherdModal', () => { }); describe('_getIframeOffset (via setupForStep)', function () { - it('accumulates offset when element is inside an iframe', () => { - const modal = createShepherdModal(container); - const rafSpy = vi + let rafSpy; + let restoreWindow = []; + + beforeEach(() => { + rafSpy = vi .spyOn(window, 'requestAnimationFrame') .mockImplementation(() => 1); + }); - const targetEl = document.createElement('div'); - container.appendChild(targetEl); + afterEach(() => { + rafSpy.mockRestore(); + restoreWindow.forEach((restore) => restore()); + restoreWindow = []; + }); - // Simulate the element being inside an iframe by mocking ownerDocument.defaultView - const fakeIframe = document.createElement('iframe'); - Object.defineProperty(fakeIframe, 'getBoundingClientRect', { - value: () => ({ - top: 10, - left: 20, - width: 100, - height: 100, - x: 20, - y: 10 - }) + function makeIframe({ top, left }, { scrollTop = 0, scrollLeft = 0 } = {}) { + const iframe = document.createElement('iframe'); + Object.defineProperty(iframe, 'getBoundingClientRect', { + value: () => ({ top, left, width: 100, height: 100, x: left, y: top }) }); - Object.defineProperty(fakeIframe, 'scrollTop', { value: 5 }); - Object.defineProperty(fakeIframe, 'scrollLeft', { value: 3 }); - - const fakeChildWindow = { - frameElement: fakeIframe, - parent: window - }; - - const origDescriptor = Object.getOwnPropertyDescriptor( - targetEl.ownerDocument, - 'defaultView' + Object.defineProperty(iframe, 'scrollTop', { value: scrollTop }); + Object.defineProperty(iframe, 'scrollLeft', { value: scrollLeft }); + return iframe; + } + + // Overrides a property of the real window for the duration of one test. + function stubWindow(key, value) { + const descriptor = Object.getOwnPropertyDescriptor(window, key); + Object.defineProperty(window, key, { value, configurable: true }); + restoreWindow.push(() => + descriptor + ? Object.defineProperty(window, key, descriptor) + : delete window[key] ); - Object.defineProperty(targetEl.ownerDocument, 'defaultView', { - value: fakeChildWindow, + } + + // A target living in the document of `frameWindow`. Only the target's + // ownerDocument is swapped: the modal container stays in the real + // document, which is what decides where the frame walk has to stop. + function startStepInFrame(modal, frameWindow) { + const targetEl = document.createElement('div'); + container.appendChild(targetEl); + Object.defineProperty(targetEl, 'ownerDocument', { + value: { defaultView: frameWindow }, configurable: true }); @@ -1315,46 +1324,88 @@ describe('components/ShepherdModal', () => { step._resolveAttachToOptions(); step.target = targetEl; - // This triggers _styleForStep -> _getIframeOffset, which should - // walk up through fakeChildWindow and accumulate the iframe offset modal.setupForStep(step); - // Restore defaultView before any assertions (jsdom needs it for instanceof checks) - if (origDescriptor) { - Object.defineProperty( - targetEl.ownerDocument, - 'defaultView', - origDescriptor - ); - } else { - Object.defineProperty(targetEl.ownerDocument, 'defaultView', { - value: window, - configurable: true - }); - } + return modal.getElement().querySelector('path').getAttribute('d'); + } + + it('offsets the opening by the frame holding the target', () => { + const modal = createShepherdModal(container); + const contentWindow = { + frameElement: makeIframe( + { top: 10, left: 20 }, + { scrollTop: 5, scrollLeft: 3 } + ), + parent: window + }; + + const d = startStepInFrame(modal, contentWindow); expect(modal.getElement()).toHaveClass('shepherd-modal-is-visible'); + // The target itself measures 0x0 at the origin in happy-dom, so the + // opening sits exactly at the accumulated frame offset. + expect(d).toContain('M23,15'); + }); - rafSpy.mockRestore(); + it('sums the offsets of every frame between the target and the modal', () => { + const modal = createShepherdModal(container); + const middleWindow = { + frameElement: makeIframe({ top: 100, left: 50 }), + parent: window + }; + const contentWindow = { + frameElement: makeIframe({ top: 60, left: 30 }), + parent: middleWindow + }; + + expect(startStepInFrame(modal, contentWindow)).toContain('M80,160'); + }); + + it('stops at the window the modal renders into when Shepherd is itself framed', () => { + // Regression test for https://github.com/shipshapecode/shepherd/issues/3478 + // top -> host frame (Shepherd, at 50,100) -> content frame (target, at + // 30,60 within the host). The overlay is fixed inside the host document, + // so only the content frame's offset applies. + const modal = createShepherdModal(container); + const topWindow = {}; + topWindow.parent = topWindow; + stubWindow('frameElement', makeIframe({ top: 100, left: 50 })); + stubWindow('parent', topWindow); + stubWindow('top', topWindow); + + const contentWindow = { + frameElement: makeIframe({ top: 60, left: 30 }), + parent: window + }; + + const d = startStepInFrame(modal, contentWindow); + + expect(d).toContain('M30,60'); + // Walking on to window.top adds the host frame's offset as well. + expect(d).not.toContain('M80,160'); + }); + + it('stops at the top window when the target is not below the modal window', () => { + const modal = createShepherdModal(container); + const topWindow = {}; + topWindow.parent = topWindow; + const siblingWindow = { + frameElement: makeIframe({ top: 60, left: 30 }), + parent: topWindow + }; + + expect(() => startStepInFrame(modal, siblingWindow)).not.toThrow(); }); it('handles cross-origin iframe SecurityError gracefully', () => { // Regression test for https://github.com/shipshapecode/shepherd/issues/3087 // When Shepherd is loaded in a nested cross-origin iframe, accessing // window.frameElement throws a SecurityError due to Same-Origin Policy. - // This test ensures the error is caught and handled gracefully. + // This test ensures the error is caught and handled gracefully, keeping + // the offset accumulated before the cross-origin boundary. const modal = createShepherdModal(container); - const rafSpy = vi - .spyOn(window, 'requestAnimationFrame') - .mockImplementation(() => 1); - - const targetEl = document.createElement('div'); - container.appendChild(targetEl); - - // Simulate a cross-origin iframe by making frameElement access throw SecurityError - const fakeChildWindow = { + const crossOriginWindow = { get frameElement() { - // Simulate browser's SecurityError when accessing cross-origin frameElement const error = new Error( 'Blocked a frame with origin "https://example.com" from accessing a cross-origin frame.' ); @@ -1363,45 +1414,18 @@ describe('components/ShepherdModal', () => { }, parent: window }; + const contentWindow = { + frameElement: makeIframe({ top: 60, left: 30 }), + parent: crossOriginWindow + }; - const origDescriptor = Object.getOwnPropertyDescriptor( - targetEl.ownerDocument, - 'defaultView' - ); - Object.defineProperty(targetEl.ownerDocument, 'defaultView', { - value: fakeChildWindow, - configurable: true - }); - - const tour = new Tour({ useModalOverlay: true }); - const step = new Step(tour, { - attachTo: { element: targetEl, on: 'bottom' } - }); - step._resolveAttachToOptions(); - step.target = targetEl; - - // This should NOT throw an error, even though frameElement access throws SecurityError + let d; expect(() => { - modal.setupForStep(step); + d = startStepInFrame(modal, contentWindow); }).not.toThrow(); - // Restore defaultView before any assertions - if (origDescriptor) { - Object.defineProperty( - targetEl.ownerDocument, - 'defaultView', - origDescriptor - ); - } else { - Object.defineProperty(targetEl.ownerDocument, 'defaultView', { - value: window, - configurable: true - }); - } - expect(modal.getElement()).toHaveClass('shepherd-modal-is-visible'); - - rafSpy.mockRestore(); + expect(d).toContain('M30,60'); }); }); });