Closed Bug 847349 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: jwalker, Assigned: jryans)

Details

Attachments

(1 file, 2 obsolete files)

Component: Developer Tools → Developer Tools: Console
Assignee: nobody → jryans
Status: NEW → ASSIGNED
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: https://tbpl.mozilla.org/?tree=Try&rev=02dff0572c3c
Attachment #734696 - Attachment is obsolete: true
Attachment #735882 - Flags: review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/16356f0121dd
Status: ASSIGNED → RESOLVED
Closed: 7 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.