Page MenuHomePhabricator

Fix JQMIGRATE: jQuery.fn.offset() requires a valid DOM element in Translate's group selector
Closed, ResolvedPublic

Description

Possibly fixing this would also fix This page is using the deprecated ResourceLoader module "jquery.ui.position".. Though, I am still not aware of any replacements for it.

Event Timeline

Nikerabbit renamed this task from Fix VM940:25 JQMIGRATE: jQuery.fn.offset() requires a valid DOM element in Translate's group selector to Fix JQMIGRATE: jQuery.fn.offset() requires a valid DOM element in Translate's group selector.Jun 26 2018, 2:54 PM
Krinkle triaged this task as High priority.Jul 12 2021, 4:11 PM
Krinkle subscribed.

Raising priority as this is blocking the jQuery upgrade in production.

@Krinkle Can you provide more information:

  • Which team/people is this blocking
  • What problem is this causing
  • What do you need from us (e.g. just get rid of the warning vs. rewriting it to not use jquery.ui.position)
  • How urgent this is/timeline

We have a sprint planning meeting tomorrow, so having this information by then would be useful.

I'm not Krinkle so I can't answer on his behalf but I want to mention this is part of T280944 and we hope to turn off jquery 1.x b/c soon (in CI/dev it'll be pretty soon, possibly in a week or two: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/703475) meaning if your system depend on really old jquery, it'll break.

It's blocking removal of jquery migrate code (introduced in 2017) which is being shipped in the default payload (meaning improving performance).

HTH

@Ladsgroup I have troubles reproducing this warning now. Could this have been fixed by your changes?

I can't say for sure but if Translate is depending on jquery.ui, the warning could have been coming from jquery.ui instead of Translate extension (or its dependencies). Can you grep for $.offset in language-team extensions?

In Translate there a few uses, but nothing in message group selector:

resources/js/ext.translate.editor.shortcuts.js
13:                     editorOffset = this.$editor.offset();
26:                             .offset( { top: middle - 15, left: maxLeft } )
33:                             .offset( { top: middle + 15, left: maxLeft } )
48:                                     .offset( offset )
59:             var offset = $element.offset();

resources/js/ext.translate.special.searchtranslations.js
160:                            top: $languages.offset().top,

resources/js/ext.translate.editor.js
372:                                    scrollTop: $( '.tux-message-editor:visible' ).offset().top - 85

resources/js/ext.translate.special.managetranslatorsandbox.js
410:                    detailsHeight = $( window ).height() - $detailsPane.offset().top,

resources/js/ext.translate.messagetable.js
293:                                    offset = $visibleIcon.offset();
815:                    messageListOffset = this.$container.offset();
914:            elementTop = $loader.offset().top;

The group selector does use https://api.jqueryui.com/position/ though.

jquery.ui.position didn't have a fix on offset recently, but had a fix on isWindow: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/703627/2/resources/lib/jquery.ui/jquery.ui.position.js

I'll dig a bit on it soon. Today is quite a Monday.

I couldn't find anything related but I want to point out that offset on invalid DOM is now basically doesn't happens anymore: https://grafana.wikimedia.org/d/000000037/mw-js-deprecate?viewPanel=18&orgId=1&refresh=1m&from=now-70d&to=now&var-Step=24h

image.png (793×1 px, 77 KB)

(Keep it in mind it's 1:100 sampling)

I suggest closing this is resolved, it's very likely fixed by my changes or others.

Nikerabbit claimed this task.