Change 560576 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Fix T229484 (broadly): scroll jumping to top and back after MediaViewer closed, or not restored at all.

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

The above patch separates the MediaViewer's scrolling to the overlay (body > .mw-mmv-wrapper) element and disables scrolling on the body with overflow: hidden, thereby eliminating the need to hide the content with display:none on open and to restore the scroll position on close. Restoring the scroll position produced a noticeable, disturbing jump to the top of the content and back and depending on browser and CPU load it failed as the content was not laid out in time.

This cleans up some hacky content-hiding css. The layout of the overlay elements are also cleaned up, z-index-es are no longer necessary, the image description (.mw-mmv-post-image) is now position: static and positioned at the bottom with margin-top: -86px; (typically), removing the need to update the inline top: css property with javascript.

Properly delegating mouse events (mouse scroll) requires pointer-events: none; on the position: fixed; image container (.mw-mmv-image-wrapper) element and the opposite pointer-events: auto; on the dialogs (.mw-mmv-dialog). This would not be necessary if position: sticky; would be used and other rules in the css would be noticeably simpler. Browser support is at 90% (not IE11, Opera Mini, UC Browser for Android). With a position: fixed fallback the complexity would be still there, therefore I've implemented just the fallback for every browser.

Change 562469 had a related patch set uploaded (by Matthias Mullie; owner: Matthias Mullie):
[mediawiki/extensions/MultimediaViewer@master] Fix scroll restoration when closing MMV

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

@AronManning Ugh - I had not noticed that you had already been working on a fix, sorry!
I submitted another patch - it looks like switching around the order of DOM changes & browser history manipulation also fixes Firefox's automatic scroll restoration.
I'm not too familiar with this codebase, so if you feel like your fix is the better (it does seem to have some welcome cleanup, but might also have some new browser hacks?), I'm happy to abandon mine & review yours once it passes CI.

@matthiasmullie Yes, I've completely removed the need to restore the position, also removing in the process a number of re-layouts, the jump to the top and back. That noticeable jump, the delay all got removed from the user experience. It is an extensive refactoring / cleanup. I expect it to be more reliable and smooth than getting the timing / order right, which can break for unexpected reasons.

Half of the cleanup is not yet committed to the patch: some javascript that updates the layout became unnecessary, as css does all of it now, resulting in a faster and smoother UX. I expected to upload a new patch a week ago, but unfortunately, I haven't had the time to do it. I note that Your patch has no conflict with mine, both should be merged in the end.

Patch is up, grunt test passed locally. If this will be merged sometime, am I right to assume it will be released to the beta server for some time before going into production?

Hmm, gerritbot did not add the below comment:
Change 560576 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Fix T229484 (broadly): scroll jumping to top and back after MediaViewer closed, or not restored at all.

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

I think we should merge https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MultimediaViewer/+/562469 as it's quite annoying to lose your place within a long article after viewing media in the mmv.

Change 564811 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@wmf/1.35.0-wmf.15] Move mmv-close event trigger higher up in lightboxinterface/unattach

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

Whilst staging and verifying this on www.mediawiki.org, I still encountered the bug in Firefox. It still worked the same in Firefox as before, and still worked correctly in other browsers. I've deployed it regardless to keep the state matching with master and to see if maybe it's a caching issue.

