Popper-core: Improve positioning for inline elements

[Madhu94]

If the reference element happens to have a block element positioned within an inline element (say a div inside span), popper's positioning is off by about the dimensions of the reference element.This is a problem with 1.16.x of popper - ever since this commit.

Why this used to work before the linked commit

Pasting code from 1.x https://github.com/popperjs/popper-core/blob/0a2329323252fd287f263c28951883b35095082a/packages/popper/src/utils/getBoundingClientRect.js#L51

// offsetwidth = 0 for span containing div, so horizScrollbar is negative the rendered width of the component
let horizScrollbar = element.offsetWidth - width; 
let vertScrollbar = element.offsetHeight - height; // same as for vertScrollbar

// this used to be NaN before the linked commit, so the condition evaluated to false. Now, we have  // two negative numbers so it evaluates to true.
  if (horizScrollbar || vertScrollbar) { 
    const styles = getStyleComputedProperty(element);
    horizScrollbar -= getBordersSize(styles, 'x');
    vertScrollbar -= getBordersSize(styles, 'y');

// we have the positions offset by the width of the reference element now. If the width is 500px, an //additional 500px is added due to this line.
    result.width -= horizScrollbar
    result.height -= vertScrollbar;

CodePen demo

I'm adding a codesandbox demo with react-popper here, hope that's okay -
https://codesandbox.io/s/popper-reproducer-7kor4?file=/src/App.js

Steps to reproduce the problem

Try toggling the type of the reference element node from span to div.

What is the expected behavior?

It would be good if popper didn't try to account for the scrollbar in such a situation. It places the popper somewhere far away from the reference element.

Any other comments?

Granted, putting an block element within an inline element is not valid markup and this problem is likely fixed by 2.x of popper, but it would be good to fix this in 1.x so that we don't have to request our clients to upgrade our library (after we upgrade to react-popper)?

[Madhu94]

I'm attaching a patch for a possible fix, happy to make any suggested changes and test.

diff --git a/packages/popper/src/utils/getBoundingClientRect.js b/packages/popper/src/utils/getBoundingClientRect.js
index f9e05436..7727c875 100644
--- a/packages/popper/src/utils/getBoundingClientRect.js
+++ b/packages/popper/src/utils/getBoundingClientRect.js
@@ -58,8 +58,12 @@ export default function getBoundingClientRect(element) {
     horizScrollbar -= getBordersSize(styles, 'x');
     vertScrollbar -= getBordersSize(styles, 'y');

-    result.width -= horizScrollbar;
-    result.height -= vertScrollbar;
+    // this can happen when the trigger element is inline but encloses a block element
+    if (horizScrollbar > 0) {
+      result.width -= horizScrollbar;
+    } else if (vertScrollbar > 0) {
+      result.height -= vertScrollbar;
+    }
   }

   return getClientRect(result);
diff --git a/packages/popper/tests/functional/core.js b/packages/popper/tests/functional/core.js
index ae9dd08c..6da8c244 100644
--- a/packages/popper/tests/functional/core.js
+++ b/packages/popper/tests/functional/core.js
@@ -1967,5 +1967,39 @@ const arrowSize = 5;
         });
       }
     );
+
+    // test for div inside span
+    it('works for inline elements containing block elements', done => {
+      jasmineWrapper.innerHTML = `
+        <style>
+          #popper {
+            background-color: yellow;
+          }
+          #container {
+            width: 500px;
+            background-color: red;
+          }
+        </style>
+        <div id='container'>
+          <span id='reference'>
+              <div>Reference element</div>
+          </span>
+        </div>
+        <div id='popper'>Popper element</div>
+      `;
+
+      const reference = document.getElementById('reference');
+      const popper = document.getElementById('popper');
+
+      new Popper(reference, popper, {
+        placement: 'right',
+        onCreate(data) {
+          expect(getRect(reference).top).toBeApprox(getRect(popper).top);
+          expect(getRect(reference).right).toBeApprox(getRect(popper).left);
+          data.instance.destroy();
+          done();
+        },
+      });
+    });
   });
});

[atomiks]

It does seem fixed in v2: https://codesandbox.io/s/popper-reproducer-0313q

react-popper@2 has the render props API which is largely compatible and you can use Popper v2, so you should be able to upgrade without much trouble. We aren't working on v1 anymore because we don't want to cause potential other regressions whenever we fix something, it's too much risk.

[Madhu94]

@atomiks thanks for taking the trouble to make a codesandbox for v2 ! The issue is not us upgrading but other clients who consume our library will be forced to upgrade to get the fix too.