shipshapecode / shipshapecode/shepherd

Iframe offset walk terminates at window.top instead of Shepherd's own window

Open
#3,478 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
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
  1. Build a real two-level same-origin repro (top → frame A hosting Shepherd → frame B holding the target) and confirm or refute the drift.
  2. If confirmed, terminate the walk at the window owning the modal container rather than window.top. Tour already knows its container (shepherd.js/src/tour.ts), so the owning window is reachable.
  3. 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.