Closed
Bug 847349
Opened 8 years ago
Closed 8 years ago
Variables View should have left arrow do collapse and navigate in separate steps
Categories
(DevTools :: Console, defect)
DevTools
Console
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 23
People
(Reporter: jwalker, Assigned: jryans)
Details
Attachments
(1 file, 2 obsolete files)
11.80 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
See bug 796009. Especially https://bugzilla.mozilla.org/show_bug.cgi?id=796009#c7
Reporter | ||
Updated•8 years ago
|
Component: Developer Tools → Developer Tools: Console
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Whiteboard: [land-in-fx-team]
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/16356f0121dd
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 7•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/16356f0121dd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•