I'm not sure this design is more usable than current hierarchical layout, because, assuming that the submenu is opened on enter and closed on exit, moving the pointer down would first open the submenu, then as soon as the pointer is on the item immediately next the submenu, this would close and move up all the following items, making it difficult to point and click items following the submenu.

(This is the same problem accordions have, but they solve it having all items the same width / height, which does not apply here)

The above issue could be addressed making the submenus open on click, behaving similarly to disclosure triangles.

Two additional questions:

1) What about keyboard navigation?
Do we automatically open the menu when pressing down on the submenu item? Do we require pressing Right (or Left or Space or Return or whatever)? Do we automatically open the menu as soon as the submenu item is focused?

2) When does the submenu close?
Never? On click of the submenu items? After some timeout?

For mouse navigation, I'm assuming:

  - Clicking a parent menuitem opens or closes its submenu.

  - Opening one submenu closes any other currently-open submenu

  - Clicking on a submenu item activates that item and closes
    the entire menu

  - If you press-and-drag to get into the menu rather than just
    clicking, then probably the submenu opens after a brief hover
    over the parent menuitem (and closes only when you open another
    submenu or activate a menuitem)

And for keynav:

  - Space or return on the parent menuitem opens/closes the submenu.

  - Right arrow on a parent menuitem opens its submenu and selects
    the first item. Left arrow on a submenu item closes the submenu
    and selects its parent menuitem.

  - (Right arrow from a submenu closes the menu and opens the next
    menu in the menubar, and left from a parent menuitem closes the
    menu and opens the previous menu.)

  - Up/down navigate the visible menu items, as though there was no
    hierarchy. So in the mockup, down would take you to "Connected", and
    up would take you to "Nexus One" (without opening that submenu or
    closing the "Fishsoupmobile" one).

(In reply to comment #4)
>   - Opening one submenu closes any other currently-open submenu

although actually, that may be confusing when doing mouse navigation

Dan, those sound really well thought out.  And overall sounds very good to me.  However, I'm not sure what the following means:
  - (Right arrow from a submenu closes the menu and opens the next
    menu in the menubar, and left from a parent menuitem closes the
    menu and opens the previous menu.)

Currently (ignoring submenus), right arrow and left arrow navigate between the different status menus (just like they do in the GNOME 2.0 panel or a GtkMenuBar). When you have submenus, there has to be minor adjustments to deal with the fact that right/left also open/close submenus. The part you quote is just confirming that when one of the special behaviors *doesn't* happen, then the normal behavior does. (And again, this is parallel to how panel/gtk menus work.)

Created attachment 174913 [details] [review]
PopupMenu: handle submenus inline

Instead of showing submenus on the left side, make PopupSubMenuMenuItem
act like an expander. The sub menu is toggled on click, opened on
right/enter/space on the parent item, closed on left on any item
or when closing the parent menu.

Created attachment 175064 [details] [review]
popupMenu: Improve menu item layout

updated version of the patches formerly on 618312, minus the now-irrelevant
bits

the expansion/contraction will need to be animated. and the triangle needs to point down when the submenu is open (you can just rotate the "▶" label around its center in unison with the other tween)

I don't think the submenu "title" should stay permanently the selected color when the submenu is open; I'm assuming the only reason it's highlighted that way in the mockup is because the pointer is still directly over it

Comment on attachment 174913 [details] [review]
PopupMenu: handle submenus inline

>+.popup-sub-menu {
>+    background-color: #6c6c6c;
>+}

The mockup is quite a bit darker. (And actually, it has a gradient, although I'm not sure how important that is, or, if it is supposed to have a gradient, whether it's supposed to have a fixed height worth of gradient followed by a solid color underneath, or if the gradient is supposed to be stretched to whatever height the "submenu" takes up.)

(Also also, the mockup uses a CSS inner shadow, but we don't have support for that yet.)

>         if (sourceActor._delegate instanceof PopupSubMenuMenuItem) {
>+        } else {
>         }

it would be nicer if we could do this via a "PopupMenuBase" with "PopupMenu" and "PopupSubMenu" subclasses or something

>+                this._box.insert_actor(menuItem.menu.actor, position+1);

spaces around the +


If we add toggle switches to the submenu items as in the mockup, we're going to need to have the column layout magic recurse into the submenus as well.

Review of attachment 175064 [details] [review]:

::: js/ui/popupMenu.js
@@ +148,3 @@
             } else
+                params.column = 0;
+        }

