988102 - Opening inspector scrolls the content page all the way to the top

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Brian Grinstead [:bgrins]]

STR:

Open a page
Scroll down a bit
Open the inspector panel

Notice that as the body gets highlighted the page gets scrolled to the top.  The scroll position of the page should not change when devtools opens.

Comment 1

[Brian Grinstead [:bgrins]]

I'm guessing this is caused by the box model highlighter landing, in bug 663778.

Comment 2

[Brian Grinstead [:bgrins]]

The "inspector-open" selection is being ignored as expected, but a "nodeselected" selection is firing immediately after this, causing the default node to get highlighted (and scrolled into view).  The comment in _shouldNewSelectionBeHighlighted indicates that we shouldn't be highlighting at all in this case.

Relevant code is https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#275 and https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#285.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: highlight-on-open-WIP.patch

Mike, this _onNewSelection function seems to get called twice when it is opened (once for inspector-open and once for nodeselected.  If you add some logging like this the top of the function:

console.log(selection.reason);
console.trace();

This patch fixes the issue, but breaks the inspector_basic_highlighter test.  I was wondering if you could take a look at it and see if anything sticks out.

The alternative to fix this issue is to remove the scrollIntoView: true on the _brieflyShowBoxModel call, because I can't tell when it is actually scrolling into except for this case, but I'm probably missing something here.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Created attachment 8397966 [details] [diff] [review]
> highlight-on-open-WIP.patch
> 
> Mike, this _onNewSelection function seems to get called twice when it is
> opened (once for inspector-open and once for nodeselected.  If you add some
> logging like this the top of the function:
> 
> console.log(selection.reason);
> console.trace();
> 
> This patch fixes the issue, but breaks the inspector_basic_highlighter test.
> I was wondering if you could take a look at it and see if anything sticks
> out.
> 
> The alternative to fix this issue is to remove the scrollIntoView: true on
> the _brieflyShowBoxModel call, because I can't tell when it is actually
> scrolling into except for this case, but I'm probably missing something here.

Failing test log: https://tbpl.mozilla.org/php/getParsedLog.php?id=36811991&tree=Try

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Taking this from Brian.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: highlight-on-open-988102.patch

I don't understand why we would ever need scrollIntoView as it is likely to cause way more problems than it solves.

I have removed the scrollIntoView option and everything appears to work fine. If we really need that option then it should be added to the markup view context menu.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=0464270710ab

Comment 8

[Paul Rouget [:paul]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #7)
> Created attachment 8404732 [details] [diff] [review]
> highlight-on-open-988102.patch
> 
> I don't understand why we would ever need scrollIntoView as it is likely to
> cause way more problems than it solves.
> 
> I have removed the scrollIntoView option and everything appears to work
> fine. If we really need that option then it should be added to the markup
> view context menu.

Agreed.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://hg.mozilla.org/integration/fx-team/rev/5033a4da6a1e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5033a4da6a1e

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8404732 [details] [diff] [review]
highlight-on-open-988102.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 663778
User impact if declined: The content page will scroll to the top after opening the inspector panel in devtools
Testing completed (on m-c, etc.): Been on m-c since 4-14
Risk to taking this patch (and alternatives if risky): Low risk, just prevents scrolling to the selected element when it is selected in DevTools
String or IDL/UUID changes made by this patch:

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/73afe69f15d3

Comment 13

[Francesca Ciceri [:madamezou]]

Hi,

I was able to reproduce and verify it successfully on:

- Windows 7 x86_64: reproduced on 31.0a1 BuildID 20140326030203 and verified the fix on latest Beta 30.0 ID:20140520115057 and Aurora 31.0a2 (2014-05-21) 
- Debian Linux x86_64: reproduced on 31.0a1 BuildID 20140326030203 and verified the fix on latest Beta (30.0 ID:20140520115057) and Aurora 31.0a2 (2014-05-21)

As these are 2 platforms out of 3 I think it's safe to mark it as Verified.

Cheers,
Francesca