Last Comment Bug 796009 - [markup panel] Pressing the left arrow key should close parent if on an already-closed node in markup panel
: [markup panel] Pressing the left arrow key should close parent if on an alrea...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 22
Assigned To: J. Ryan Stinnett [:jryans] (use ni?)
:
Mentors:
Depends on:
Blocks: 796015
  Show dependency treegraph
 
Reported: 2012-10-01 11:08 PDT by Heather Arthur [:harth]
Modified: 2013-03-07 03:07 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Improve left and right navigation (2.68 KB, patch)
2013-02-28 19:58 PST, J. Ryan Stinnett [:jryans] (use ni?)
jwalker: review+
Details | Diff | Splinter Review
Patch v2 (2.71 KB, patch)
2013-03-03 20:01 PST, J. Ryan Stinnett [:jryans] (use ni?)
jwalker: review+
Details | Diff | Splinter Review

Description Heather Arthur [:harth] 2012-10-01 11:08:09 PDT
If you currently have a node selected in the markup panel that isn't expanded, pressing the left arrow key (close or un-expand node) will do nothing.

We should close the parent node if that's the case.
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-01 11:23:04 PDT
Or we could just navigate to the parent. That way we can kill 2 bugs in one, and we can still close a deeply nested tree by hammering on LEFT.
Comment 2 J. Ryan Stinnett [:jryans] (use ni?) 2013-02-28 19:58:30 PST
Created attachment 719810 [details] [diff] [review]
Improve left and right navigation
Comment 3 J. Ryan Stinnett [:jryans] (use ni?) 2013-02-28 20:02:05 PST
I've given additional functionality to both the left and right arrows:

* Left Arrow on a collapsed node will move its the parent node
* Right Arrow on an expanded node will move to the first child (if any)
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-03-01 02:25:44 PST
Comment on attachment 719810 [details] [diff] [review]
Improve left and right navigation

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

It looks to me that this patch doesn't do what the title asks i.e. it doesn't "close parent", it navigates to the parent. Not that I'm complaining - I think that navigate to parent is what windows has done for a while and what osx has been doing more recently. Harth, are you happy with this modification?
Comment 5 Victor Porof [:vporof][:vp] 2013-03-01 02:36:48 PST
(In reply to Joe Walker [:jwalker] from comment #4)
> 
> - I think that navigate to parent is what windows has done for a while and
> what osx has been doing more recently.

Isn't "navigate to parent and collapse" the more pertinent behavior? I thought that was the default action (at least on OS X). In the interest of keeping keyboard navigation in trees consistent across all our tools (profiler, variables view), maybe it'd be a good idea to implement it here as well.
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-03-01 03:42:11 PST
(In reply to Victor Porof [:vp] from comment #5)
> (In reply to Joe Walker [:jwalker] from comment #4)
> > 
> > - I think that navigate to parent is what windows has done for a while and
> > what osx has been doing more recently.
> 
> Isn't "navigate to parent and collapse" the more pertinent behavior? I
> thought that was the default action (at least on OS X).

The OS X 10.8.2 Finder does navigate when it can't collapse, and collapse when it can, which is what this patch does.
Mail.app has a combined navigate+collapse action as you suggest.
iTunes (as you might expect) does something utterly bizarre and incomprehensible.
At this point I stopped looking because we obviously can't trust Apple to lead here.

Windows is (mostly) consistent in having separate navigate and collapse actions.

> In the interest of
> keeping keyboard navigation in trees consistent across all our tools
> (profiler, variables view), maybe it'd be a good idea to implement it here
> as well.

FWIW, the profiler on a Mac has separate navigate and collapse actions. I can't test variables view easily, but perhaps the XUL tree control is doing something different for different OSs? (But then you're on a Mac?)
Comment 7 J. Ryan Stinnett [:jryans] (use ni?) 2013-03-01 11:48:56 PST
(In reply to Joe Walker [:jwalker] from comment #4)
> It looks to me that this patch doesn't do what the title asks i.e. it
> doesn't "close parent", it navigates to the parent. Not that I'm complaining
> - I think that navigate to parent is what windows has done for a while and
> what osx has been doing more recently. Harth, are you happy with this
> modification?

Yeah, I was attempting do what you alluded to in comment 2, namely to combine the request of this bug and bug 796015 (to improve navigating to parent).

(In reply to Victor Porof [:vp] from comment #5)
> (In reply to Joe Walker [:jwalker] from comment #4)
> > 
> > - I think that navigate to parent is what windows has done for a while and
> > what osx has been doing more recently.
> 
> Isn't "navigate to parent and collapse" the more pertinent behavior? I
> thought that was the default action (at least on OS X). In the interest of
> keeping keyboard navigation in trees consistent across all our tools
> (profiler, variables view), maybe it'd be a good idea to implement it here
> as well.

On OS X 10.8.2, it looks like the other tools disagree with each other:

* Profiler (same as markup with my patch)
  * When moving forward with right arrow, expand and navigate are separate steps
  * When moving back with left arrow, collapse and navigate are separate steps
* Variables View
  * When moving forward with right arrow, expand and navigate are separate steps
  * When moving back with left arrow, collapse and navigate happen simultaneously

I personally prefer them as separate steps myself, but I agree that whatever we go with, we should get all our tree views consistent.
Comment 8 J. Ryan Stinnett [:jryans] (use ni?) 2013-03-03 20:01:51 PST
Created attachment 720513 [details] [diff] [review]
Patch v2

Updated patch to match commit message format. Sorry for missing this, nothing else has changed.
Comment 9 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-03-04 04:33:13 PST
(In reply to J. Ryan Stinnett [:jryans] from comment #7)
> On OS X 10.8.2, it looks like the other tools disagree with each other:
> 
> * Profiler (same as markup with my patch)
>   * When moving forward with right arrow, expand and navigate are separate
> steps
>   * When moving back with left arrow, collapse and navigate are separate
> steps
> * Variables View
>   * When moving forward with right arrow, expand and navigate are separate
> steps
>   * When moving back with left arrow, collapse and navigate happen
> simultaneously
> 
> I personally prefer them as separate steps myself, but I agree that whatever
> we go with, we should get all our tree views consistent.

Thanks for this analysis:

I raised bug 847349 - Variables View should have left arrow do collapse and navigate in separate steps.
Comment 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-03-04 04:35:19 PST
Heather, are you happy with this? It's going on my list of things to get landed.
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-03-04 07:54:44 PST
https://tbpl.mozilla.org/?tree=Try&rev=089f57640fc8
Comment 13 Panos Astithas [:past] 2013-03-06 23:29:24 PST
https://hg.mozilla.org/mozilla-central/rev/09e85bef5d8d
Comment 14 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-03-07 03:07:55 PST
Thanks for the patch Ryan

Note You need to log in before you can comment on or make changes to this bug.