Popper-core: 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
  
  
  
  • [x] add a built step (Rollup + Babel)
  
  
  • [x] add a CI setup
  
  
  • [x] add unit tests (Jest)
  
  
  • [x] add functional/integration cross-browser tests (Karma?)
  
  
• Core
  
  
  
  • [x] rewrite the positioning engine to work on all the supported cases (and more?)
  
  
  • [x] rewrite the modifiers API
  
  
• Modifiers
  
  
  
  • [x] computeStyles
  
  
  • [x] applyStyles
  
  
  • [x] arrow
  
  
  • [x] preventOverflow
  
  
  • [x] flip
  
  
  • [x] offset/shift (merge them together)
  
  
  • [ ] other modifiers will be added later, if anyone wants them please send a PR to implement them
  
  
• Miscellaneous
  
  
  
  • [x] 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.

https://github.com/FezVrasta/popper.js/pull/793

Any help is very appreciated.

[atomiks]

What would be amazingly helpful is a contributing guide (how to run/set it up) and maybe a basic walkthrough of how the next codebase works? 💙

[FezVrasta]

Isn't this enough? If you think something is missing please let me know or feel free to send a PR to improve it.

https://github.com/FezVrasta/popper.js/tree/next#hacking-the-library

[FezVrasta]

I just published a new version at @popperjs/[email protected].

This version includes:

• flip modifier
• placement variations (start/end)

[oliviertassinari]

Awesome, I will try it.

[FezVrasta]

I published @popperjs/[email protected], it includes:

• preventOverflow modifier
• several fixes
• docs for the other modifiers

It would be great if you guys tried it and let me know how it feels.

There is a small quirk that requires the popper element to have position: absolute; top: 0; left: 0; applied _before_ Popper.js runs, so keep that in mind while testing it.

We are at 3.36 kB minified and gzipped, I'm sure we can squeeze some bytes by optimizing the build, but I'm quite happy with it as is even now.

[oliviertassinari]

There is a small quirk that requires the popper element to have

We have noticed the same with v1. Using position fixed helped to fix a scroll focus issue: https://github.com/mui-org/material-ui/blob/b9a983f431d168d6b34726976199ce676455eed5/packages/material-ui/src/Popper/Popper.js#L208-L212

[oliviertassinari]

A couple of notes:

• Crash ReferenceError: __DEV__ is not defined
• Crash Object doesn't support property or method 'find' in IE 11
• Crash .destroy() is not a function
• Crash if no modifiers is provided
• Warn PopperJS: "detectOverflow" can accept as boundaryElement only a parent node of the provided popper. when an empty array modifiers is provided

I couldn't go further without the destroy method.

[FezVrasta]

I may need to implement the destroy method 😅

[atomiks]

Crash ReferenceError: __DEV__ is not defined

I guess this is due to Babel being used, not Rollup to generate the "module" files. So we might need the expression plugin after all

[FezVrasta]

@popperjs/[email protected] includes the destroy method, and the fix for the __DEV__ error.

[FezVrasta]

I'm unable to reproduce the point 5 with next.7, may you verify it's still an issue? If so, I may need a repro case.

About point 2, I'm considering asking the consumers to polyfill the unsupported methods, rather than ponyfilling it in the library. It looks like a clean approach that keeps focus on the modern browsers, while allowing dated ones to still work. What's your thoughts?

[atomiks]

IIRC just a note, instanceof checks don't work for iframes. You're using that all over the codebase so might need to be refactored into something else.

[atomiks]

Bugs

1. Popper cut off in offset parent (fixed)

2. Weird flipping loop behavior (maybe fixed 😱)

This one requires a super specific scroll config in the test so it's hard to repro

https://i.imgur.com/NO9Uo4z.mp4

I'm not sure if the modifier is necessary

  window.instance = new Popper(reference, popper, {
    placement: 'top',
    modifiers: [
      {
        name: 'preventOverflow',
        options: {
          padding: 10,
        },
      },
    ],
  });

