Page MenuHomePhabricator

JQMIGRATE warning "jQuery.fn.offset() requires an element connected to a document" when the element is the root
Closed, ResolvedPublic

Description

jQuery Migrate generates a false warning "jQuery.fn.offset() requires an element connected to a document" when the element in question is the root element (<html> / document.documentElement). OOjs UI PopupWidget code ends up triggering this in some cases (e.g. with https://gerrit.wikimedia.org/r/#/c/352071/ at Special:Preferences#mw-prefsection-rc when clicking the help icons). I'm pretty sure our usage is entirely safe (it works fine when I remove jQuery Migrate).

Minimal test case: open any page and run the following in the console:

$( 'html' ).offset();

Expected output is {top: 0, left: 0} with no warnings.

Related Objects

Event Timeline

matmarex created this task.Oct 2 2017, 8:30 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 2 2017, 8:30 PM

The fix is to change if ( !jQuery.contains( docElem, elem ) ) to if ( !jQuery.contains( docElem, elem ) || docElem === elem ) in jquery.migrate.js.

Krinkle added a comment.EditedOct 2 2017, 9:48 PM

I can't seem to find what jQuery 3 behaviour this warning was added for. Technically, "requires an element connected to the document", should allow documentElement given it is technically an element. However, since we're talking about what jQuery 3 does without jQuery Migrate enabled, it occurs to me that (unlike what jQuery Migrate claims) this behaviour is in fact not deprecated, and jQuery offset() does not return null, undefined, or throw for disconnected elements.

The warning was added in https://github.com/jquery/jquery-migrate/issues/108 and intends to cover changes from jQuery commit 1617479fcf. That commit was merged in 2016 and released in 2.2.0. But both jQuery 3.0.0 and 3.2.1's offset() return { top: 0, left: 0 } just fine for disconnected nodes. No null, undefined or exception thrown. The code must've changed before release..

I do find however that linked thread discussing these deprecations also states that it seems unnecessary to support offset() on document or documentElement in the long-term because 1) it would always return 0/0 anyway, and 2) is technically not part of the method contract, which is to "return the offset of an element relative to the document" - that doesn't exactly include the document per se. documentElement does inherit from Element and Node and its getBoundingClientRect method does work. And for the current implementation of jQuery's offset(), that's enough, so it works.

We can await further upstream discussion, but given the above, it might make sense to instead avoid calling offset() for documentElement.

Krinkle edited projects, added OOUI; removed MediaWiki-General.Oct 3 2017, 6:09 PM
matmarex claimed this task.Oct 3 2017, 7:50 PM

Change 382212 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] Do not call .offset() on $( 'html' )

https://gerrit.wikimedia.org/r/382212

matmarex renamed this task from False JQMIGRATE warning "jQuery.fn.offset() requires an element connected to a document" when the element is the root to JQMIGRATE warning "jQuery.fn.offset() requires an element connected to a document" when the element is the root.Oct 4 2017, 7:28 PM
Volker_E triaged this task as Normal priority.Oct 5 2017, 9:56 PM
Volker_E moved this task from Backlog to Doing on the OOUI board.

Change 382212 merged by jenkins-bot:
[oojs/ui@master] Do not call .offset() on $( 'html' )

https://gerrit.wikimedia.org/r/382212

Volker_E closed this task as Resolved.Oct 10 2017, 10:42 PM
Volker_E moved this task from Doing to OOjs-UI-0.23.4 on the OOUI board.
Volker_E edited projects, added OOUI (OOjs-UI-0.23.4); removed OOUI.
Volker_E removed a project: Patch-For-Review.