Page MenuHomePhabricator

Replace usages of jQuery .scrollTop and .height for plain DOM calls
Open, NormalPublic2 Story Points

Description

Let's stop using jQuery's scrollTop and height when unnecessary in Minerva and MobileFrontend.

AC

  • Calls to .scrollTop() are substituted by window.pageYOffset where equivalent
  • Calls to .height() are substituted by document.documentElement.clientHeight where equivalent

Source

From: https://gerrit.wikimedia.org/r/c/415491/

Krinkle
Patch Set 7: Code-Review-1

(1 comment)

Avoid jQuery for simple window scrollTop() and height(), which are the same cross-browser now. But unfortunately, their abstraction is not without cost, probably not worth using.

resources/mobile.infiniteScroll/InfiniteScroll.js :
Line 115:
Neither of these should jQuery.

As of jQuery 3, both of these methods unconditionally access a single plain property when given a Window object. And these are the standard properties used for this purpose, as documented on MDN and elsewhere. As good a time as any to start using the standard properties without the CPU and memory costs of jQuery indirection.

$.fn.scrollTop (jQuery 3.2.1):

   jQuery.each({ .., scrollTop: "pageYOffset" }, function(method, prop) {
     jQuery.fn[method] = function() {
       return access(this, function(elem, ..) {
         var win;
         if (jQuery.isWindow(elem)) {
           win = elem;
         }
         return win ? win[prop] : ..;

$.fn.height (jQuery 3.2.1):

    jQuery.each({ Height: "height", .. }, function(name = "Height", type) {
        jQuery.each({ content: type, ..  }, function(defaultExtra, funcName = "height") {
            jQuery.fn[funcName] = function( .. ) {
                return access(this, function(elem, type, value) {
                    if (jQuery.isWindow(elem)) {
                        return .. ? .. : elem.document.documentElement["client" + name];
                    }
In other words:

 var top = $window.scrollTop() === window.pageYOffset;
 var tall = $window.height() === document.documentElement.clientHeight;

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 2 2018, 9:27 AM

Change 415988 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Use native JS methods rather than jQuery indirection

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

ovasileva triaged this task as Normal priority.Mar 14 2018, 4:13 PM
MBinder_WMF raised the priority of this task from Normal to Needs Triage.Mar 14 2018, 4:13 PM
ovasileva triaged this task as Normal priority.Mar 14 2018, 4:13 PM

Should this task include updating our ESLint configs to forbid scrollTop references?

ovasileva set the point value for this task to 2.Mar 14 2018, 4:14 PM

Joaquin: we anticipate this change to be very mechanical and that's why this is pointed low.

Jdlrobson lowered the priority of this task from Normal to Low.Apr 11 2018, 6:01 PM

Thanks for flagging this!
We use scrollTop and height/width to set scrollTop, width and height (and on other things that may not be a window)
I'm not sure whether this makes sense right now so bumping down to low for the time being.

Change 415988 abandoned by Jdlrobson:
Use native JS methods rather than jQuery indirection

Reason:
not planning to work on this any time soon

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

Jdlrobson moved this task from Backlog to Bugs on the MinervaNeue board.Apr 26 2018, 11:38 PM
Jdlrobson raised the priority of this task from Low to Normal.Tue, Sep 10, 2:21 PM