Mentioned in SAL (#wikimedia-operations) [2020-01-15T20:17:01Z] <krinkle@deploy1001> Synchronized php-1.35.0-wmf.15/extensions/MultimediaViewer/resources/: T229484 (duration: 01m 06s)

In T229484#5807166, @Krinkle wrote:

Whilst staging and verifying this on www.mediawiki.org, I still encountered the bug in Firefox. It still worked the same in Firefox as before, and still worked correctly in other browsers. I've deployed it regardless to keep the state matching with master and to see if maybe it's a caching issue.

It was worth a try. Fyi, I'm having the issue even in chrome when the cpu is burdened.

I'll submit a new version of the patch that completely removes the scroll restoration. I had trouble with the selenium tests that had to be changed as those were built on the assumption that the content is hidden with display:none; which is no longer true.
I can't run the selenium tests locally and some from the other batch. Unfortunately, I couldn't find anybody who could help with that, thus there will be a few commit-test-fail cycles before it's final.
Also, can I ask you to trigger Jenkinsbit, whenever I commit a patch for review? It's not testing automatically.

Are you proposing to remove MMV's scroll position logic because you expect that the browser's own logic in Chrome and Firefox works fine by default? (Just want to double check that I understand correctly.)

In T229484#5807260, @Krinkle wrote:

Are you proposing to remove MMV's scroll position logic because you expect that the browser's own logic in Chrome and Firefox works fine by default? (Just want to double check that I understand correctly.)

I propose to remove just the scroll restore logic (not the open-close metadata on scroll logic), which is unnecessary if the scroll position is not changed at all. The point is to separate scrolling mediaviewer to its own element and leave the content in peace. That way the position never changes. I've wrote more detailed reasoning in the commit comment.

Change 560576 had a related patch set uploaded (by Jforrester; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] After closing, browser sometimes scrolls to the top of the page

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

The patch passes the QUnit and Selenium-Cucumber tests. It's ready for hands-on testing.
If it gets accepted, will there be an internal test period on beta before it goes live?

How do you tell an IP to comment here? That is the advice given on the Help Desk.

@Vchimpanzee: Welcome to Wikimedia Phabricator. If you have comments or questions unrelated to the topic of this very task but about Phabricator in general, then please refer to https://www.mediawiki.org/wiki/Talk:Phabricator/Help instead. Thanks.

In T229484#5813935, @Vchimpanzee wrote:

How do you tell an IP to comment here? That is the advice given on the Help Desk.

By making an account. No IP editing on phabricator. If this is related to the scroll-up bug after viewing and closing an image, then you could link to the Help Desk request, or tell them the fix is under review, which might take weeks.
If the request is from a MediaWiki operator in dire need of a fix, with the necessary technical skills, they can check out the fix into a folder with:

mkdir MultimediaViewer && cd MultimediaViewer
git clone "https://gerrit.wikimedia.org/r/mediawiki/extensions/MultimediaViewer" .
git fetch "https://gerrit.wikimedia.org/r/mediawiki/extensions/MultimediaViewer" refs/changes/76/560576/14 && git checkout FETCH_HEAD

and use that folder as the htdocs/w/extensions/MultimediaViewer folder by symlinking it or replacing it. At their own risk only.

Change 560576 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Bug: T229484 After closing, browser sometimes scrolls to the top of the page

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

Change 560576 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] After closing, browser sometimes scrolls to the top of the page

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

Change 553523 abandoned by Tim Starling:
Fix failure to restore the scroll position on close in Firefox

Reason:
Use Aron Manning's patch instead

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

The problem does not seem to happen anymore in Firefox 74 or 75. Does anybody know why?

In T229484#6048930, @Aklapper wrote:

The problem does not seem to happen anymore in Firefox 74 or 75. Does anybody know why?

Patch 562469 treated the symptom, making it very unlikely to happen.
The timing issue and the race condition is still present and possible to occur on very slow systems. This possibility can be removed by disabling browser scroll restore (no patch for that yet) or by redesigning the DOM to avoid the scrolling hacks as it is done in patch 560576.

Still happening every time I open and image and close it. Ubuntu 18.04, Firefox 75.0 64-bit

Important: it does not happen most of the time, when pressing ESCape (it happens in small % of time). It always happens when closing image via X button.

Definitely not on a slow system, i7 16GB RAM etc.

After a long wait the patch has been cleaned up, rebased and updated for a final review.

Test wiki created on Patch Demo by Aron Manning using patch(es) linked to this task:

