Popper.js v2

[FezVrasta]

If anyone wants to contribute to the Popper.js v2 rewrite please do so sending PRs against the next branch.

It's a completely new codebase, I think the current codebase is too messy to continue to work on it, it started as a little proof of concept and ended up being a full fledged library, I think it deserves a complete rewrite.

The idea is to copy the good code from the old version and write new one to make the new code more stable.

I'd like to make the library even more performant by better leveraging to an internal cache the modifiers can use to get most of the values they need that have been already read from the DOM.

The only decisions I made so far are:

• prettier to format the code
• Flow to type check it
• New system to order modifiers [WIP] (discussion)

To do:

• CI
  • add a built step (Rollup + Babel)
  • add a CI setup
  • add unit tests (Jest)
  • add functional/integration cross-browser tests (Karma?)
• Core
  • rewrite the positioning engine to work on all the supported cases (and more?)
  • rewrite the modifiers API
• Modifiers
  • computeStyles
  • applyStyles
  • arrow
  • preventOverflow
  • flip
  • offset/shift (merge them together)
  • other modifiers will be added later, if anyone wants them please send a PR to implement them
• Miscellaneous
  • Implement the changes labelled as BREAKING CHANGE

If you'd like to contribute please write here so others know who's working on what.

[lukebatchelor]

Commenting here to follow progress.

Exciting stuff!

[inomdzhon]

Do we really need set undefined for property in getElementClientRect.js when no element? 🤔

Why we can't return 0 or make element required, that is can't be null.

And we need set return types there — getBoundingClientRect return ClientRect | DOMRect.
I can make PR for this, but my first question is still relevant.

[FezVrasta]

I don't think it's useful to return 0 in case the browser can't return the correct values. Better make it clear something is wrong and return undefined no?

[inomdzhon]

I think if we expected number, we need get number, but not undefined or something else. It's make function more clear.

[FezVrasta]

It's common practice to return a null value if it's not possible to return the expected type 🤷‍♂️

Better than letting think the user that everything is fine and it actually isn't.

[inomdzhon]

Hm, yes, I agree. But I would say that depends on the situation.

Anyway, I will try to help with PR.

[FezVrasta]

Positioned by PopperJS 2! (gonna change the name to remove that dot lol)

[FezVrasta]

Just to share a bit of updates on the status of the next branch:

Right now there aren't any modifiers included, if you open the basic.html example you can see I created a super simple applyStyle modifier to showcase the current status (the screenshot above).

We need to add support for all the logic that handles the possible different contexts a popper may live in (scrolling parent, overflow hidden etc).

I really want to reach the point where we'll have a rock solid core positioning logic, independent by any additional modifier. So that we can confidently write additional code (modifiers) on top of it and never worry to have wrong results from the core computations.

Also, secret wish, make PopperJS compatible with react-native 🤭Ideally if we keep a good separation between DOM utilities and the rest, we should be able to write react-native specific utilities to replace the DOM ones. And reuse all the rest of the library

[FezVrasta]

Daily update, initial support for nested scroll parents:

[donnysim]

Will it be possible to import separate methods from popper? like get scroll parent, get position etc.? Sometimes there's a need of such when dealing with manual implementations, so this would be super awesome not having to write/copy the logic again. Just a wish :)

Edit: nvm, I see it does separate exports :)

[FezVrasta]

Sorry for the lack of updates, life been busy recently.

I just finished to update the branch to work with the latest version of the dependencies (babel, jest etc).

I just published the next branch to popper.js@next so that everybody can try it on their own projects and report back any bugs.

As I mentioned earlier, right now there aren't any modifiers included, not even the applyStyle one, so you have to manually include it as follows:

  new Popper(
    reference,
    popper,
    {
      placement: 'right',
      modifiers: [
        {
          name: 'applyStyle',
          phase: 'write',
          enabled: true,
          fn(state) {
            console.log(state);
            popper.style.top = `${state.offsets.popper.y}px`;
            popper.style.left = `${state.offsets.popper.x}px`;
            return state;
          }
        }
      ]
    }
  );

[FezVrasta]

I published 2.0.0-next.3

It finally ships with some modifiers (computeStyles and applyStyles), so you can now use it without any additional configuration required.