3. Popper flips and gets cut off (fixed)

http://localhost:5000/scrolling/nested

Resize window to the right so it flips

  window.instance = new Popper(reference, popper, {
    placement: 'right',
  });

4. Popper cut off on load (fixed)

http://localhost:5000/scrolling/nested

Make sure your console window (if docked at bottom) isn't too tall

  window.instance = new Popper(reference, popper, {
    placement: 'left',
  });

[FezVrasta]

There was a bug with the flip modifier, I pushed a fix for it.

There was also a bug in the getClippingParent function used by detectOverflow (bug 1) and now it should be fixed too.

@atomiks may you try again?

[atomiks]

1. fixed
2. not sure too hard to repro atm
3. not fixed
4. not fixed

[atomiks]

5. Positioning logic is not correct when reference element has margins (fixed)

[atomiks]

Should we add a ResizeObserver (_optional_) to watch for size changes of the popper, along with the scroll and resize listeners? Currently you need a loop if the popper's size changes in an animation or something.

I guess the user can just add it themselves.

const resizeObserver = new ResizeObserver(popperInstance.update);
resizeObserver.observe(popperElement);

[FezVrasta]

ResizeObserver would be neat, it shouldn't add too much overhead.

[FezVrasta]

I just finished to setup some functional tests on the new code base, they run with Puppeteer and do a screenshot comparison to make sure nothing breaks.

I will try to add support for Firefox also, but I won't be able to support Internet Explorer or Edge since they don't provide Puppeteer APIs.

Once Edge migrates to Chromium I guess we'll be able to add tests for it.

[FezVrasta]

Added Firefox tests.

If anyone would like to contribute but don't have enough experience to contribute to the library code, please consider writing new functional tests to cover as many cases as possible. It will be infinitely valuable! 😍

Tests docs are available here https://github.com/popperjs/popper.js/tree/next#functional-tests

[atomiks]

6. getElementClientRect doesn't take into account transforms (fixed)

offset properties are being used, but these don't take into account transforms. However, it looks like they were chosen because they greatly simplify logic regarding offsetParent and scrolling containers, right? 😕

How do we get it to handle transforms? I'm guessing there is no other option but to use getBoundingClientRect and handle all that logic? Also the offset properties don't exist on SVGElements.

[atomiks]

~### 7. Change flip Options#behavior to Options#fallbackPlacements~

~The word "behavior" isn't self-descriptive, fallbackPlacements makes much more sense. Thoughts?~

[FezVrasta]

I like it! I'm sorry but my english is pretty limited so I often can't find good names for the lib functionalities 😩

[atomiks]

I don't think fallbackPlacements made sense in v1, because you had to specify the original placement as well inside the array.

In v2, you can do something like this, right?

new Popper(ref, pop, {
  placement: 'right',
  modifiers: [
    {
      name: 'flip',
      options: {
        // no need to put "right" first
        fallbackPlacements: ['bottom'],
      },
    },
  ],
});

[FezVrasta]

Yes it should be correct

[atomiks]

Also I think we discussed, but I think the flipping default should be smarter than fallbackPlacements: [oppositePlacement]

placement: right
fallbackPlacements: [left, top, bottom]

e.g. most common use case is a left/right placement that won't fit on small screens like mobile, so it needs to use vertical space. The alternative is overlapping or overflowing, both of which aren't good options.

[atomiks]

8. flip boundary should be based on the viewport not document

The docs read:

The flip modifier can change the placement of a popper when the detectOverflow modifier reported it's overflowing in the direction of the current placement.

Currently, while using the flip test, the popper doesn't flip when it goes beyond the bottom of the viewport (but not the bottom of the whole page). This is unexpected.

While both should use clippingParent first (I think?), preventOverflow should fallback to the document rect by default, while flip should fallback to the viewport window.

Maybe detectOverflow needs to also provide information about viewport overflow so that flip can use it.

[kumarharsh]

