Fix bug where ContextMenu items were being improperly positioned

[ghsolomon]

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

• Caught up with develop branch as of last change.
• Added CHANGELOG entry, or determined not required.
• Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
• Updated doc comments / prop-types, or determined not required.
• Reviewed and tested on Mobile, or determined not required.
• Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

[ghsolomon]

Fixes #3723 . Issue appears to be with the following CSS style:

hoist-react/kit/blueprint/styles.scss

Lines 112 to 117 in c117858

	// Constrain menu height to the viewport, for menus with more items than will fit vertically.
	// Note that this is not perfect - menus are shown relative to their target, and if a menu is
	// in the middle of the page and forced to render above/below the target, it can still clip.
	// The 100px offset is designed to account for menus shown from app/tab-level top/bottom toolbars.
	max-height: calc(100vh - 100px);
	overflow-y: auto;

By applying a max-height and overflow-y: auto to the Menu container, sub-menus which are rendered in Popovers do not get positioned properly. This can be remedied by rendering the submenu popovers in portals. Per BP's docs:

Using a Portal is necessary if any ancestor of the target hides overflow or uses very complex positioning.
https://github.com/palantir/blueprint/blob/9754f3d6e/packages/core/src/components/popover/popoverSharedProps.ts#L315

This is true by default for Popovers, but overridden by BP for MenuItems so that submenus will live in the same container per BP's docs:

popoverProps: Props to spread to the submenu popover. Note that content and minimal cannot be changed and usePortal defaults to false so all submenus will live in the same container.
https://blueprintjs.com/docs/#core/components/menu.menu-item

An alternative solution would be to remove the max-height and overflow-y: auto styles we apply to Menu. This would cause clipping when the menu height exceeds that of the viewport but would allow us to keep the existing usePortal: false behavior, which does have some benefits -

Not using a Portal can result in smoother performance when scrolling and allows the popover content to inherit CSS styles from surrounding elements

[amcclain]

Thanks for diving into this, Greg. I'm assuming since you setup the PR like this, you are thinking that the portal is the best overall option. (I am inclined to agree.)

The style rules to ensure we don't overflow the viewport height are important for menus in general, as it is not difficult to end up with a big menu that can exceed the height of the viewport on a short screen or compressed window - we have seen this a number of times with the primary app menu, and it's one of those badly broken outcomes we really want to avoid (i.e. it looks really silly).

We could consider not applying that rule to context menus in particular - those do seem less likely to overlfow vs. the main app menu - but if the portal approach works OK and we aren't seeing janky scrolling, etc. let's go for this. I'll merge it now and we can keep an eye out.

[ghsolomon]

Agreed - those were my thoughts as well. It would be nice if BP built overflow handling into Menu, but I think this is a reasonable workaround.

[TomTirapani]

Thanks for the research and for this fix, looks great!