751836 - After stepping in the debugger, any open nodes in the property view are collapsed

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

Because we're rebuilding the tree (and there's kinda no way to do it otherwise).
Fortunately, bug 724229 gives us a nice hierarchy of the previously opened vars/properties in all the scopes.
Chrome does this too, and reopens the tree after a few seconds. I think we can do better.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Mango juice.

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v1, part2

From the patch:

+   * Emptying this container and rebuilding it immediately afterwards would
+   * result in a brief redraw flicker, because the previously expanded nodes
+   * may get asynchronously re-expanded, after fetching the prototype and
+   * properties from the server.
+   *
+   * To avoid such a behaviour, a normal container list is rebuild, but not
+   * immediately attached to the parent container. The old container list
+   * is kept around for a short period of time, hopefully accounting for the
+   * data fetching delay. In the meantime, any operations can be executed
+   * normally.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v1.1, part2

Experimented with different depths of expanded properties and lowered the empty delay a bit. I think it feels ok. With 200ms we're *really* safe. 100ms still gets us flicker with > 2 levels deep. 150ms seems like a good compromise, but I may be wrong.

Also fixed an issue with the variable-changed-flash-transitions-thing not showing up (d'uh, the node wasn't drawn yet).

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v1.1, part3

Properties want flashing too :)

Currently only the .variables had the transition rules in the css. Copied them to the .properties as well.

Comment 5

[Victor Porof [:vporof][:vp]]

Attachment: v2

qfolded ftw.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 674172 [details] [diff] [review]
v2

Review of attachment 674172 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice usability improvement!

::: browser/devtools/debugger/debugger-view.js
@@ +39,5 @@
>      this.Variables = new VariablesView(document.getElementById("variables"));
>      this.Variables.emptyText = L10N.getStr("emptyVariablesText");
>      this.Variables.nonEnumVisible = Prefs.nonEnumVisible;
>      this.Variables.eval = DebuggerController.StackFrames.evaluate;
> +    this.Variables.lazyEmpty = true;

How come you don't just initialize it to true in VariablesView? I see that head.js already sets it to false for tests, so what am I missing?

Comment 7

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #6)
> Comment on attachment 674172 [details] [diff] [review]
> v2
> 
> Review of attachment 674172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very nice usability improvement!
> 
> ::: browser/devtools/debugger/debugger-view.js
> @@ +39,5 @@
> >      this.Variables = new VariablesView(document.getElementById("variables"));
> >      this.Variables.emptyText = L10N.getStr("emptyVariablesText");
> >      this.Variables.nonEnumVisible = Prefs.nonEnumVisible;
> >      this.Variables.eval = DebuggerController.StackFrames.evaluate;
> > +    this.Variables.lazyEmpty = true;
> 
> How come you don't just initialize it to true in VariablesView? I see that
> head.js already sets it to false for tests, so what am I missing?

Since the VariablesView is going to be used by other things (scratchpad, webconsole), it may be unwise to have this on by default, since it may yield some unexpected behavior if the source/comments in the jsm aren't carefully read :) Also, I'm not sure yet if those other tools will ever need that functionality, so it's off by default.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Good point.

Comment 9

[Victor Porof [:vporof][:vp]]

Green. https://tbpl.mozilla.org/?tree=Try&rev=1f45cf38d9f9

Comment 10

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/293e0f58187d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/293e0f58187d