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

RESOLVED FIXED in Firefox 19

Status

defect
P3
normal
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: vporof, Assigned: vporof)

Tracking

12 Branch
Firefox 19
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Priority: -- → P3
Posted patch v1 (obsolete) — Splinter Review
Mango juice.
Attachment #673698 - Flags: review?(past)
Depends on: 707302
Posted patch v1, part2 (obsolete) — Splinter Review
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.
Attachment #673723 - Flags: review?(past)
Posted patch v1.1, part2 (obsolete) — Splinter Review
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).
Attachment #673723 - Attachment is obsolete: true
Attachment #673723 - Flags: review?(past)
Attachment #673736 - Flags: review?(past)
Posted patch v1.1, part3 (obsolete) — Splinter Review
Properties want flashing too :)

Currently only the .variables had the transition rules in the css. Copied them to the .properties as well.
Attachment #673738 - Flags: review?(past)
Posted patch v2Splinter Review
qfolded ftw.
Attachment #673698 - Attachment is obsolete: true
Attachment #673736 - Attachment is obsolete: true
Attachment #673738 - Attachment is obsolete: true
Attachment #673698 - Flags: review?(past)
Attachment #673736 - Flags: review?(past)
Attachment #673738 - Flags: review?(past)
Attachment #674172 - Flags: review?(past)
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?
Attachment #674172 - Flags: review?(past) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/293e0f58187d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 19
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.