796009 - [markup panel] Pressing the left arrow key should close parent if on an already-closed node in markup panel

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Heather Arthur [:harth]]

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]]

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 needinfo, replies may be slow)]

Attachment: Improve left and right navigation

Comment 3

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

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]]

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]]

(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]]

(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 needinfo, replies may be slow)]

(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 needinfo, replies may be slow)]

Attachment: Patch v2

Updated patch to match commit message format. Sorry for missing this, nothing else has changed.

Comment 9

[Joe Walker [:jwalker]]

(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]]

Heather, are you happy with this? It's going on my list of things to get landed.

Comment 11

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=089f57640fc8

Comment 12

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=7eb5914cbb5d
https://hg.mozilla.org/integration/fx-team/rev/09e85bef5d8d

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/09e85bef5d8d

Comment 14

[Joe Walker [:jwalker]]

Thanks for the patch Ryan