KDropdownMenu: Context Menu support

[AlexVelezLl]

Description

KDropdownMenu menu support to show context menus on right click setting the new isContextMenu prop to true.

Issue addressed

Closes #571

Screencast

Compartir.pantalla.-.2024-03-20.12_49_31.mp4

Changelog

• #583

  • Description: KDropdownMenu menu support to show context menus with isContextMenu prop.
  • Products impact: new API.
  • Addresses: Proposal: Enhance KDropdownMenu for Context Menu Support #571, Use KDropdownMenu to display contentNode Menus studio#4403.
  • Components: KDropdownMenu.
  • Breaking: no.
  • Impacts a11y: no.
  • Guidance: -.

• #583

  • Description: New useKContextMenu private composable
  • Products impact: - .
  • Addresses: - .
  • Components: - .
  • Breaking: - .
  • Impacts a11y: - .
  • Guidance: -.

• #583

  • Description: Expose the event object as second argument on KDropdownMenu's select event.
  • Products impact: updated API.
  • Addresses: - .
  • Components: KDropdownMenu.
  • Breaking: no.
  • Impacts a11y: no.
  • Guidance: -.

• #583

  • Description: KDropdownMenu menu support to show a header slot.
  • Products impact: new API.
  • Addresses: - .
  • Components: KDropdownMenu.
  • Breaking: no.
  • Impacts a11y: no.
  • Guidance: -.

Steps to test

1. Run the web server.
2. Go to http://localhost:4000/kdropdownmenu in the Context menu section
3. Play with the examples

Implementation notes

We only need to add a isContextMenu prop to the KDropdownMenu and it will attach a listener to the parent element to open the menu.

<template>
  ...
  <div class="card">
    ...
    <KDropdownMenu
      isContextMenu
      :options="options"
    /> <!-- every time the use right clicks the card, the menu will open -->
  </div>
</template>

At a high level, how did you implement this?

Took advantage of the tippy.js library to support the right click operation. tippy.js does not natively support a right click trigger, so I had to programmatically do a manual trigger when the user right clicked.

To track this right click I made a new private composable _useKContextMenu which will be in charge of giving us the clientX and clientY of the right click, a boolean if the context menu should be active, and ensure that there is only one context menu open at a time.

Furthermore, I Increased the UiPopover support so that it receives a position x and y where to render the popover.

There were some complications with the positioning of the popup using tippy. First I tried to use the tippy offset prop to achieve the popup positioning, but the main-axis offset was buggy and it was not working. Then I tried to use its followCursor prop to 'initial' so the popup can appear in the cursor position, but it didnt work with the "manual" trigger. And seeing in their code how they achieved the positioning of the followCursor I realized that they achieved it by rewriting the getBoundingClientRect of the popper instance, so that was what I replicated here.

Testing checklist

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical and brittle code paths are covered by unit tests
• The change is described in the changelog section above

Reviewer guidance

• Is the code clean and well-commented?
• Are there tests for this change?
• Are all UI components LTR and RTL compliant (if applicable)?
• Add other things to check for here

After review

• The changelog item has been pasted to the CHANGELOG.md

Comments

[bjester]

@AlexVelezLl I think it would be worth considering whether this is an API we want to introduce. If we make this available, and start using it, and then decide we rather have this cleanly separated between the various parts-- a menu, a dropdown, a context menu, and any composables that power them-- that could mean more refactoring down the line.

Also, creating a separate composable but then conditionalizing it through a component prop doesn't really give us the advantages of the composable-- the implementer can't modify that behavior between the logic and its representation. This is the same issue you ran into previously with KDropdown and how the Keen library uses tippy, because that makes it more monolithic than it does modular. So the value add of the composable is lost here.

[AlexVelezLl]

@bjester I was looking at what the refactor would be like, and there are some complications because the menu has an effect on the dropdown, since when choosing an option from the menu, the dropdown closes, and this would have repercussions on the implementer. We could do something like what vuetify does by using the on props in the dropdown slot, and the v-on in the menu:

<KIconButton>
  <template #menu>
    <KDropdown>
      <template #default="{ on }">
        <KMenu
          v-on="on"
        />
      </template>
    </KDropdown>
  </template>
</KIconButton>

But in my opinion, it has never seemed attractive to me that the implementer has to put those v-on connections, since that also couples the components a bit, makes the code more complex to understand, and hides the v-on's intentions (since we don't see exactly what event it listens to). So it's a lot more work for the implementer.

Another option could be that the KDropdown put inside the KButtons, and that the menu slot is to render the KMenu:

<KIconButton>
  <template #menu="{ on }">
    <KMenu
      v-on="on"
    />
  </template>
</KIconButton>

But it sounds like a lot more refactoring to have these separate components (and all the refactor we would need to do in the different products), plus we would still depend on the v-on.

We could still separate the components, in case in the future we want to have a dropdown or a context click with something inside other than a menu, but providing a concise api to the implementer for the most common use cases seems to me to be much more worth it. In this case I would propose having the new separate components: KDropdown, KMenu, KContextClick in case someone wants to use them differently. But have our components that are for the most common use cases: KDropdownMenu and KContextMenu (separate the context menu as a different component to not have the conditional composable). In such a way that if we need something like a dropdown or a menu alone and separate, we can use them, but we still have the option of having an easier to use API for the most common use cases.

In the end, in the KDropdownMenu we would have something like:

<KDropdown>
  <KMenu
    @select="closeDropdown() && emit("select")"
  />
</KDropdown>

But we would provide a much cleaner API to implementers.

[bjester]

@AlexVelezLl You bring up a good point regarding the handling of the menu activation and deactivation, because it isn't easy to structure. The question in my mind is how we build the minimal set of components and composables that an implementer can use to create these results, while also isolating components to care only about what they're supposed to do, and not what other components are doing. For instance, why does <KIconButton> need to know about the menu or potentially having a menu, and vice versa? Using composables separates the logic and data from its representation, which opens the door to the flexibility that you found missing in the previous architecture, and overall for better composition of components.

Ignoring for the moment that this <KMenuItem> doesn't exist, if you wanted to go with scoped slots, I think this would be the ideal form:

<KDropdown>
    <template #activator="{ open }">
        <KIconButton @click="open" />
    </template>
    <template #menu="{ close, positionX, positionY }">
        <KMenu :positionX="positionX" :positionY="positionY">
            <KMenuItem @click="close">Item 1</KMenuItem>
            <KMenuItem @click="close">Item 2</KMenuItem>
            <KMenuItem @click="close">Item 3</KMenuItem>
        </KMenu>
    </template>
</KDropdown>

But I think composables can be a better solution. I think the following is a good example of how you could implement this:

<template>
  <div>
    <KIconButton
        @click="open" 
    />
    <KMenu
        v-if="isOpen"
        :options="menuOptions" 
        :positionX="positionX" 
        :positionY="positionY" 
    />
  </div>
</template>
<script>
    import useKDropdown from './useKDropdown';
  
    export default {
        setup() {
            const { isOpen, open, close, positionX, positionY } = useKDropdown();
            const menuOptions = computed(() => [
                { text: 'Item 1', onClick: close },
                { text: 'Item 2', onClick: close },
                { text: 'Item 3', onClick: close },
            ]);
            return { isOpen, open, close, positionX, positionY, menuOptions };
        }
    }
</script>

What I haven't shown above is how useKDropdown would be implemented, like it would have to involve the notion of position like KDropdownMenu does.

Either way, I believe the benefits of the above are that:

• KIconButton doesn't need to know about the menu or potentially having a menu.
• KMenu doesn't need to know about the activator or potentially having an activator.
• KMenu doesn't need to relay a close event.
• The useKDropdown composable can be reused or its outputs altered as needed in the implementation.
• The useKDropdown composable can be tested in isolation.
• The items of the menu are hoisted closer to where they are defined.
• There's less nesting of components.
• It allows you to position KMenu in a way that is more flexible, like CSS relative positioning.
• In this specific example, it promotes composing these together in the implementation, which makes sense because the activator and menu are tightly coupled.

I also think this something you mentioned in your issue, but you noted that a prop could simply be added to KDropdownMenu. Obviously the above would involve lots of refactoring, but the code would still be backwards compatible. For instance, with KMenu accepting positioning and not caring the methodology behind it, you can recreate KDropdownMenu as a shim which uses KMenu and useKDropdown. This would allow you to keep the old API while also providing a new one, but the shim is just the sum of the parts, allowing for optimal flexibility. For <KIconButton>, since slot is optional, you can migrate to this newer style without breaking the old one.

In the end, I think useKDropdown is probably not too much different than what you would write for useKContextMenu. Since KContextMenu doesn't exist, you can implement the composable directly along with the new KMenu.

[AlexVelezLl]

I didn't understand very well, do you mean that this code is what the implementer uses? Or that this code is inside the KDropdownMenu?

<template>
  <div>
    <KIconButton
        @click="open" 
    />
    <KMenu
        v-if="isOpen"
        :options="menuOptions" 
        :positionX="positionX" 
        :positionY="positionY" 
    />
  </div>