http://patchdemo.wmflabs.org/wikis/b8b692ed0df0cfb694260dfeb7cfaa3f/w/

In T229484#6594656, @PatchDemoBot wrote:

http://patchdemo.wmflabs.org/wikis/b8b692ed0df0cfb694260dfeb7cfaa3f/w/

Tested I can confirm that it appears to be fixed on the patchdemo instance

In T229484#6594650, @Demian wrote:

After a long wait the patch has been cleaned up, rebased and updated for a final review.

Tested on a personal Mediawiki project and I confirm this patch fixes the issue

Hey, Firefox Web Compatibility Engineer here,

This is creating a certain number of Web compatibility duplicate issues for Firefox.
Look at the duplicates on https://bugzilla.mozilla.org/show_bug.cgi?id=1594345
And https://bugzilla.mozilla.org/show_bug.cgi?id=1681977

Do we know when this could land?
Are there any issues that Mozilla should address on its side to make it easier for you to land it?

Thanks a lot.

@CBogen: Could someone from the Structured Data team please reply to the last comment? Thanks in advance!

We received a helpful email on the subject and have brought it into our process, which is currently fairly tied up with an imminent feature release and associated work. I would anticipate this work can be completed sometime in the next month, but at the moment, I have no further specifics. The maintenance of the Media Viewer feature has been Complicated(tm) since the Multimedia team became the Structured Data team and moved on to other projects.

To our friends at Mozilla: I'm sorry this is on your doorstep more often than it is on ours, but hopefully it won't be too much longer before it's fixed.

Some information from the folks at Mozilla:

The issue happens rarely using current Firefox or Chrome, and more often when enabling Fission[1] in Firefox Nightly. Fission makes session history operations "more" asynchronous than before.

The issue seems to be about race condition around https://github.com/wikimedia/mediawiki-extensions-MultimediaViewer/blob/master/resources/mmv.bootstrap/mmv.bootstrap.js#L584-L588 or some other similar code (that link is a bit old).

If I enforce code inside Firefox to make timers run later, the bug goes away. Per HTML specification tasks related to timers and session history use different task sources, so their ordering isn't guaranteed.

The code seems to be relying both timers and popstate event but those aren't connected in anyway. Could the code dealing with popstate fire the timer to do whatever scrolling needs to happen? Or perhaps history.scrollRestoration = "manual" should be used since apparently some manual scrolling is happening anyhow.

In fact, if I

• load https://en.wikipedia.org/wiki/Finland in Fission/Nightly
• set history.scrollRestoration = "manual" using the web console
• scroll down to 'Finland on a medieval map'
• click the image
• click the X to close the image

page isn't scrolled to top.

[1] out of process iframes and session history running in the parent process etc. To enable it in Nightly, Preferences->Nightly Experiments->Fission

Change 691210 had a related patch set uploaded (by Seddon; author: Seddon):

[mediawiki/extensions/MultimediaViewer@master] Lightweight fix to FF Nightly/Fission scroll issue

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

Change 691210 merged by jenkins-bot:

[mediawiki/extensions/MultimediaViewer@master] Lightweight fix to FF Nightly/Fission scroll issue

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

Checked on enwiki betalabs with two imported articles - https://simple.wikipedia.org/wiki/Impressionism and https://test.wikipedia.org/wiki/Jiří_z_Poděbrad - the fix is in place.

Checked on production wmf.18 - testwiki, hewiki: FF 91, Chrome 90, Safari 14 - works as expected.

Test wiki on Patch Demo by Aron Manning using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/b8b692ed0df0cfb694260dfeb7cfaa3f/w/

Change 560576 abandoned by Ladsgroup:

[mediawiki/extensions/MultimediaViewer@master] Save history position before opening MMV; remove need to restore scroll position; replace javascript-positioning with static css; cleanup html and css.

Reason:

The author has been banned indef from our tech spaces and the patch has not been touched for really long time.

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