feat(Popup): show on contextmenu

[jurokapsiar]

Often, what appears to be just menu is not only a menu. Below are two examples that are called "Menu" by the component libraries, but are actually combining Menu with other components in a Popup - Semantic UI and Fabric:

There are two use cases for context menu (or ... menu):

• Simple menu using only menu items
• Augumented menu that also has a search box or other elements

It makes sense to allow both and the plan to do that is:

• allow the Popup to open on contextmenu event
• reuse the Popup and create "ContextMenu" component based on this (name is not final and will be discussed as apart of the actual PR that introduces it)

Reasons to reuse Popup in Context menu:
Both create a new surface - intline or portal
Both have trigger element
Both can have similar events opening and closing: click, hover, focus (contextmenu)
Positioning - above, below
Alignment
Click outside to close
Opening children surfaces
Focus trap, auto focus functionality

What is not currently in Popup:

1. Positioning around cursor
2. Context action in popup

It makes sense to add both of these two and allow to create augumented menus using Popup.

Right click popup cannot be easily repositioned as it is attached to a fixed point (mouse position). There are two ways how the libraries or apps are handling it:

• dismiss the menu/popup when user scrolls
• do not allow scrollwhile the popup is open

Currently it is easier just to dismiss, but we will add an option to prevent scrolling: #1579

[codecov]

Codecov Report

Merging #1524 into master will decrease coverage by 0.08%.
The diff coverage is 58.18%.

@@            Coverage Diff             @@
##           master    #1524      +/-   ##
==========================================
- Coverage   71.31%   71.22%   -0.09%     
==========================================
  Files         855      857       +2     
  Lines        7052     7094      +42     
  Branches     2008     2048      +40     
==========================================
+ Hits         5029     5053      +24     
- Misses       2017     2035      +18     
  Partials        6        6

Impacted Files	Coverage Δ
packages/react/src/lib/isRightClick.ts	100% <100%> (ø)
...omponents/Popup/createReferenceFromContextClick.ts	12.5% <12.5%> (ø)
packages/react/src/components/Popup/Popup.tsx	65.71% <57.57%> (-0.74%)	⬇️
packages/react/src/lib/positioner/Popper.tsx	87.75% <80%> (-1.38%)	⬇️
...lib/accessibility/Behaviors/Popup/popupBehavior.ts	93.75% <85.71%> (-2.41%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb8e686...b991d24. Read the comment docs.

[sophieH29]

I am afraid that we are overcomplicating Popup with too many things it can do. Hence, make it hard for the consumer to figure out all the API and abilities of the component.
We might need to discuss all the trade-offs here. But how about creating <ContextMenu> component, it can render content using <PopupContent> as well. It's very straightforward from the component's name what are the capabilities - open menu on the right click.
@jurokapsiar & folks, what are your thoughts?

[jurokapsiar]

too many things it can do

This PR only adds the possibility to open the popup on right click. It can already open on focus, hover, click and combination of them so I don't think it is a big addition to the API. I agree that <ContextMenu> should be separate, but it is not part of the PR. <ContextMenu> can be used if you only have menu items in it. If there is more complicated structure with other focusable elements, <Popup> would still need to be used.

[bmdalex]

we are overcomplicating Popup

how about creating <ContextMenu> component

I agree with @sophieH29 that a separate component makes sense.

@jurokapsiar can you please update the description of the PR with the problems and user scenarios we're trying to solve based on what we discussed offline?

I did a bit of research and here is what other libraries are doing:

1. Use a context menu

• Fabric's ContextualMenu
• Blueprint's Context menu
• Ant Design's Dropdown with Menu overlay and contextMenu trigger

2. Have a select / overflow menu (that we can customize to work for right-click)

• Carbon Design System's Overflow Menu
• Vuetify's Menu
• Evergreen's Select Menu
• Material's MenuList composition

Let's take the best out of these and come up with best compromise based on the user scenarios we're trying to solve.

[jurokapsiar]

@Bugaa92 we had a discussion offline and the agreement was indeed to create a separate component for the context menu, but to reuse the Popup for that. This PR only includes changes necessary for the Popup, there will be a followup PR with the rest. I updated the description with the reasons.

[sophieH29]

Jush sharing my thoughts. This prop seems to be only needed to define if set tabindex to trigger or not. Frankly, for me a bit confusing from the prop name what it is for.
If this prop is false, the trigger will be set attribute tabindex=0, what will make it tabbable, nit just focusable.
As one of the options:

1. we can have a prop shouldTriggerBeTabbable and make it true by default
2. Or, utilize attribute data-is-focusable. If it set to "false" to trigger, in popupBehavior check it and do not set tabindex. Though it needs proper documentation, but kind of consistency of using it (we use it for FocusZone)

Maybe other folks will have thoughts around that :)

[jurokapsiar]

I like the first option, seems cleaner

[mnajdova]

In my opinion the second option is better, because we already have a mechanism for this, why introduce a new one. If we continue with this new pattern, we will start adding tabbable props on different components, and it's not something we want to do...

[jurokapsiar]

But that means that data-is-focusable prop would add data-is-focusable attribute as well as add tabindex. It would make that prop both handled and unhandled, do we really want that?

[mnajdova]

What is the use case of this prop? If this is connected with the 'context' value on the on prop can we use that one instead?

[jurokapsiar]

it's not only related to context value. I have renamed the prop, hopefully it is more clear now.

[layershifter]

1. Popup flies in unpredictable way
2. A new Popup is not closed on right click

[sophieH29]

Just noticed that we have a paragraph on right-click support in documentation, would be great to update it too https://stardust-ui.github.io/react/accessibility#right-click-support

[layershifter]

#1524 (review)

I merged the latest master and issues that were mentioned in that comment are still present 😭

@jurokapsiar can you please check?

[jurokapsiar]

@Bugaa92, @layershifter thanks for the review. I have resolved your comments. For your points:

• preventing scrolling mechanism does not work when opening popups on context
• right click does not dismiss context popups; what's the expected behavior?

1 I was not able to repro but I will check with you tomorrow to see if it's not version specific behavior - but I checked chrome and FF
2 is fixed now, probably some forgottnen commit, I thought I already fixed that

[layershifter]

Yep, second issue is fixed 🎉

However, scrolling behavior is different in IE/Edge/Firefox and Chrome.

Chrome

Edge

But, as I remember we agreed to hide it on scroll...

[layershifter]

To fix CI, please update build/gulp/tasks/test-projects/rollup/rollup.config.js:

 namedExports: {
        'node_modules/keyboard-key/src/keyboardKey.js': [
        // items
          'PageDown',
          'PageUp',
      ]
}