[Popover] Warn when the height of the Popover is too tall

[amilagm]

If the popover height is larger than the viewport, the top of the popover is set to a negative value and thus clips it.

[oliviertassinari]

I don't get the extra logic. cc @eyn

[amilagm]

Hi, the logic is,
If the top ends up less than the marginThreshold (i.e. control top is actually above the viewport top), change the top to the margin threshold. Or else, just leave it.

The issue I faced is that, I have a select with many items (100+) and when you open it, the dropdown has the top set as -9900px and it clips most of the dropdown.

This is a simplified version of the code, I used the one with the || and sqrt to keep the code coverage intact because the codecov kept complaining and I was too lazy to change the tests :).

  if (top - _diff < this.marginThreshold) {
          transformOrigin.vertical += top - this.marginThreshold;
          top = this.marginThreshold
        }
        else {
          top -= _diff;
          transformOrigin.vertical += _diff;
        }

What the sqrt does is that, if the top is below marginThreshold, the marginThreshold - top will be a negative value and sqrt of that will be a NaN and Nan || 0 will give 0 (hence don't change the top).

But if the marginThreshold - top is positive (i.e. top is above marginThreshold) it will move the top to the marginThreshold level.

Hope this helps.

[oliviertassinari]

@amilagm Thanks for the extra details. Do you have a reproduction example? Or a failing test case? I don't think that we should rely on the sqrt behavior. I would rather have a dump logic, also, sqrt open the door for float imprecision issue.

[amilagm]

Hi,

Sorry I only have these screenshots,

Current - you can notice that the dropdown is moved way up and is clipped. (the actual top is set to 9900px )

Fixed:

[amilagm]

I agree with the sqrt error but I think it will be insignificant, or you can just use normal IF logic but it requires test changes to improve coverage, that's why I did not go ahead with that.

[oliviertassinari]

Thanks for the extra screenshots.

it requires test changes to improve coverage, that's why I did not go ahead with that.

It's not because the test coverage doesn't change that we don't require all bug fixes to be tested 😁 to prevent regressions.

[oliviertassinari]

@amilagm I'm finishing this PR.

[oliviertassinari]

@amilagm I have reverted your changes and added a new warning. The Popover wasn't designed to goes off screen. Worse, once an element is off screen it can't be interacted with. We block the scroll as soon as the Popover is opened.

[oliviertassinari]

@amilagm Thanks for raising this concern!

[amilagm]

@oliviertassinari Thanks,

Sorry if I'm mistaken, but wouldn't it still move the popover up ? I know you will get a warning, but is there anyway for the user to prevent the control from moving past the viewport top ?

The problem I was facing was a Select control with too many items in that and I don't have control over the number of items. So ideally I need it to start from the top of the page. I can control the max height and scroll via external css.

[oliviertassinari]

but wouldn't it still move the popover up ?

@amilagm Yes, it will. You can prevent the behavior by adding a max-height on the container. We have a demo in the documentation:

https://github.com/callemall/material-ui/blob/8860a60ea5a6ddba06b7088bd3580ba753c2b012/docs/src/pages/demos/menus/LongMenu.js#L53-L64

[cameronb23]

Any help on how to make this scrollable? I get the warning but am not sure how to proceed.

[oliviertassinari]

@cameronb23 This is a regression in beta.19. It was fixed in the v1-beta branch. Hold on for a new release.