1469959 - Clicking on scrolled out expanded object updates the scroll

Status: VERIFIED FIXED

(DevTools :: Console, defect, P1)

Description

[Julian Descottes [:jdescottes]]

STRs:
- open console
- log an object with many properties, eg `document`
- expand the object
- scrolldown until you see `prototype`
- expand prototype

ER: prototype is expanded
AR: viewport scrolls to show the beginning of the object, prototype is not expanded

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: console_object_expand_scroll.mp4

see attachment for STRs

Comment 2

[Julian Descottes [:jdescottes]]

I see the tree component already tries to avoid scrolling when user clicks on the tree. However before triggering the click event, a focus event is first received, which triggers the scroll. 

The tree seems to lose the focus every time we expand it, fixing this is probably another way to approach this issue.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

That looks bad. I'll do a mozregression when I get a better connection to see how long it's been here.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

DevEdition doesn't appear to be impacted.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

With the help of mozregression, I found this change to be the one introducing this issue: https://hg.mozilla.org/integration/mozilla-inbound/diff/eceb175f714a/devtools/client/shared/components/reps/reps.js (from this changeset https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a04929db78d35914678d26aee21872cebceb98cf&tochange=eceb175f714a6b6821c9b5ea874b2982defaa883). 
This was originally made in this PR https://github.com/devtools-html/debugger.html/pull/6335

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1469959 - Don't use relatedTarget to check if the ObjectInspector was focused by tabbing; .

Basically this rolls back the changes made to the reps bundle
in Bug 1463415. This change will be backported in Github later
so the fix can ride the train.
A test is added to make sure we don't regress again.

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

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8987748 [details]
Bug 1469959 - Don't use relatedTarget to check if the ObjectInspector was focused by tabbing; .

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

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8987748 [details]
Bug 1469959 - Don't use relatedTarget to check if the ObjectInspector was focused by tabbing; .

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

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8987748 [details]
Bug 1469959 - Don't use relatedTarget to check if the ObjectInspector was focused by tabbing; .

https://reviewboard.mozilla.org/r/253024/#review259616

::: devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_scroll.js:10
(Diff revision 3)
> +
> +// Check that expanding an objectInspector node doesn't alter the output scroll position.
> +const TEST_URI = "data:text/html;charset=utf8,test Object Inspector";
> +
> +add_task(async function() {
> +  const toolbox = await openNewTabAndToolbox(TEST_URI, "webconsole");

Note: we should probably have a getter for toolbox on `hud`, so we could still use openNewTabAndConsole to get hud directly then fetch toolbox from there.

This is done in a number of tests (https://searchfox.org/mozilla-central/search?q=toolbox.getCurrentPanel().hud&path=) so maybe we should spin off a separate bug to simplify this access.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8987748 [details]
Bug 1469959 - Don't use relatedTarget to check if the ObjectInspector was focused by tabbing; .

https://reviewboard.mozilla.org/r/253024/#review259616

> Note: we should probably have a getter for toolbox on `hud`, so we could still use openNewTabAndConsole to get hud directly then fetch toolbox from there.
> 
> This is done in a number of tests (https://searchfox.org/mozilla-central/search?q=toolbox.getCurrentPanel().hud&path=) so maybe we should spin off a separate bug to simplify this access.

Sounds good, I filed Bug 1471245 for this.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8987748 [details]
Bug 1469959 - Don't use relatedTarget to check if the ObjectInspector was focused by tabbing; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253024/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb6d71b705a8
Don't use relatedTarget to check if the ObjectInspector was focused by tabbing; r=bgrins.

Comment 14

[Raul Gurzau (:RaulG)]

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

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8987748 [details]
Bug 1469959 - Don't use relatedTarget to check if the ObjectInspector was focused by tabbing; .

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1463415
[User impact if declined]: Can't expand an object in the webconsole + weird scroll behavior
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes
- Open the webconsole at the bottom of the screen
- Evaluate `inspect(Math)`
- Make sure there's a vertical scrollbar on the console
- Scroll to the bottom and click on the "<prototype>" node
- Make sure the node is correctly expanded and that the scroll position stays the same.
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: No
[Why is the change risky/not risky?]: Because it rolls back the small piece of code that introduced this issue and a test was added to make sure this works as expected.
[String changes made/needed]: -

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8987748 [details]
Bug 1469959 - Don't use relatedTarget to check if the ObjectInspector was focused by tabbing; .

Fix for a new regression, let's take this for beta 4. Thanks for the new test!

Comment 17

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4b518c97f12c

Comment 18

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I've reproduced the issue on Firefox Nightly 62.0a1 (2018-06-19) under Windows 10 (x64), using STR from Comment 0.

The issue is no longer reproducible on Firefox Beta 62.0b4 and Firefox Nightly 63.0a1 (2018-06-29). The tests were performed under Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.12.

Comment 19

[Julian Descottes [:jdescottes]]

This issue is back in recent nightlies, with the new JSTerm. Should we reopen or file a new bug?

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

We should file a new one since the regression happened recently (I think I know where)