</template>
<script>
    import useKDropdown from './useKDropdown';
  
    export default {
        setup() {
            const { isOpen, open, close, positionX, positionY } = useKDropdown();
            const menuOptions = computed(() => [
                { text: 'Item 1', onClick: close },
                { text: 'Item 2', onClick: close },
                { text: 'Item 3', onClick: close },
            ]);
            return { isOpen, open, close, positionX, positionY, menuOptions };
        }
    }
</script>

If it's the first, I feel like it's a lot of code that isn't relevant to the implementer. Why would the implementer want to know the positionX and positionY of the dropdowns? What happens if we would liike to have multiple dropdowns inside the same component? We would be having many more variables within the implementer code that would only be there to be passed to the KMenu and the KIconButton, the implementer would only be functioning as a delegator. And that all these lines of code would be exactly repeated every time we want a Dropdown Menu.

But beyond that, the main problem is we would have to completely refactor the way the dropdown is positioned. Until now, tipty.js has been in charge of that, it is in charge of seeing the button trigger that activated it, knowing the position of the trigger element, and setting a position for the menu, knowing how much width and height the menu has, whether there is enough space down to render it down, or whether it should render it up, etc. Tipty.js does quite a few calculations to correctly position the menu, it does not expose the clientX or clientY where the popover will be positioned. And even if it did, we still have to pass it the content that is going to be rendered so that it knows the dimensions and can position the element correctly.

But the way that is proposed to expose the positionX and positionY would practically require that we do this entire process manually and that we stop using the library, and this would be so much more work and complexity added that makes me question if it is worth it.

The popover positioning does require that it be coupled to the element that is going to be rendered, because it requires knowing its dimensions.

[bjester]

I didn't understand very well, do you mean that this code is what the implementer uses? Or that this code is inside the KDropdownMenu?

The implementer

Why would the implementer want to know the positionX and positionY of the dropdowns?

For the exact reason you ran into the issue in the first place. The components are currently less modular, and more monolithic. It does make sense for the implementer to know the position because only the implementer knows the exact constraints that exist in implementing that particular menu. You wanted to use KDropdownMenu as a context menu, but were unable to because of this exact reason-- you could not have your component choose how to position the menu.

What happens if we would liike to have multiple dropdowns inside the same component?

This approach promotes coupling, to couple each's logic and implementation into individual components, promoting better, more focused component architectures.

We would be having many more variables within the implementer code that would only be there to be passed to the KMenu and the KIconButton

how is this different from passing a multitude of props to a component? if we keep adding and adding props?

the implementer would only be functioning as a delegator. And that all these lines of code would be exactly repeated every time we want a Dropdown Menu.

Remember this is a simplistic example. I think your point of view that it would 'only be functioning as a delegator' isn't as true in a realistic example. For example, the component example would contain similar logic to Studio's ContentNodeOptions, so this actually makes sense.

And that all these lines of code would be exactly repeated every time we want a Dropdown Menu.

The alternative shifts more to the template, though, so you're not really saving much. Like with potentially more props, slots, and scoped variables to compensate? And we'd be eliminating one component overall. so in reality, the difference in the amount of code is negligible. In the long run, this makes it easier to remove tippy.js, which will make KDS smaller overall!

But beyond that, the main problem is we would have to completely refactor the way the dropdown is positioned. Until now, tipty.js has been in charge of that

I didn't propose anything on what the composables might do. They could use tippy.js or not. I said "What I haven't shown above is how useKDropdown would be implemented, like it would have to involve the notion of position like KDropdownMenu does."

The popover positioning does require that it be coupled to the element that is going to be rendered, because it requires knowing its dimensions.

This is exactly what I proposed! The implementer can directly influence this.

[bjester]

The majority of use cases probably don't even need to use the composable or tippy.js, meaning it's overkill. You can easily pass a static position and make it's container position: relative.

[AlexVelezLl]

I have worked before with menus relative to their relative-positioned parents. There are cases where it is a pain to work like this when there are specific needs of the parent like having a scroll: hidden for example (I am sure there are other issues, but that is the one that comes on the top of my mind). And that is where I have realized the reason why these libraries exist, and why they usually render these popovers with absolute position directly in the body instead of as a children of the activator.

Even vuetify does it in its VMenu in their most basic examples:

And they render the menu on a different "overlay" layer that is directly attached to the body:

And probably that is the why the first implementation of the KDropdownMenu uses the UiPopover and, in the end, tippy.js to position the menu. So I dont think is overkill, It has a reason for being.

