1641719 - First click anywhere in the JSON viewer scrolls back to the top

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

STR:

1. Open https://bug1641719.bmoattachments.org/attachment.cgi?id=9152582 in the JSON Viewer
2. Scroll a bit
3. Click on any item below the fold

Current result:

The view gets scrolled to the top.

Expected result:

The element expands.

This only happens on the first click. After that, next click expands rows as expected.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: timeseries-country.json

Comment 2

[Oriol Brufau [:Oriol]]

So when you load it, no row is selected initially.
When you mousedown, if there is no selected row, the 1st one is selected for some reason.
When a row is selected, it's scrolled into view.
Then, if it was already scrolled to the top, and you don't move the mouse, the mouseup will happen in the same element, which will produce a click, which will select the desired row.
Otherwise, the mouseup will most probably happen in some other element, which won't produce a click, so the 1st row stays selected.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Harald Kirschner :digitarald

ah, found it
zbraniecki: https://searchfox.org/mozilla-central/source/devtools/client/shared/components/tree/TreeView.js#315 is called for "focus"
events fired are focus-click-focus-focus, so first focus does not have a selected state yet in the Tree I guess

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Another STR:

1. Visit https://api.github.com/
2. Click on any node

Current Result:

The first node is selected for a split second before the clicked node gets selected.

Expected Result:

Only the clicked node should get selected.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Kwan

regression window is currently https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d5efb485c09c230a90fc2a24ba473ae35c1963f2&tochange=ef8f91e97c99d3013dbe78e24f8cb52765ca6262 so I'm assuming Harald is right about 1545727 being the regressor

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1641719 - Select the first row when mounting JSON Viewer to avoid it being selected on the first click.

Comment 7

[Ian Moody [:Kwan] (UTC+0)]

18:00.25 INFO: Narrowed integration regression window from [903f141a, 41f8b0b2] (3 builds) to [7192ec5b, 41f8b0b2] (2 builds) (~1 steps left)
18:00.25 INFO: No more integration revisions, bisection finished.
18:00.25 INFO: Last good revision: 7192ec5bf245b12aead3067b33e80fa8614a5870
18:00.25 INFO: First bad revision: 41f8b0b21560997f8b63b0389afbce68235cdced
18:00.25 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7192ec5bf245b12aead3067b33e80fa8614a5870&tochange=41f8b0b21560997f8b63b0389afbce68235cdced

Comment 8

[Pascal Chevrel:pascalc]

Too late to fix in 77, wontfix for this release.

Comment 9

[Dickson Tan]

I can't seem to add any comments in the linked PR. I get this error:

Members of the project "Restricted Project" can take this action.

Anyways, I wanted to say that I did try the approach in that PR when implementing keyboard selection - to always ensure that something always has focus, but there were 2 concerns raised:

• Will it accidentally move keyboard focus?
• This could be confusing visually unless a separate set of styles is used for when something is selected vs actually having keyboard focus if the first row always gains focus on initial render.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1641719 - Don`t set 0 state in TreeView on first click. r?bomsy

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:Honza, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Video of STR's as expected.

Hi Zibi,
I just tested with only https://phabricator.services.mozilla.com/D79322 applied (to master) and it seems to work as expected (e.g. no scroll). I'm attaching a video with what I see, let me know if my steps are different from yours.

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ah, you're right! It does fix it! Thank you!

I abandoned my revision and handing the bug over to you. I think your patch is sufficient!

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #13)

Ah, you're right! It does fix it! Thank you!

I abandoned my revision and handing the bug over to you. I think your patch is sufficient!

Great!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ecc3ceb6bbe
Don`t set 0 state in TreeView on first click. r=bomsy

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7ecc3ceb6bbe