If anyone wants to write additional modifiers please do so, it'd be super helpful!

For now, we got a completely type safe code base, easy to understand (I hope) and which once minified weights as little as 2.42 kB!

[FezVrasta]

2.0.0-next.4 is here, the positioning algorithm is now more solid, it'd be helpful if you guys sent PRs to add more visual test cases (failing or not) so that I can improve the algorithm to address them or avoid to break them while I work on it.

[atomiks]

2.42 kB is insane, great work!!

[FezVrasta]

Thanks, now the next challenge is to write the preventOverflow modifier. As usual if someone wants to give it a shot please do so. I'll be on vacation for a week so I will not be able to work on it during that time.

[brunolemos]

Also, secret wish, make PopperJS compatible with react-native 🤭Ideally if we keep a good separation between DOM utilities and the rest, we should be able to write react-native specific utilities to replace the DOM ones. And reuse all the rest of the library

What's the current state of react-native compatibility?
Related: #357 (@slorber)

[FezVrasta]

I haven't yet experimented with it. Developement of v2 is going slowly unfortunately.

[atomiks]

Will you be getting some time to work on this or is it postponed indefinitely for now? 🥺

[FezVrasta]

The prevent overflow modifier is yet to be written, it's the most important piece still missing.

For some reason I can't figure out how to write it ☹️

[atomiks]

Maybe you can do a 2.0 release that fixes the common current API painpoints, and schedule a complete rewrite (considering it seems very difficult) for a minor release? Basically, try to restructure the code in a way that allows for a better/smaller architecture in the future without completely throwing out the existing code, assuming that is possible.

Dev + prod versions without the warning messages in production would also cut down on some of the existing size to help justify it a bit.

[FezVrasta]

I don't have bandwidth to work on the current code base, and it's so messy that I fear I'd break something. 😔

[garygreen]

@FezVrasta out of interest will v2 be treeshakeable? I remember the humble days of old versions where Popper was more lightweight but now as you mention it's turned into almost a full blown framework. It would be awesome to be able to just import the basic functionality that you need if you just want the bare basics to position a tooltip around an element.

[FezVrasta]

It will be treeshakeable 👌

[FezVrasta]

If anyone is interested in helping me out with the prevent overflow logic, I created a PR to show the progress.

#793

Any help is very appreciated.

[atomiks]

Yeah it still comes up with the config folder error without any options or ./src/ at the start of include

[atomiks]

We're pretty much finished now btw – auto placements was the last feature to add and is now merged. alpha.4 is now published.

🕒alpha.4 is likely the last or second last before the beta. If you have been using the alpha so far, please comment with any issues you have with the API so we can make final breaking changes if necessary!

[RobbieTheWagner]

Just updated to alpha.4 and now I am getting this Error: 'createPopper' is not exported by node_modules/@popperjs/core/lib/index.js

[atomiks]

?? it's there

It works for me in a create-react-app too. Make sure the version is correct.

[donnysim]

So is the destroy method not needed anymore? how do we unbind it?

[atomiks]

The destroy method is there still

[donnysim]

Then it's somewhere hidden?

"popper.js": "2.0.0-next.4",

this.popper = new PopperJs(referenceEl, popupEl);
console.log(this.popper.destroy);

undefined

[atomiks]

next.4 is old - you need alpha.4 (@alpha)

The next README has all the info you need: https://github.com/popperjs/popper.js/tree/next (along with the docs folder for details of the API - the website will be published soon)

[donnysim]

Ohhh, totally my bad, did not notice it's alpha, not next.

[donnysim]

Everything works really well so far, tried in nested tables, fixed elements, modals etc.. (though all popper elements are attached to body). Only thing I noticed is that when attached to a fixed navbar and scrolling the page, the popup keeps kind of jittering on firefox (chrome seems fine).

