1229928 - Scroll position isn't restored on vk.com after history.go(-1)

Status: RESOLVED FIXED

(Web Compatibility :: Site Reports, defect, P3)

Description

[arni2033]

Attachment: screenshot 1 - Scroll position isn't restored on vk.com after history.go(-1).png

>>>   My Info:   Win7_64, Nightly 45, 32bit, ID 20151201030226
STR:
1. Open https://vk.com/webmproject
2. Scroll the page down at least by several pixels
3. Click any small _video_, not a huge one (e.g. posted in comments). Request a screencast if necessary
4.A) Click on the free space on the right side of the video (see screenshot)
4.B) Open console, execute   history.go(-1)

Result:       
 The page is scrolled to the very top (scroll position isn't restored)

Expectations: 
 Scroll position should be preserved

I think that this is TE bug, because if I click close button or perform the same STR for images (not videos) on that site, everything works fine.
I notified the developers via Support, but any guesses on how to fix that are welcome.

Comment 1

[arni2033]

It was regressed(?) between 2015-08-30 and 2015-08-31 by bug 1186774:
> pushlog_url:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=993384a9dc2ea2af1c86d841ed78698ebc028dea&tochange=b3bc092e919b981e158e696ce601bb26fd5370a2

Comment 2

[Mike Taylor [:miketaylr]]

Olli, should we move this to Core, assuming it's a regression?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Well, bug 1186774 was purposeful change to do what some other browsers do.
So this might be evangelism bug. Not sure...

Comment 4

[Mike Taylor [:miketaylr]]

Ah, OK.

Comment 5

[arni2033]

I'm currently having a conversation with one of vk.com developers (or maybe just support service yet?)
with a speed of 1 post per 4 hours. I've notified them about this bug and bug 1186774.
I think it's better to wait for their answer for ~1-2 days, but there's not much time left until Release 43, if there's an option to back out changes made in 1186774

Comment 6

[arni2033]

They said that they aren't going to fix this yet and "beta-version of a browser is made for finding bugs". Haha, they expect this to be fixed, even though I'm the only person noticed it and Nightly is still affected.
But they could just say that to me and silently start to fix the bug.

Note, this can also be tested with another Step 3 (easier):
> 3. Click any small _video_ or a link under a big video to open it.

Comment 7

[Hallvord R. M. Steen [:hallvors]]

If our new behaviour is closer to what (certain) other browsers do, how is vk.com working in those browsers? Maybe there's some browser detection in their code?

Comment 8

[arni2033]

They fixed it on their side long time ago (with a hack of course)
Closing as "Fixed"

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Why why would have fixed this with a hack? We follow the latest HTML spec here.

Comment 10

[arni2033]

I decided to reopen this bug, because the hack is too bad.
It causes twitching on the page: the page is briefly scrolled to the beginning (natively, in Firefox), then the page overrides that by setting correct scrollTop.

(In reply to Olli Pettay [:smaug] from comment #9)
> Why why would have fixed this with a hack? We follow the latest HTML spec here.
That doesn't guarantee that everything should work OK, because Firefox behaves differently than Chrome (and also there're things not included in spec (and possibly Firefox doesn't follow the latest spec for everything)). But this bug is most likely mistake on vk.com side.

[1] is the video of "new" bug. [2] is testcase. Please read comment near each of 5 lines with text "//arni2033" (in sources) and tell me what should I say to vk.com developers when I contact them next time. Well, I can't speak with developers, but there's some kind of support service.

> [1] https://dl.dropboxusercontent.com/s/jclbrcoyb1c974j/0_2016.08.12%2017-21-13.webm?dl=0
> [2] https://dl.dropboxusercontent.com/s/1j2shtp9677skrq/bug%201229928%20testcase%201.zip?dl=0

Comment 11

[Sky Schubert [:skyschub]]

Thank you for your work! Actually, you have already found the issue. You are right, Firefox stores a scrollTop of 0, but that's the truth.

(from common.js)
> window._oldScroll = scrollNode.scrollTop;
> pageNode.style.height = (_oldScroll + (window.lastWindowHeight || 0)) + 'px';
> pageNode.style.marginTop = -_oldScroll + 'px';

This code is in a block that only gets executed in Firefox and they set the height of #page_wrap to the current window height, emulate the previous scrolling position by moving the element up with a negative top margin. Basically, the page is no longer heigher than the viewport, so it automatically scrolls to 0, so the scroll position is indeed 0.

I am guessing they use this hack to avoid scrolling away the video. In other browsers, they use

> bodyNode.style.overflow = 'hidden';

for that purpose. I remember some issues with `overflow: hidden;` in the past, but the hidden overflow on #page_wrap seems to work now in Firefox 48+ so there is no reason to keep the exception for Firefox in there. So the best suggestion you can do is to remove the Firefox specific workarounds since the issues are no longer there.

(clearing the ni? since I don't think we need more feedback here)

Comment 12

[arni2033]

(In reply to Dennis Schubert [:denschub] from comment #11)
> I remember some issues with `overflow: hidden;` in the past, but the hidden overflow on #page_wrap
> seems to work now in Firefox 48+ so there is no reason to keep the exception for Firefox in there.
I think you meant bug 848656, which really seems fixed (and I also tested original page using Fiddler (application for replacing page sources) and it was OK).
Bug I disagree. I mentioned it in one of the comments in testcase [2] that there's at least one reason to keep that exception: it's bug 1272084 (bug 325942). Without that exception, I can accidentally scroll the page while autoscrolling opened box with video.
I'm going to contact them at the end of the week and tell about several bugs, including this one.

As I noticed in comment 0, this works fine for images, but is broken for videos.
I figured out that it happens, because images and videos are handled in 2 different .JS files, which cause 2 different sequences of main pieces of code when I click outside of image/video:

Images:
 popstate event handler
 scrollTop is restored by Firefox
 Image box is hidden
 scrollTop is restored by page

Videos:
 popstate event handler
   video hidden   (inside popstate event handler)
   scrollTop is restored by page   (inside popstate event handler)
 scrollTop is restored by Firefox
 scrollTop is restored by page   (in setTimeout) [THE HACK!]

Videos (before bug 1186774):
 scrollTop is restored by Firefox
 popstate event handler
   video hidden   (inside popstate event handler)
   scrollTop is restored by page   (inside popstate event handler)
 [no workaround was required] (but "THE HACK" doesn't break anything)

Comment 13

[Sky Schubert [:skyschub]]

Thanks for clarifying "autoscrolling", I assumed you are talking about something different. This still appears to be an issue, indeed, but that's tracked in bug 1272084, as you've mentioned.

As far as the scroll position goes, there is nothing to blame Firefox here. It totally works as expected and I've confirmed that Firefox handles scrolling position storing the same way as other browsers do. If you apply the Firefox specific code in commons.js to all browsers (i.e. replacing the conditions in lines 2035 and 2083 of your version with true), Chrome also jumps back to the top of the page after going back the history. Scrolling to the top is not a bug, as I've explained in comment 11. The browser technically IS at the top of the page after the Firefox specific stylings are applied by the script.

If removing the Firefox specific is not an option for them, pushing the history state before line 2035 (thus, before resetting the actual scroll position) could work as well.

Comment 14

[Mike Taylor [:miketaylr]]

Let's close here. I don't think there's anything worth pursuing.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Well, we could get vk.com to remove the broken run-only-in-Firefox code right?

Comment 16

[Mike Taylor [:miketaylr]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> Well, we could get vk.com to remove the broken run-only-in-Firefox code
> right?

Yeah, we should. We should also check to see it still reproduces. Dennis, does this bug/Fx-only codepath still exist?

Comment 17

[Sky Schubert [:skyschub]]

Yeah, it's basically still the same situation here.

Comment 18

[Sergiu Logigan [:sergiu]]

It doesn't seem to reproduce anymore, on Windows 10 and Firefox Nightly 64.