Just dropping by to say I'm really loving the progress here. Keep it up :)

[FezVrasta]

I just added a bundle visualizer to the Rollup config. Hopefully it will help us to understand where can we focus our efforts when we need to simplify parts of the library.

The popper-base is still very light (1.96 kB gzip), popper-lite is at 2.85 kB gzip, which is great. But the full library is at 4.25 kB gzip, we may want to keep an eye on it.

(the sizes in the chart refer to the minified, not gzipped code)

edit: reduced computeStyles modifier from 2.14 kB to 1.88 kB

[atomiks]

31.88 KB for popper.min.js seems wrong, is it using unminified sourcemap sizes?

It looks really cool tho 🚀

[FezVrasta]

You are right, it's unminified. I need to fix that 🧐

[FezVrasta]

Here's the minified visualization.

[atomiks]

I bet index.js could be around 1.5 kB if we used a closure instead of a class:

new Popper => createPopper. Is there a reason for keeping the class syntax?

[FezVrasta]

no, actually I did a little poll on Twitter and people preferred the non-class approach, so we may change it.

I'll send a PR in a few minutes.

[FezVrasta]

https://github.com/popperjs/popper.js/pull/867

not a big size reduction (12.03 > 11.76 - ~4.25 gzip > 4.14 gzip), but at least now the default modifiers API is clearer.

import { popperGenerator } from '@popperjs/core`;

const createCustomPopper = popperGenerator(myDefaultModifiers);

[atomiks]

I wonder if we could release Popper v2 alpha soon or around Christmas time? 🚀 🎄
Would be a nice little gift for everyone 🎁

There are next versions already but too unstable and undocumented to use. I feel like we've covered most features already that were in v1 now. There are just a few behavior tweaks to do in the next few weeks and it should be ready to make an article about to get people to try out the new API

[FezVrasta]

I was planning to go from next to beta, any reason to use alpha as well?

[atomiks]

I don't really know what the difference between them is regarding breaking changes 🤷‍♂

beta makes more sense with an article, but can you continue to make breaking changes during them?

[FezVrasta]

The one below is the pseudo-thermal view of the gzipped popper.min.js file, red parts are the one that can't be compressed, blue the ones that get compressed more.

Posting just because it's kinda cool 😄 It looks like the library is already pretty well compressed.

This lead to this commit https://github.com/popperjs/popper.js/commit/75a9917b6ad4d4b0c4d9d15fe41726e372249698 that optimized the compression by using lowercase node names.

Click to show the image

[atomiks]

For custom modifiers, how do we allow adding to compute/applyStyles?

e.g. if hide won't have built-in support, they need to add an data-popper-out-of-bounds attribute in a separate write modifier?

[FezVrasta]

Maybe we could move the styles and attributes out of the modifier namespace, leave them in state.*

[FezVrasta]

I just published 2.0.0-next.14 with support for TypeScript, if anyone could try it out and report if there are any problems it would be great.

edit: 2.0.0-next.15 gets rid of a type dependency.

[NemoDima]

@FezVrasta please, excuse me for the dummy question? I'm a bit nooby with GitHub and current project workflow.

835 was it solved in 2.0.0-next.15 or should I wait for the new corresponding status of my bugreport?

Bets wishes, Dmytro

[FezVrasta]

The fix is available in 2.0.0-next.15, but we are not there yet for a stable release. You can start playing with it and see if everything works as you expect though.

[FezVrasta]

Just a heads up, @popperjs/[email protected] is now available on npm, it should be quite stable.

Report any feedback here if you have a chance to try it.

[atomiks]

As far as I can tell, these features are still yet to be implemented that are in v1:

• auto placement
• flipVariations / flipVariationsByContent (that a PR recently introduced?)

What else?

[FezVrasta]

There's the clockwise and counter-clockwise flip behavior, but I don't think it's something I want to see in v2, it can be easily achieved in user-space, and hopefully, once auto is ready, we'll be able to set auto as fallback placement, to cover the majority of the use cases.

