Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Menu] [Popover] Menu is not anchored correctly if the active item is deep in the scroll view #6731

Closed
mbixby opened this issue Apr 28, 2017 · 5 comments
Labels
bug 🐛 Something doesn't work

Comments

@mbixby
Copy link

mbixby commented Apr 28, 2017

Version: next

<Menu /> with scrolling list of items is not positioned correctly if the active item is deep in the scroll view.

https://gfycat.com/ThinAffectionateAlbertosaurus

Quick fix: getContentAnchorOffset() here https://github.com/callemall/material-ui/blob/next/src/internal/Popover.js#L344 should take scrollTop into account:

function getScrollParent(node, until) { // http://stackoverflow.com/questions/35939886/find-first-scrollable-parent
  if (node === null || node === until) {
    return null;
  }

  if (node.scrollHeight > node.clientHeight) {
    return node;
  } else {
    return getScrollParent(node.parentNode);
  }
}
// ...
contentAnchorOffset = contentAnchorEl.offsetTop - scrollParent.scrollTop + (contentAnchorEl.clientHeight / 2) || 0;

Edit:

contentAnchorOffset = contentAnchorEl.offsetTop - (scrollParent ? scrollParent.scrollTop : 0) + (contentAnchorEl.clientHeight / 2) || 0;
@oliviertassinari oliviertassinari added next bug 🐛 Something doesn't work labels Jun 5, 2017
@oliviertassinari
Copy link
Member

Hey, I haven't found the time to look into this issue, but is that fix worth porting in the next branch?

@farahphillips
Copy link

I'm also having this same issue. I tested out the fix above (thanks @mbixby) and it seems to be working. Is it possible to port this fix into v1-alpha/next?

@mbixby
Copy link
Author

mbixby commented Jun 28, 2017

The fix was tested only on v1-alpha/next. If you're adopting the patch, there's a missing null check for scrollParent (see edit).

@oliviertassinari
Copy link
Member

This issue is also related to #7266.

@oliviertassinari oliviertassinari added this to the v1.0.0-prerelease milestone Jul 4, 2017
@quiaro
Copy link
Contributor

quiaro commented Jul 5, 2017

Looks like there's more to this issue then simply taking into account scrollParent.scrollTop since the active item will not persistently show in the same position within the menu. When the menu is made visible, the active item may end up appearing at the top or the bottom of the menu thereby loosing the alignment with the anchor element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

No branches or pull requests

4 participants