shipshapecode / shipshapecode/shepherd
Iframe offset walk terminates at window.top instead of Shepherd's own window
Nobody has claimed this yet.
- Dominant language
- JavaScript
- Stars
- 13.8k
- Forks
- 658
- Avg merge
- 4d 5h
- Merged PRs (30d)
- 15
Description
Split out of #1692, where it was noticed while answering the same-origin iframe question. Not reproduced — this is inferred from reading the code, so the first job is to confirm or kill it.
The suspicion
_getIframeOffset in shepherd.js/src/components/shepherd-modal.ts walks the frame chain like this:
let targetWindow: Window | null = el.ownerDocument.defaultView;
while (targetWindow && targetWindow !== window.top) {
const targetIframe = targetWindow?.frameElement;
if (targetIframe) {
const rect = targetIframe.getBoundingClientRect();
offset.top += rect.top + targetIframe.scrollTop;
offset.left += rect.left + targetIframe.scrollLeft;
}
targetWindow = targetWindow.parent;
}
The loop terminates at window.top — the outermost frame. But the offsets are applied to the modal overlay, which is position: fixed inside the document Shepherd itself renders into, not necessarily the top one.
So when Shepherd runs inside a same-origin iframe and targets an element in a frame nested deeper, the walk should stop at Shepherd's own window. Terminating at window.top looks like it keeps accumulating past that point, adding the intermediate frame's offset to a cutout that is already positioned relative to it — pushing the overlay opening off the target by roughly that frame's position.
The straightforward case (Shepherd in the top document, target one iframe down) is unaffected, since there window.top is Shepherd's window. That is also the only case the tests cover.
Why it wasn't caught
The existing tests in shepherd.js/test/unit/components/shepherd-modal.spec.js mock the frame chain in jsdom but only assert that the modal became visible and that cross-origin traversal doesn't throw. Neither asserts the resulting offset values, so any arithmetic error passes.
Suggested work
- Build a real two-level same-origin repro (top → frame A hosting Shepherd → frame B holding the target) and confirm or refute the drift.
- If confirmed, terminate the walk at the window owning the modal container rather than
window.top.Touralready knows its container (shepherd.js/src/tour.ts), so the owning window is reachable. - Add tests that assert the computed offset numbers, not just visibility. That gap is the reason this went unnoticed.
Cross-origin nesting stays out of scope and unsupported — see #1692 and #3087.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in shepherd.js/src/components/shepherd-modal.ts and inspect _getIframeOffset, then review the container ownership in shepherd.js/src/tour.ts. Run shepherd.js/test/unit/components/shepherd-modal.spec.js and build a two-level same-origin iframe reproduction to verify the offset arithmetic. Done means the suspected drift is confirmed or refuted, with computed-offset assertions added if confirmed; cross-origin nesting remains out of scope.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, typescript
- Domain
- frontend, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100