How is params.column used, beyond setting the column for subsequently added actors?
It seems that actors are always allocated in insertion order.

@@ +297,3 @@
+            } else {
+                childBox.x1 = x;
+                childBox.x2 = x + naturalWidth;

This should be Math.min(naturalWidth, availWidth), to accomodate underallocation cases.
Also we may need to check both here and in the expand code path that minimalWidth is ensured.

(Really, we should distribute extra width among all expand actors if allocated more than requested, and reduce proportionally all actors if allocated less, but this may become too complex)

Created attachment 175321 [details] [review]
PopupMenu: handle submenus inline

Instead of showing submenus on the left side, make PopupSubMenuMenuItem
act like an expander. The sub menu is toggled on click, opened on
right/enter/space on the parent item, closed on left on any item
or when closing the parent menu.

(In reply to comment #12)
> How is params.column used, beyond setting the column for subsequently added
> actors?
> It seems that actors are always allocated in insertion order.

Yeah, looks like that never worked. I just removed it.

> +                childBox.x2 = x + naturalWidth;
> 
> This should be Math.min(naturalWidth, availWidth), to accomodate
> underallocation cases.

Actors aren't expected to deal sanely with underallocation.

> (Really, we should distribute extra width among all expand actors if allocated
> more than requested, and reduce proportionally all actors if allocated less,
> but this may become too complex)

Yeah, if we want menu min-widths to work reliably we're going to need to fix that, but that can be a separate bug.

Pushing this patch with the params.column removal, to get this bug back to being just your patches.

Comment on attachment 175321 [details] [review]
PopupMenu: handle submenus inline

OK, this is basically good; the coloring/highlighting still feels off, but the mockups don't clearly indicate how it should be in all states. We'll just wait for further guidance from the designers after it gets committed.

Three notes on the animation:

    - The first time the submenu is shown it appears instantly rather than
      tweening, because it hasn't been set up for the "hidden" state correctly

    - I don't think squishing is the right effect. The submenu should slide
      out (ie, just tween height, with clip_to_allocation: true). (Actually,
      I feel like ideally, when it slides out, the bottom of the embedded menu
      should be attached to the top of the following item, rather than having
      the top of the embedded menu be attached to the bottom of the parent
      item [as happens when you just tween height]. I'd thought that tweening
      height and anchor_y together would do this, but it doesn't; you'd need to
      add another container to do the alignment, or else add a special tweening
      property so you can tween the clip box. Probably not worth it.)

    - It would still be nice if the arrow animated (rotating). If you don't do
      it now they're just going to file a bug later. :)

Created attachment 176654 [details] [review]
PopupMenu: handle submenus inline

Instead of showing submenus on the left side, make PopupSubMenuMenuItem
act like an expander. The sub menu is toggled on click, opened on
right/enter/space on the parent item, closed on left on any item
or when closing the parent menu.

Now with rotating arrows!

Created attachment 176665 [details] [review]
fixes

If menuItem.menu gets destroyed before menuItem, then trying to
disconnect signal handlers will fail because they'll already have been
removed. So just destroy the menu instead (which will be a no-op if
it's already destroyed).

Comment on attachment 176654 [details] [review]
PopupMenu: handle submenus inline

ok to commit after squashing the attached fixes

(In reply to comment #17)
> Created an attachment (id=176665) [details] [review]
> fixes
> 
> If menuItem.menu gets destroyed before menuItem, then trying to
> disconnect signal handlers will fail because they'll already have been
> removed. So just destroy the menu instead (which will be a no-op if
> it's already destroyed).

1) PopupSubMenuMenuItem and PopupSubMenu are JS objects, so .destroy() has no effects on signals (it does not call .disconnectAll() )

2) when 'destroy' is emitted on menuItem, menuItem.menu has been already destroyed.

There is an error though: I was disconnecting on menuItem.menu an handler connected to self.

right, ok, so fix "menuItem.menu.disconnect(menuItem._closingId);" to be "this.disconnect", and then it's ok to commit

Pushed