[FezVrasta]

Is there some Windows user willing to help us figure out why Popper 2 is not working on Edge and Internet Explorer 11?

On both of the browsers no errors are displayed (after the required polyfills are included #792)

[pat-son]

I've only tested Edge so far, but here's what I've found:

The culprit seems to be line 5 in getBoundingClientRect.js. On Edge, JSON.stringify(element.getBoundingClientRect()) results in an empty object, so the resultant rect.x and rect.y are both undefined. Changing line 5 to simply const rect = element.getBoundingClientRect(); fixes it in Edge (at least, the basic example I was looking at). What is the reason for stringifying then parsing element.getBoundingClientRect(), anyway?

I can take a closer look (specifically, at IE11) this weekend if that would be helpful.

(Oops, I meant to comment this on the specific issue)

[atomiks]

Couple questions about the library behavior:

Should preventOverflow#rootBoundary be viewport like flip's?

If our goal is to keep the popper in view as best as possible, this makes sense I think. The reason is, the viewport is as valid as a clipping element as a scrolling container for example. It's more intrusive like this, and doesn't match v1's behavior though

If no placements fit (flip), should we use the placement that's most visible instead of the original placement? Or make it configurable?

There are some cases where the original placement makes the popper fully clipped but it's still used in the case where no placements fully fit, even if one of the flipped placements almost completely fits, like 95% for example

[FezVrasta]

Sorry for the delay, I suppose viewport is a good default, I agree.

I think using the best fitting placement would be nice to have, but it's not something that is required for the first stable v2

[pers-web]

how to attach index.html to poper i can't find where to connect

[rwwagner90]

I just tried to use v2, and got an error with this https://github.com/popperjs/popper.js/blob/next/package.json#L50

Error: Failed to load config "./.config/eslint.config" to extend from.
Referenced from: /Users/rwwagner90/shipshape/shepherd/node_modules/@popperjs/core/package.json

Also, even if I delete that config, which makes things run, I do not seem to be able to use Popper as expected. The div is created and the content is in it, but nothing shows up. You can see my attempt here https://github.com/shipshapecode/shepherd/pull/752

[atomiks]

@rwwagner90 I think you need to prevent eslint from running on node_modules

As for the issue, we can only really help if you have a reproducible CodePen/Sandbox

[rwwagner90]

@atomiks it's reproducible in the PR I linked https://github.com/shipshapecode/shepherd/pull/752

If you pull down the branch and run yarn watch you can see the Popper div is created, but it's totally invisible for some reason.

Also, I am using rollup-plugin-eslint, and there does not seem to be a configuration that will correctly disable the lint issue. I have not encountered this problem in the past.

[atomiks]

When I pull down the branch, install deps with yarn and run yarn watch

welcome.js:12 Uncaught ReferenceError: Shepherd is not defined
    at setupShepherd (welcome.js:12)
    at HTMLDocument.init (welcome.js:5)

It can be hard to understand the problem when it's masked in the context of a 3rd party lib. If you can manage to remove parts of your library gradually to get to the core problem it may help

[rwwagner90]

@atomiks you're probably getting that because of the lint issue, which seems to be impossible to disable. rollup-plugin-eslint has never had this issue before and should be excluding node_modules by default.

[atomiks]

Yeah I fixed that. Anyway it's because you have opacity: 0 on it which isn't being changed. I am guessing you were using callbacks that no longer get called (they don't exist anymore) like onCreate

~Explicitly writing: exclude: 'node_modules/**' seems to work. Maybe it's a bug with the plugin since that should be implicitly default.~ (nevermind, the file wasn't updating after I removed the whole plugin lol 🤦‍♂ )

cc @TrySound any idea why this is happening

[FezVrasta]

I think we may need to ship the .config folder in the npm package

[TrySound]

@FezVrasta No need to publish it on npm. @atomiks Did you try to use it without include option?

[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!

[rwwagner90]

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/[email protected]).

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!