An even vuetify provides a way to render the menu inside a parent activator. And they don't expose the positionX or positionY of the menu, nor if the menu is open or closed nor methods to close it.

With the context menu it would be the same but instead of listening on the left-clicks on the parent, we listen to its right-clicks.

And I think it's fair to bring in and compare it to what vuetify does given how big and how widely used it is, and the ease of use that it gives to its users.

[bjester]

I have worked before with menus relative to their relative-positioned parents. There are cases where it is a pain to work like this when there are specific needs of the parent to have scroll: hidden for example

@AlexVelezLl I think there's an important distinction here. I think you're talking about an implementation, when we're in KDS talking about the generic components and composables. The example I shared was an implementation of the KDS components, like something in Kolibri. So the relative-positioned use case I mentioned can be used at will as determined by the implementer and would not exist in KDS.

I would guess that your pains probably dealt with relative positioning being buried in the immutable library component you were using. I see your proposal as keeping that same monolithic view of the component. I think what you're proposing encapsulates the logic in the components, which means you definitely would have to be concerned with how it could be used, like you said with scoll: hidden. But in my example, relative positioning would happen in the implementation, not the KDS components, so that approach could be avoided in such circumstances.

Rather I'm proposing that if the implementer has a specific implementation, like you said scroll: hidden for example, it is more appropriate to implement the dropdown to fit that usage instead of trying to predict that within the generic component. This pre-optimization of the component is what I think hurts.

Regardless, those pains could be taken care of by another composable. As a composable, it can be used as necessary, and maintain the flexibility and freedom of the generic components. For instance, Studio's dropdown wrapper is an example that relates to that same scroll: hidden situation, and could be converted into a composable to accommodate for one potential downside of relative positioning. Also, if you notice, that component uses the pre-composable scoped-slot approach, which was one I shared earlier.

Overall, I think this helps to reveal the nature of composables, in that you can mix and match multiple composables together to achieve the desired result that fits the implementation, instead of building do-all components. Thereby you compose the behavior that fits the implementation, without trying to predict or channel that in the generic component. And Vue 3 this all about the composables.

Looking at Vuetify is good insight, but if we only follow what has already been done, we'll never be the ones innovating. There have been a lot challenges with Vuetify's inflexibility that we've found using it on Studio, and personally I don't view it highly in regards to flexibility.

[MisRob]

Generally I think many of us have had a good experience with composables so it may be interesting to explore them in this context. I can also hear @AlexVelezLl worries about ease of use. I think that could be resolved by having a reasonable default behavior that would fit the most common positioning use-cases, while allowing it to still be configured by something like @bjester's positionX/Y. Would this make sense to both?

[MisRob]

And that is where I have realized the reason why these libraries exist, and why they usually render these popovers with absolute position directly in the body instead of as a children of the activator.

This is another important thing I noticed in the discussion raised by @AlexVelezLl - it's something we already run into with some components, and Keen had several issues like that too. Both them and us found no other alternative than to position in the body. It seemed to be the only approach sufficiently robust. So I believe that being able to position in the body will indeed be important. However, I don't think that usage of composables would block that, right? From what @bjester says, it seems to me that composables were supposed open it up and make more easily configurable rather than limit this behavior?

Overall, I see the way we could combine all these ideas to create a really good experience, even though I didn't fathom technically what that would mean in regards to the amount of refactoring needed. Perhaps next steps may be to (1) confirm the final desired API, (2) scope refactoring needed, (3) determine priority for this work?

[AlexVelezLl]

Thank you! @MisRob

However, I don't think that usage of composables would block that, right?

The main block would be that to position the menu with tippy.js we need to have the content element rendered, the "popover" or "dropdown" side needs to know the element it has inside, so Its not feasible for us to have a v-if="open" on the child node for example. And also tippy.js is responsible for listening to the click on the activator to show and hide the content. So we wouldnt use the isOpen, open, close, positionX, positionY of the composable.

To introduce the usage of composables here to expose an api like that, would require us to eliminate the use of tippy.js and implement our own popover positioning calculations. That would not be as robust as tippy.js is, we would no longer have functionalities such as whether it is rendered up or down, or whether it is rendered at a height with an offset just enough for the menu to fit well on the screen, because we have the constraint of not knowing the content that we are positioning.

But that makes me think is if it is really worth it? We're going to do a lot of work to make the most basic use cases much more complex. What are the real benefits, already translated into code, translated into new use cases, what exactly allows us to have it in this way.

I think that the middle point to have both, the flexibility of using it in many use cases without being coupled and having a simple api without repeating so much code in the implmenter would be what I proposed a few days ago, separating the components into KDropdown KMenu, KContextClick and have a dropdown menu like:

