1382341 - Refreshing page while Web Developer Inspector is open never remembers scroll position

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[yoni]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Open any page, long or short, scroll down, open Web Developer console via F12 or keyboard shortcut (eg Ctrl Shift C for inspector).  Scroll down any length. Refresh via button or F5. 


Actual results:

Page always reloads at top, forgetting position in page.


Expected results:

Page should remember position, very awful for workflow.  When console is closed and page is refreshed (button or F5) Firefox remembers position perfectly and returns to same place.

Comment 1

[yoni]

Problem happens whether console is docked at bottom, sidebar or popped out into new window - upon reload will never remember page position. Tried on brand new FireFox, brand new profile - same results.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

I am reproducing on Nightly 56.0a1 (2017-07-18), but only if I open the inspector. If I only open the console (Cmd + alt + K) and scroll in it, the scroll position of the page is still kept on reload.

Comment 3

[yoni]

Just tried the very latest Nightly - 56.0a1 (2017-07-26) (64-bit) - can confirm exact same problem is still happening! I know we're not supposed to complain, just report bugs but this is incredibly aggravating and makes me need to developer work in Chrome, which I have avoided until now.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Hi yoni, I understand your frustration :) we need to fix this.

Matteo, pbro was saying to me that this can be due to Highlighters, does it look familiar to you ?

Comment 5

[Jelle]

I'm having the same problem. It is really annoying when building on a website.

Comment 6

[quentin.favrie]

I'm having the same issue with Firefox Developer Edition 56.0b5 (64-bit) on Ubuntu 17.04

Comment 7

[Patrick Brosset <:pbro>]

Just discussed with Matteo. It seems like bug 1341756 might be the cause of this bug. On reload, highlighters are removed, which makes the DOM of the page different which, in turn, prevents the auto-scroll feature from working after reload.

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #7)
> Just discussed with Matteo. It seems like bug 1341756 might be the cause of
> this bug. On reload, highlighters are removed, which makes the DOM of the
> page different which, in turn, prevents the auto-scroll feature from working
> after reload.
Confirmed. Removing the fix for bug 1341756 fixes this bug.

Comment 9

[yoni]

Is there a way to remove the fix and/or download a different nightly/previous build that fixes this bug? THanks.

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

I did some tests, we could basically revert the changes of bug 1341756, basically removing this line:

http://searchfox.org/mozilla-central/source/devtools/server/actors/highlighters/utils/markup.js#311

I thought that doing so it would cause again the behavior fixed by such bug; but *apparently* not. Visually, everything works fine.
However, it would make the RDM causing the same issue of bug 1396690, that currently seems to happen specifically when we drag & drop a tab (the swapFrameLoaders is involved here too).

To be more precise, entering and quitting RDM mode, would add new highlighters markup every time, even if – compare to bug 1341756 – the old ones would be hidden and inactive.

I would suggest to revert bug 1341756 here, add a unit test to check that the scroll is working as intended for any future regression, and fixes the duplicated markup in bug 1396690.

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Bug 1382341 - removed `this._remove()` from `onWindowReady`;

This basically revert the changes made in bug 1341756; but it doesn't seems to
bring the same issue as before since `pagehide` event is emitted properly when
entering in RDM.

Added unit test to catch future regression.

The removal of duplicated hidden markup in anonymous content will be handled by
Bug 1396690.

Review commit: https://reviewboard.mozilla.org/r/176316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176316/

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8904468 [details]
Bug 1382341 - removed `this._remove()` from `onWindowReady`;

https://reviewboard.mozilla.org/r/176316/#review181218

Feels a bit weird just removing a prior bug fix, but if the fix was useless, then great!

::: devtools/client/inspector/test/doc_inspector_highlighter_scroll.html:7
(Diff revision 1)
> +<html>
> +  <head>
> +    <meta charset="utf-8">
> +    <style>
> +      p {
> +        height: 3000px;

3000px is probably ok for all screens we test on, but why not use 200vh instead? At least you're garanteed to be future-proof. You never know, maybe one day we add a test VM that has a crazy long screen size :)

Comment 13

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Patrick Brosset <:pbro> from comment #12)

> Feels a bit weird just removing a prior bug fix, but if the fix was useless,
> then great!

It wasn't. It is now! Apparently before `onpagehide` it wasn't emit properly during the `swapFrameLoaders` therefore we had the issue shown here: https://bug1341756.bmoattachments.org/attachment.cgi?id=8840059

But I'm not able to reproduce it anymore (with this new patch applied), and the only downside is that we introduced the issue described in bug 1396690 (only when we enter / quitting RDM); but I think it's better having this than the document that lost the previous scroll – also, we're going to address that in bug 1396690 anyway.

> ::: devtools/client/inspector/test/doc_inspector_highlighter_scroll.html:7
> (Diff revision 1)
> > +<html>
> > +  <head>
> > +    <meta charset="utf-8">
> > +    <style>
> > +      p {
> > +        height: 3000px;
> 
> 3000px is probably ok for all screens we test on, but why not use 200vh
> instead? At least you're garanteed to be future-proof. You never know, maybe
> one day we add a test VM that has a crazy long screen size :)

Indeed! :)

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8904468 [details]
Bug 1382341 - removed `this._remove()` from `onWindowReady`;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176316/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd3069bc00a3
removed `this._remove()` from `onWindowReady`; r=pbro

Comment 16

[Matteo Ferretti [:zer0] [:matteo]]

Here the try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a376c51d597c79578afccbb9f0bb75f4ba4c501a

The oranges doesn't seems related. I fixed the ES error before pushing to inbound (even if mozreview doesn't seems to show the line at the end of the file).

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/dd3069bc00a3

Comment 18

[Saheda Reza [:Antora]]

I have reproduced this bug according to (2017-07-19)

Fixing bug is verified on Latest Nightly--
Build ID    :20170915220136
User Agent  :Mozilla/5.0 (Windows NT 6.1; rv:57.0) Gecko/20100101 Firefox/57.0

Tested OS-- WIndows7 32bit