Page MenuHomePhabricator

[Bug] Exiting and re-entering the image gallery via browser history loses page scroll offset
Open, NormalPublic

Description

Note: The scroll restoration works perfectly if you open the overlay once, but dare you open it a second time...
It also only impacts users who use the browser navigation controls. Clicking "X" restores scroll position every time.

Related: T209418

Steps to Reproduce (1)

  1. Visit https://en.m.wikipedia.org/wiki/Charles_Darwin?oldid=871874243 in the MinervaNeue mobile skin on desktop browser
  2. Scroll down to an image deep in the article such as the evolutionary tree diagram
  3. Tap the image
  4. Press browser back button
  5. Press browser Forward button
  6. Press browser back button

Steps to Reproduce (2)

  1. Visit https://en.m.wikipedia.org/wiki/Charles_Darwin in the MinervaNeue mobile skin
  2. Scroll down to an image deep in the article such as the evolutionary tree diagram
  3. Tap the image
  4. Tap details
  5. Press browser back button twice

Expected Results

  • Scroll offset in page is unchanged

Actual Results

  • Scroll location is reset to the top

Environments Observed

  • enwiki prod

Browser Version

  • Chrome v71.0.3578.57

OS Version

  • Chrome OS v11151.33.0

Device Model

  • Pixel Slate

Device Language

  • English

Developer notes

  • When an Overlay is shown using Overlay.prototype.show window.pageYOffset is stored locally in a variable called scrollTop
  • When hide is executed, that scrollTop is passed back and restored in the hide function.
    • Note it is also set inside the OverlayManager (_checkRoute method)
  • On clicking the image, this.scrollTop is set correctly
  • On clicking back, this.scrollTop is restored.
  • When clicking forward, this.scrollTop is set incorrectly as "1"

Removing the handling in checkRoute fixes this problem at the cost of breaking things not using the OverlayManager.
It doesn't look like any of our Overlay's are not using the OverlayManager so it should be safe to remove this code altogether \o/

Event Timeline

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptDec 4 2018, 11:45 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Confirming this bug (although on iOS/Chrome & Safari I am unable to reproduce it). As far as I can tell this only occurs as the result of using the browsers "forward" button to enter into the media overlay. I would guess this isn't a particularly common flow, and thus this is lower priority, but perhaps we could confirm with data?

A related bug that seems to be occurring on Chrome: if, from article A, I follow the "Steps to Reproduce (1)" above, then tap a blue link (going to article B), then tap the browser's "back" button (returning to article A), I return to a different part of article A than I departed from. This is somewhat described in T209418, although I was unable to reproduce that bug by itself.

Regarding ""Steps to Reproduce (2)", I noticed that if at the end of those steps I then press the browsers "forward" button again, the "Details" button has black text (instead of white). This seems rather trivial, I am noting it mainly incase it is some sort've clue as to what's going on in general here.

@Niedzielski can you clarify in the reproduction steps in the task's description when you mean the browser's back/forward button, versus the back/forward controls on the media overlay?

Niedzielski updated the task description. (Show Details)Dec 11 2018, 2:58 PM

can you clarify in the reproduction steps in the task's description when you mean the browser's back/forward button, versus the back/forward controls on the media overlay?

@alexhollender, the dedicated back and forward buttons in the browser chrome available on all webpages. Also, the back button on Android and equivalent back / forward gestures on some tablets.

Jdlrobson renamed this task from [Bug] Exiting the image gallery loses page scroll offset to [Bug] Exiting and re-entering the image gallery via browser history loses page scroll offset.Dec 13 2018, 1:09 AM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Dec 13 2018, 1:12 AM

Possibly related to T189225

Jdlrobson updated the task description. (Show Details)Dec 18 2018, 8:05 PM
Jdlrobson moved this task from Needs Analysis to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: nray.Mar 12 2019, 7:07 PM

@nray while you are looking at the ImageOverlay could you take a look at if your changes in T216198 can or could fix this?

nray added a comment.Mar 12 2019, 8:30 PM

@nray while you are looking at the ImageOverlay could you take a look at if your changes in T216198 can or could fix this?

My changes don't touch this code so the bug is still there (was able to replicate with my patch). I suggest we work on this separately from my changes as mine are already very large

Cool just checking! Thanks for confirming the bug is still present!

I verified again today and this is still a problem.