(it's a bit worse than visible in the gif, sometimes jumping on top of the menu or way below), but it also happens with v1. In content popups seem fine.

On Edge, nothing seems to stay in place when scrolled 🤔


everything works fine with v1 on Edge, though with it moving to webkit, I'm not sure how relevant this is at the moment.

[atomiks]

Does Edge work with alpha.3? There was a change in alpha.4 to how scrolling containers are determined.

For fixed elements you should use strategy: 'fixed' in the options

[donnysim]

No, doesn't seem like it, still incorrect placement, alpha.2 doesn't work at all.

[atomiks]

What is instance.state.scrollParents in Edge? If it contains empty arrays then there's a problem with this particular function - maybe try debugging there

[donnysim]

They both (popper and reference) contain 1 entry - Window (on chrome and on edge).

/lib/dom-utils/getCompositeRect.js

  return {
    x: rect.left + scrollSum.scrollLeft - offsets.x,
    y: rect.top + scrollSum.scrollTop - offsets.y,
    width: rect.width,
    height: rect.height
  };

on Edge the offsets.x and offsets.y is what breaks it seems, after removing them everything starts to fall into places (but breaks chrome and ff).

[donnysim]

Heres what they return when scrolled 100px
Chrome:

rectLeft: 1634.5
rectTop: 82
scrollSumScrollLeft: 0
scrollSumScrollTop: 0
offsetX: 0
offsetY: -100
x: 1634.5
y: 182
width: 16
height: 20

Edge:

rectLeft: 1635.0799560546875
rectTop: 82.6500015258789
scrollSumScrollLeft: 0
scrollSumScrollTop: 100
offsetX: 0
offsetY: -100
x: 1635.0799560546875
y: 282.6500015258789
width: 16
height: 19.899993896484375

as in:

  console.log({
    rectLeft: rect.left,
    scrollSumScrollLeft: scrollSum.scrollLeft,
    offsetX: offsets.x,
    x: rect.left + scrollSum.scrollLeft - offsets.x,
    rectTop: rect.top,
    scrollSumScrollTop: scrollSum.scrollTop,
    offsetY: offsets.y,
    y: rect.top + scrollSum.scrollTop - offsets.y,
    width: rect.width,
    height: rect.height
  });

[atomiks]

So getScrollSum is different for some reason.. check there?

[donnysim]

If only Edge dev tools weren't so atrocious to debug in 😂

[donnysim]

So document.body.scrollTop is deprecated and on chrome and firefox, probably safari too, it always returns 0 despite it being scrolled, you need to use document.documentElement.scrollTop. So in this case, Edge actually returns the correct values, except the way you get it is deprecated. So I'm assuming the placement is broken in all browsers 😓

[atomiks]

So it only happens when body has position: relative;? Since it's only the offsetParent whose scrollSum is calculated.

[Joelkang]

Not sure if this is related or not, but alpha.3 is broken for me since we load Popper in an iframe, but our tooltips are in the window.top.

In particular, I think https://github.com/popperjs/popper.js/blob/48334d01dfe496333a2dae5cced89dc1c38c3840/src/dom-utils/getWindow.js#L6 is not correct since if node is body of the top window, document.body.hasOwnProperty('ownerDocument') is false, causing it to fall back to the window of the iframe, and not the host window.

It's not 100% clear to me why we need to check for hasOwnProperty in this case.

[FezVrasta]

I use the presence of ownerDocument as a way to check if the element is a window element.

[Joelkang]

@FezVrasta is it possible to do node instanceof Window for that purpose? document.documentElement also does not have its own property ownerDocument

[FezVrasta]

The problem is that instanceof doesn't work across different window instances.

[atomiks]

What's wrong with the v1 function?

function getWindow(element) {
  const ownerDocument = element.ownerDocument;
  return ownerDocument ? ownerDocument.defaultView : window;
}

I'm guessing if element is a window, we want to just return itself?

Not sure how to workaround any here - will this work?:

// @flow

export default function getWindow(node: Node): any {
  if ({}.toString.call(node) !== '[object Window]') {
    const ownerDocument = node.ownerDocument;
    return ownerDocument ? ownerDocument.defaultView : window;
  }

  return node;
}

[FezVrasta]

In Flow “window” is typed as “any” so I think you could return “Window” there

[FezVrasta]

Hey everybody! I just finished to switch popper.js.org to the v2 website. I also published the first release candidate (@popperjs/core@2.0.0-rc.1).

Popper 2 is now on the master branch, and v1 is on the v1.x branch.

[FezVrasta]

I'm going to close this issue, feel free to post here if you want to talk about the release. For anything else, please open a new issue!