<KIconButton>
  <template #menu>
    <KDropdown>
      <template #default="{ on }">
        <KMenu
          v-on="on"
        />
      </template>
    </KDropdown>
  </template>
</KIconButton>

Where if we want to hide the templates and the "on" params passing we can put this inside the KDropdownMenu component that can be in charge of that. With this we can keep the simple use of KDropdownMenu, but having the base components separated in the case we want to use them in other contexts.

[bjester]

would require us to eliminate the use of tippy.js and implement our own popover positioning calculations. That would not be as robust as tippy.js is

So you feel it is impossible to use tippy.js in a composable?

we would no longer have functionalities such as whether it is rendered up or down, or whether it is rendered at a height with an offset just enough for the menu to fit well on the screen, because we have the constraint of not knowing the content that we are positioning

Again I think this relates to the different viewpoints we have @AlexVelezLl, where you're viewing it as a monolithic component that 'needs to know how it's used and handle X possibilities' versus I'm proposing the components do not care how they're used and flexibility exists to implement it in a specific way as necessary through simplified props.

simple api without repeating so much code in the implmenter

But the scoped slots approach does repeat code in the implementation. You have 2+ extra lines where a composable could be just the same amount of lines.

[bjester]

From what @bjester says, it seems to me that composables were supposed open it up and make more easily configurable rather than limit this behavior?

@MisRob Yes exactly. The idea is to remove the limitations instead of perpetuating them. Also, it's worth having a forward looking eye towards where we will need to go with KDS. Incremental progress towards newer patterns, such as introduced by composables and Vue 3 will help us migrate in the future.

[AlexVelezLl]

So you feel it is impossible to use tippy.js in a composable?

Yes

Again I think this relates to the different viewpoints we have @AlexVelezLl, where you're viewing it as a monolithic component that 'needs to know how it's used and handle X possibilities' versus I'm proposing the components do not care how they're used and flexibility exists to implement it in a specific way as necessary through simplified props.

I wasnt talking about a component, but a way of calculating the positioning of an element in the screen.

But the scoped slots approach does repeat code in the implementation. You have 2+ extra lines where a composable could be just the same amount of lines.

I proposed that code to be inside a KDropdownMenu and keep the api simple as <KDropdownMenu :options="options"/> for the most common use cases we currently have, but separating the base components if we have cases like a dropdown with a different content, a menu relative positioned into something else, a context menu with a menu with different styles, etc.

[bjester]

So you feel it is impossible to use tippy.js in a composable?

Yes

I will implement the approach and show you it is not impossible!

[bjester]

I wasnt talking about a component, but a way of calculating the positioning of an element in the screen.

Right, you're talking about the logic, which should be a composable :)

[marcellamaki]

I am going to schedule a call for next week for us (@AlexVelezLl @bjester @MisRob) to hash out the requirements in detail, and then decide next steps, because trying to achieve this through the comments is going to be too challenging.

[bjester]

Thanks @marcellamaki

[marcellamaki]

After much further discussion, we will be moving forward with merging this PR, and Alex (with support from Misha and I) will open a follow up issue that will lay out some paths forward towards composable usage and further defining (or updating) our Menu related API, potentially connected to Liana's work around floating-ui. 🎉

Before I approve and we proceed though.... Since this is targeting to release-v3, I'm wondering if it should be retargeted to v4? @AlexVelezLl @MisRob? Or, should we keep it here, merge and QA the corresponding Studio work, and then later to a merge up to v4 (and upgrade Studio unstable. Right now I think it's just hotfixes that is on KDSv4). The rebrand is breaking my brain 🫠 😂

[AlexVelezLl]

Hi @marcellamaki, thank you!

Since this is targeting to release-v3, I'm wondering if it should be retargeted to v4?

It will depend on how far we are from merging the rebrand work into unstable. After that, we would have to rebase studio-usability-enhancements, and then rebase the PR branch of the change corresponding to introducing these changes into Studio. If we release it in v3, we don't have to wait for the rebrand work to be merged into unstable and we won't have to do two rebases, just one.

[MisRob]

No problem to do one last v3 release if it will make things simpler ;)

[MisRob]

(this means it will also be available in v4, so any product branch can use it from any of these two releases)

[marcellamaki]

Approving this iteration of context menu support! Alex has an initial follow up issue drafted, which is referenced above, that KDS circle will continue to build out collaboratively as we discuss as a group and consider things like the API we want, possible connections to other libraries and work, tippy/popper upgrades to floating-ui, etc.