Page MenuHomePhabricator

MultimediaViewer uses an old version of jquery.scrollTop which is broken for Chrome 61
Closed, ResolvedPublic

Description

I'm a chromium author and wanted to report a Wikipedia bug that was reported to us.

VIew bug: https://bugs.chromium.org/p/chromium/issues/detail?id=756868&

In debugging it seems Wikipedia is using an old version of the jquery scrollTop library. It is broken in the following code:

return this.map(function() {
    var elem = this
      , isWin = !elem.nodeName || $.inArray(elem.nodeName.toLowerCase(), ['iframe', '#document', 'html', 'body']) != -1;
    if (!isWin)
        return elem;
    var doc = (elem.contentWindow || elem).document || elem.ownerDocument || elem;
    return /webkit/i.test(navigator.userAgent) || doc.compatMode == 'BackCompat' ? doc.body : doc.documentElement;
});

The test for webkit is wrong now for Chrome 61. I suggest adding:

"if (doc.scrollingElement) return doc.scrollingElement;"
condition before return or upgrading to a newer version of jquery scrollTo

Event Timeline

There is no jquery.scrollTo.js library that ships with MediaWiki core. The library is actually shipped through the MediaViewer extension.

https://github.com/wikimedia/mediawiki-extensions-MultimediaViewer/blob/ed75655518/resources/jquery.scrollTo/jquery.scrollTo.js

It's currently using version 1.4.9, the latest is 2.1.2. The change away that removed the assumption of document.body being the scrollable container in WebKit-based browsers is d87e471a3f9, which was first released in 2.0.0.

TheDJ renamed this task from Wikipedia uses an old version of jquery.scrollTop which is broken for Chrome 61 to MultimediaViewer uses an old version of jquery.scrollTop which is broken for Chrome 61.Aug 21 2017, 8:38 AM

What is the progress on this issue? Just looking for when we can expect it to be fixed? Chrome 61 ships in two weeks to stable.

Chrome 61 is now released as stable, and this breaks MV badly.

What is the purpose of this lib? Why not use $(window).scrollTop()?

Change 377808 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/MultimediaViewer@master] Remove jquery.scrollTo library

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

What is the purpose of this lib? Why not use $(window).scrollTop()?

Browser differences in what exactly needs to be scrolled. See the comments on https://gerrit.wikimedia.org/r/#/c/107384/1.

What is the purpose of this lib? Why not use $(window).scrollTop()?

Browser differences in what exactly needs to be scrolled. See the comments on https://gerrit.wikimedia.org/r/#/c/107384/1.

Whether the scrolling element is documentElement (<html>) or body does indeed differ in browsers. If we need that, per https://developer.mozilla.org/en-US/docs/Web/API/document/scrollingElement and https://github.com/mathiasbynens/document.scrollingElement/blob/master/scrollingelement.js, the following then should work in modern browsers: ( document.scrollingElement || document.documentElement )

However, upon closer inspection, I don't think we need to interact with the scrolling element directly. We only need that when interacting with e.g. the clientHeight/offset properties, such as when animating the property (neither body nor html is window).

The Window.scrollTo API, as used by jQuery's scrollTop method when called on $(window), is a higher level method that works in browsers as-is.

$( window ).scrollTop( val );
// Equivalent to:
window.scrollTo( val, window.pageYOffset );

Change 377808 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Remove jquery.scrollTo library

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