System menu toggle

[corwin-of-amber]

Seems reasonable that clicking the Menu (top left) opens the system menu, then another click closes it. Currently the second click does nothing (in fact it closes and opens the menu, but this is not observable).

This suggestion depends on a second fix to @osjs/gui.

[corwin-of-amber]

Ok this was a bit more complicated than I anticipated, because it means now there is an extra <div> between the osjs-panel-item and its contents. (The alternative was to put it between the osjs-panel and the osjs-panel-item, which IMO is even worse.)

So I had to change the CSS to account for that... if this is fixed e.g. by using Hyperapp 2 then the CSS can be rolled back.

[andersevenrud]

Gah, I'm trying to do this on mobile using desktop web mode, and it's so hard :laughing: The previous comment was supposed to be a request change, not just a comment. Also I didn't finish my comment....

[previous comment ended here] But that's an improvement for the [near] future.

[andersevenrud]

This PR will affect a rule in the theme(s) as well:

https://github.com/os-js/osjs-standard-theme/blob/master/src/_desktop.scss#L39-L43

[corwin-of-amber]

This PR will affect a rule in the theme(s) as well:

Must say, I haven't even noticed that the opacity of the window panel changes... 😄 Also, you have an asterisk there bud 🤓

[corwin-of-amber]

Here too? Or is this one supposed to still reference the direct child div (which now will be the .osjs-panel-item-content...)

Ugh I really wanted to keep the DOM structure as it was but Hyperapp just wouldn't let me...!

https://github.com/os-js/osjs-standard-theme/blob/bfee148198d4a9e87aa8bcd558e07533d7d3ab62/src/_animations.scss#L47

[andersevenrud]

Also, you have an asterisk there bud :nerd_face:

Hehe, indeed. I believe its because the window panel item thingy has two child elements of different tagName-s and I was lazy. Since their parent is what has the white background (by default) the opacity was not applied directly there.

[andersevenrud]

Here too?

Where's this ? Can't see a reference here :thinking:

[corwin-of-amber]

Where's this ? Can't see a reference here 🤔

Ah my bad, it's there now.

[andersevenrud]

I've been thinking about this for a while now, and I think a possibly better solution to handle the toggling is add this into ContextMenu itself. This gets rid of the local state entirely as well as the setTimeout. In the changes below all cases are covered, however in the context of the Menu panel item it would still require the position change to work as expected.

Thoughts ? :)

// osjs-gui/src/contextmenu.js
// init() method

    let lastPosition = {};

    this.actions = app({
      /* ... */
    }, {
      show: (options) => (props, actions) => {
        /* ... */
        const sameLeft = position.left === lastPosition.left;
        const sameTop = position.top === lastPosition.top;

        lastPosition = position;

        if (sameLeft && sameTop) {
          actions.hide();
          lastPosition = {};
          return;
        }

        /* ... */
      }
    } /* ... */)

[corwin-of-amber]

Thoughts ? :)

It is rather short and sweet, although keying the menu by its position does produce some code smell. Is there anything else by which the menu can be keyed? A unique identifier can be passed in the call. Also, having a method called show() that in fact either shows or hides the menu is a bit weird, isn't it?

[andersevenrud]

although keying the menu by its position does produce some code smell

Whatever is passed as a position gets converted into a {top,left} object.

Is there anything else by which the menu can be keyed?

Not at the moment. It's just displayed from some kind of event subscription, then immediately disposed of if click occurs that does not collide with the context menu DOM element(s).

Also, having a method called show() that in fact either shows or hides the menu is a bit weird, isn't it?

Yeah, you're right here. But this behavior could be activated with an option maybe ?

[corwin-of-amber]

Yeah, you're right here. But this behavior could be activated with an option maybe ?

I would suggest maybe an option toggle: unique_identifier.

[andersevenrud]

I would suggest maybe an option toggle: unique_identifier.

Is it even necessary to have a unique identifier here ? Maybe I'm still not thinking straight, but if this simply was a boolean it would still work as expected. Since when you bring up the contextmenu upon another coordinate after it was brought up with toggle: true it would never actually get to the "same position" condition. :thinking:

[corwin-of-amber]

the "same position" condition.

Yes, I was thinking of how to avoid using the position as the menu's identity. But maybe I'm overthinking it. Position is probably good enough, as long as for the same element it would be consistent.

[andersevenrud]

I was thinking of how to avoid using the position as the menu's identity. But maybe I'm overthinking it.

The contextmenu actually stays alive in the DOM at all times and is only repainted whenever it's brought up (show()), and at that point the visibility+position is toggled via CSS.

[andersevenrud]

So the position would be its "identity" I suppose. Seeing how it's immediately disposed of afterwards. Does this make any sense ? :P

[corwin-of-amber]

Uhm but there is a single context menu for the entire client UI, right? I guess it's worth considering. If a context menu is opened in some app, and then I click "Menu", do I expect the menu to open? I just checked on my Mac and (I was a bit surprised) clicking anywhere when a context menu is open only hides the menu and "swallows" the click event. I guess this makes sense somehow and perhaps it's even a better UX, so that could be the behavior in OS.js as well.

[andersevenrud]

I fired up a Windows 10 VM and the clicks never gets swallowed there. Not sure how it would be in Gnome or KDE ex. (I'm on i3 and don't really have any menus) :thinking:

[corwin-of-amber]

IMHO either is fine (fact is, neither of us noticed the diverging behavior). Implementing "swallow" click behavior is slightly easier, since otherwise you have to remember which component initiated the currently active menu – although using the position is an acceptable proxy to that.

[corwin-of-amber]

An orthogonal question, perhaps, is whether you want to have the div.osjs-panel-item-content there or not. Using ev.currentTarget and identity-by-position covers the menu toggle functionality, but there may be some other stability pitfalls down the line that the PanelItem.$element creation may prevent.

[corwin-of-amber]

So. I did the "swallow" semantics in @osjs/gui and I'm comfortable with it. I will create another PR there and you can then tweak the condition to your liking. I will update this branch to use the new option, toggle: true.

[andersevenrud]

Could you possibly split this up into two separate pull requests ? One for the menu toggle, then another for the DOM changes ?

[andersevenrud]

I just published a new version of @osjs/panels with these changes.