847349 - Variables View should have left arrow do collapse and navigate in separate steps

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Joe Walker [:jwalker]]

See bug 796009. Especially https://bugzilla.mozilla.org/show_bug.cgi?id=796009#c7

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Patch v1

This makes the variables view navigation more like the markup view:

* Left arrow now treats collapse and navigate as separate steps

Also, I tentatively removed the special casing of variables and properties vs. scopes from left arrow and down arrow.  I did not follow the justification for this distinction here.  During my testing of the change above, I found it unintuitive to have down arrow expand a scope instead of moving to the next scope, as it does with variables and properties

Comment 2

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

Comment on attachment 734446 [details] [diff] [review]
Patch v1

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

I love this, the code is much simpler :)
You'll need to fix and add some testing for this. Currently browser_dbg_propertyview-data.js fails, so giving f+ for now.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Patch v2

I've fixed up the existing tests and added a few more for the new behavior.  All tests in the debugger directory are now passing.

Also, I tweaked the right arrow so that it does not advance / move focus if you are on a non-expandable item.  This makes it behave the same as the markup view.

Comment 4

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

Comment on attachment 734696 [details] [diff] [review]
Patch v2

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

Nice coding! R+ with the change mentioned below.
Thanks for the patch.

::: browser/devtools/debugger/test/browser_dbg_propertyview-data.js
@@ +661,5 @@
> +                "The bar item should be focused now.");
> +
> +              EventUtils.sendKey("DOWN", gDebugger);
> +              is(gVariablesView.getFocusedItem().name, "foo",
> +                "The foo item should be focused now.");

This behavior shouldn't be tested, it can change anytime (it's actually a bug). Use UP instead.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Patch v3

Carrying over Victor's r+ after tweaking the test as he mentioned.

Try push looks good: https://tbpl.mozilla.org/?tree=Try&rev=02dff0572c3c

Comment 6

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

https://hg.mozilla.org/integration/fx-team/rev/16356f0121dd

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/16356f0121dd