Popper-core: Positioning incorrect when popper inside relatively or absolutely positioned parent

[brentertz]

CodePen demo

http://codepen.io/anon/pen/gmRRwz

Steps to reproduce the problem

1. Instantiate a new popper, with default settings, inside of a relatively or absolutely positioned parent.
2. Scroll or resize the page so that popper hits the bottom of the viewport, thus causing it to flip to the other side.

What is the expected behavior?

Initial placement should be correct.

The popper should flip to the opposite side of the target.

What went wrong?

The initial placement is incorrect.

The popper flips, but the placement is incorrect.

Any other comments?

Not at this time.

[FezVrasta]

This is caused by the default padding applied by Popper.js.

https://codepen.io/FezVrasta/pen/LWLzRP

About the fact that the flipped popper is placed inside the reference element, this is because of the padding applied to the body, the default behavior is to treat the body as the preventOverflow parent. This is probably debatable and a PR to modify the behavior is welcome.

https://codepen.io/FezVrasta/pen/XMgepO

[brentertz]

In your latter codepen above, when the padding is removed and the flip activated, the popper is still placed over top of the reference element.

Surely not every container can be expected to have sufficient padding to allow a popper enough space to flip correctly.

FWIW, all of this flipping behavior/placement works as expected when preventOverflow: { boundariesElement: 'viewport' } is used, however I simply don't want the popper to stay in view when scrolled out of bounds.

I am unclear on what your suggestion is to work around this issue. Can you perhaps provide a little more direction?

[FezVrasta]

So, if you add a padding/margin to the body, things get messy because Popper takes the body as container of everything and it tries to avoid to make any popper overflow from it.

Actually I thought this issue was fixed with v1 but, hey, here it is 😅

I'm going to create a new issue to track it. Or feel free to create one if you prefer, I'm a bit busy atm.

[FezVrasta]

by the way, this behavior is the correct and expected one:

[FezVrasta]

https://github.com/FezVrasta/popper.js/issues/189

[brentertz]

Your comment above about the correct and expected behavior may be true when there is no more room at the top of the page/viewport, but it should not be the expected behavior when in the middle of a page and there is plenty of room above the target for the popper. In that case, the popper should render entirely above the target.

See the following codepen and toggle the 2 different modifier configurations.

https://codepen.io/anon/pen/dvRmRz

[FezVrasta]

Sorry I didn't noticed this problem.

By default preventOverflow should use as container its scrollParent, I fear it's taking the offsetParent instead for some reason?

https://github.com/FezVrasta/popper.js/blob/master/src/popper/modifiers/index.js#L53

Demo:
https://codepen.io/FezVrasta/pen/XMgEYj

[FezVrasta]

So, I investigated a bit thank to the test you added, looks like the problem is that preventOverflow is using the boundaries offset, that are relative to the document, together with the popper offsets, that are relative to its offset parent.

This situation should be normalized, making the boundaries relative to the popper's offsetParent, or making the popper offsets relative to the document.

I'm throwing here these info in case you (@brentertz or someone else) want to prepare a PR to fix this behavior. I don't know when I will have some free time to dedicate to this bug unfortunately, since I'll be in the bay area for a week in a week and I've other priorities scheduled for this week.

[miljan-aleksic]

@FezVrasta, I run into what seems the same issue. I am trying to implement Popper into Vuikit with UIkit 3, basically using it styles with popper js. As you can see in this pen the position right doesn't get placed on the right if the parent is relative. While the UIkit js does place it correctly.

You can edit the .parent style to check how it gets positioned as expected when is set as static.

BTW, UIkit use jQuery.offset({ coordinates }) to position the element.

[FezVrasta]

@miljan-aleksic I don't think I'll have time to work on this in the near future, if you or anyone else want to adopt this issue please feel free and ask me anything you need to fix the bug.

[FezVrasta]

So, I'm sharing here the work I'm doing to fix #188 and #171.

We need a way to find the common offset parent between popper and reference nodes, and this function I written seems to find it in a pretty elegant way:

function isOffsetContainer(element) {
  return element.firstElementChild.offsetParent === element;
}

function findCommonOffsetParent(element1, element2) {
  const range = document.createRange();
  if (element1.compareDocumentPosition(element2) & Node.DOCUMENT_POSITION_FOLLOWING) {
    range.setStart(element1, 0);
    range.setEnd(element2, 0);
  } else {
    range.setStart(element2, 0);
    range.setEnd(element1, 0);
  }

  const { commonAncestorContainer } = range;

  if (isOffsetContainer(commonAncestorContainer)) {
    return commonAncestorContainer;
  }

  return commonAncestorContainer.offsetParent;
}

Now I'm trying to rewrite the core of Popper to use this stuff...

[brentertz]

Nice work! I can confirm that popper.js@next fixes the issue in my aforementioned codepens.

[nblackburn]

Fixes my problems also, thanks for all of your hard work 🥇

[ChrisGao2016]

@FezVrasta
I read the code of popper.js recently. I found you change the algorithm of getting the reference position after the 1.0.8 version. I think the original algorithm still works in this case. when the boundariesNode of popper is body. You didn't calculate the offset that from boundariesNode to the offsetParent of popper is the cause of the problem.
You could change the code in the getBoundaries function to fix this bug. when the boundariesElement is not 'viewport', always run this line of code:
boundaries = getOffsetRectRelativeToCustomParent(boundariesNode, offsetParent, isFixed(popper));

[FezVrasta]

@ChrisGao2016 please may you send a PR adding a failing test case for this issue? I thought it was fixed

[ChrisGao2016]

@FezVrasta The new algorithm is nice. so the PR of old version has little meaning. Thanks for your wonderful code of popper.js.