[Menu] [Popover] Fix anchoring on active item scroll into view

[quiaro]

Fix Menu not anchored correctly if active item is deep in the scroll view

• PR has tests / docs demo, and is linted.
• Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
• Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Popover was calculating the anchor offset via getContentAnchorOffset using the index of the menu's selected item, but this made the calculation unreliable once the height of the menu list exceeded the Popover's height (i.e. clientHeight). I moved getContentAnchorOffset to Menu instead, where it's possible to know the height of the list as well.

getContentAnchorOffset calculates the anchor offset for the popover while ensuring that the menu's selected item stays anchored to the anchor element. To reduce as much as possible the need to adjust the popover's position based on the selected item, getContentAnchorOffset scrolls the selected item into view with the goal of keeping the popover centered vertically over the anchor element.

I'm very excited to make my first contribution to this great project. Big props to the maintainers :)

[stunaz]

does this fix #5937?

[quiaro]

No, it would be a separate fix for #5937.

[arracso]

Hi, I'm new using Material-UI and npm.
I'm having this bug. Will this fix be applied to npm package early?

[quiaro]

@oliviertassinari After rebasing, argos now reports a lot of regression tests failing. Looks like there are some style changes, though I did not change any styles. Is there anything I should do or is this a problem with argos failing to load some of the styles? Earlier today before the rebase, all tests were passing 😕

[oliviertassinari]

I have started a new build.

[oliviertassinari]

It's green, I'm looking into the issue now.

[oliviertassinari]

I found the corresponding issue #6731

[oliviertassinari]

I was able to get a rough fix with the following changes. Looking into that solution now.

+++ b/src/internal/Popover.js
@@ -4,6 +4,7 @@ import React, { Component } from 'react';
 import type { Element } from 'react';
 import classNames from 'classnames';
 import { createStyleSheet } from 'jss-theme-reactor';
+import Style from 'fbjs/lib/Style';
 import contains from 'dom-helpers/query/contains';
 import withStyles from '../styles/withStyles';
 import customPropTypes from '../utils/customPropTypes';
@@ -355,7 +356,10 @@ class Popover extends Component<DefaultProps, Props, void> {
     if (this.props.getContentAnchorEl) {
       const contentAnchorEl = this.props.getContentAnchorEl(element);
       if (contentAnchorEl && contains(element, contentAnchorEl)) {
-        contentAnchorOffset = contentAnchorEl.offsetTop + contentAnchorEl.clientHeight / 2 || 0;
+        contentAnchorOffset =
+          contentAnchorEl.offsetTop +
+            contentAnchorEl.clientHeight / 2 -
+            Style.getScrollParent(contentAnchorEl).parentNode.scrollTop || 0;
       }
     }

[oliviertassinari]

by rough, I mean that "my" solution have this limitation, but I have seen the same one with that PR, so I don't follow. What's the extra complexity is for?

[oliviertassinari]

@quiaro Thanks for working on that issue, it's higly appreciated 👍

[oliviertassinari]

I have applied the proposed logic in the issue, thanks for your time!

[oliviertassinari]

Let us know if we still have issues to fix.