Page MenuHomePhabricator

After closing, browser sometimes scrolls to the top of the page in Firefox 70
Open, Needs TriagePublic

Description

Steps to reproduce

  1. Go to https://test.wikipedia.org/wiki/Ji%C5%99%C3%AD_z_Pod%C4%9Bbrad or https://cs.wikipedia.org/wiki/Ji%C5%99%C3%AD_z_Pod%C4%9Bbrad
  2. Scroll down and open an image with mediaviewer enabled
  3. Close mediaviewer (reproduced only with esc pressed, but that might be just a coincidence)
  4. If browser didn't scroll up, try again

Details

This appears to always happen in Firefox >= 70 and sometimes happen in Chrome. These may be two different problems, but may have the same solution.

Cause

https://www.fxsitecompat.dev/en-CA/docs/2019/history-navigation-methods-are-now-asynchronous/

Screencast

see https://martin.urbanec.cz/files/screencasts/T229484.webm for screencast

Event Timeline

Urbanecm created this task.Jul 31 2019, 9:08 PM
Restricted Application added a project: Multimedia. · View Herald TranscriptJul 31 2019, 9:08 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Urbanecm updated the task description. (Show Details)Jul 31 2019, 9:13 PM
Dvorapa added a subscriber: Dvorapa.Aug 6 2019, 2:28 PM

Since ~ 1 week this happens for every image view and is very annoying. Maybe it is related to the new releases Firefox 70.0? It does not happen in Chrome 77.

Aklapper renamed this task from After closing, browser sometimes scrolls to the top of the page to After closing, browser sometimes scrolls to the top of the page in Firefox 70.Oct 26 2019, 11:16 AM

This is driving me bonkers... for me it seems to happen on Firefox, but not Chrome

Aklapper edited projects, added Upstream; removed Multimedia.Nov 18 2019, 12:26 PM
Aklapper moved this task from Backlog to Reported Upstream on the Upstream board.

Quoting https://bugzilla.mozilla.org/show_bug.cgi?id=1594345#c7 :

This is a tech evangelism bug. A quick research finds:

Quoting https://bugzilla.mozilla.org/show_bug.cgi?id=1594345#c8 :

Workaround:
user_pref("dom.window.history.async", false);

dbarratt updated the task description. (Show Details)Nov 18 2019, 2:19 PM
dbarratt updated the task description. (Show Details)Nov 18 2019, 2:24 PM
dbarratt updated the task description. (Show Details)Nov 18 2019, 2:30 PM

(Reading web team does not maintain the MediaViewer extension)

Change 553523 had a related patch set uploaded (by Aklapper; owner: Tim Starling):
[mediawiki/extensions/MultimediaViewer@master] Fix failure to restore the scroll position on close in Firefox

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

Esanders removed a subscriber: Esanders.Dec 2 2019, 10:51 AM
Demian added a subscriber: Demian.Dec 23 2019, 11:57 AM

This is driving me bonkers... for me it seems to happen on Firefox, but not Chrome

Been happening to me in Chrome (and derivatives such as Vivaldi) for a long time.

Hi, we're still getting reports of this issue in bugzilla.mozilla.org, and I want to make sure to follow up. Is there anything we can do in Firefox or should we just wait for your patch to land? Thanks!

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.

Perlence removed a subscriber: Perlence.Dec 25 2019, 4:28 PM

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.

Demian added a comment.EditedJan 11 2020, 10:13 AM

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)

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.)

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.

This comment was removed by Demian.

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.

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