Skip to content

fix(mvc.$): make $(window) hold the window - #3542

Merged
kumilingus merged 2 commits into
clientIO:masterfrom
MartinKanera:fix/mvc-dom-window
Oct 12, 2026
Merged

kumilingus merged 2 commits into
clientIO:masterfrom
MartinKanera:fix/mvc-dom-window

Conversation

@MartinKanera

@MartinKanera MartinKanera commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

mvc.$(window) has returned an empty collection since 4.0.0. A window has no nodeType, so the Dom constructor treats it as an array-like and copies window.length items, which are the page's frames. Events bound through it go nowhere, and the el === window branches of width()/height() are unreachable through $(window). With this change, $(window) holds the window, so the event methods and width()/height() work on it.

Changes

  • Dom constructor: holds any window, an iframe's contentWindow included, as a one-item collection. The check uses jQuery's window test (obj != null && obj === obj.window) right after the DOM-element branch, so on a page with iframes $(window) holds the window instead of its frames.
  • width() and height(): test for any window instead of el === window, so an iframe's window measures its own document.
  • Only on(), one(), off(), width() and height() support a window, and a comment on the constructor branch says so. The prop() aliases (innerWidth(), outerWidth(), scrollTop(), ...) read undefined and their setters write a plain property onto the window. Most other element methods throw a TypeError.
  • Methods that pass a window on to $(): $(el).add(window) adds the window. closest(window), appendTo(window), prependTo(window) and a view with el: window throw a TypeError, as they already did on a page with an iframe.
  • Other calls that change: inside an iframe, $(window.parent) holds the parent window instead of the parent's frames, and a one() handler bound through $([window]) now removes itself after it runs.
  • Tests: a $(window) module that pins the constructor (alone, on a page with an iframe, and an iframe's window), width()/height() on the window and on an iframe's window, and on(), one() and off().
  • A patch changeset for @joint/core.

Verification

  • yarn workspace @joint/core lint: pass
  • yarn workspace @joint/core test: pass (Karma 2146/2146, geometry 350/350, vectorizer 453/453, mocha, qunit, TypeScript; coverage thresholds included)
  • Test first: the 8 new tests failed before the fix. With only the constructor branch, the iframe width()/height() test still failed with a TypeError. A width() that returns window.innerWidth fails the scrollbar test, and so does a height() that returns window.innerHeight.
  • yarn changeset status --since=upstream/master: @joint/core patch
  • Browser: no separate check, since nothing in core calls $(window)

@MartinKanera MartinKanera changed the title fix(mvc.$): make $(window) hold the window fix(mvc.$): throw an error for a window Oct 12, 2026
@MartinKanera
MartinKanera marked this pull request as ready for review October 12, 2026 07:08
@kumilingus
kumilingus requested a balanced review from Copilot October 12, 2026 10:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unguarded collection-building APIs can still contain windows, causing regressions in width() and height().

1 open finding
What changed in this PR

This PR makes mvc.$ reject window objects instead of silently wrapping them.

Changes:

  • Detect windows in direct and array-like inputs.
  • Remove window-specific measurement handling.
  • Add QUnit coverage and a patch changeset.
File Description
Dom.mjs Rejects window-containing inputs.
methods.mjs Removes viewport measurement branches.
mvc.Dom.js Tests direct, iframe, and array-contained windows.
Changeset Documents the behavioral change.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/joint-core/src/mvc/Dom/methods.mjs Outdated
@MartinKanera
MartinKanera marked this pull request as draft October 12, 2026 14:31
Since 4.0.0, $(window) fell through to the array-like branch and
copied window.length items, which counts the page's frames: it held
nothing on a page without iframes and the frames' windows otherwise.
The event methods bound nothing and the window branches of width()
and height() were unreachable.

The constructor now holds any window as a one-item collection, and
width()/height() accept any window, so an iframe's window measures its
own document. The other methods still have no window case.
@MartinKanera MartinKanera changed the title fix(mvc.$): throw an error for a window fix(mvc.$): make $(window) hold the window Oct 12, 2026
The scrollable-window helper scrolled the window back to the top after
each test, but no test scrolls the window.
@MartinKanera
MartinKanera marked this pull request as ready for review October 12, 2026 16:11
@MartinKanera
MartinKanera requested review from kumilingus and removed request for kumilingus October 12, 2026 16:11
@kumilingus
kumilingus merged commit e3862a8 into clientIO:master Oct 12, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants