Closed Bug 847349 Opened 8 years ago Closed 8 years ago

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


(DevTools :: Console, defect)

Not set


(Not tracked)

Firefox 23


(Reporter: jwalker, Assigned: jryans)



(1 file, 2 obsolete files)

Component: Developer Tools → Developer Tools: Console
Assignee: nobody → jryans
Attached patch Patch v1 (obsolete) — Splinter Review
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
Attachment #734446 - Flags: review?(vporof)
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.
Attachment #734446 - Flags: review?(vporof) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #734446 - Attachment is obsolete: true
Attachment #734696 - Flags: review?(vporof)
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.
Attachment #734696 - Flags: review?(vporof) → review+
Attached patch Patch v3Splinter Review
Carrying over Victor's r+ after tweaking the test as he mentioned.

Try push looks good:
Attachment #734696 - Attachment is obsolete: true
Attachment #735882 - Flags: review+
Whiteboard: [land-in-fx-team]
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.