Update to Popper.js to 2.0.0

Added link to release post, according it the v2 is full rewrite to be future-proof and removal of ie10 and other legacy

At this point, let's at least asses how difficult it would be for us to upgrade to Popper.js 2.0.0.

The ultimate question is how long is popper 1 supported? (A library that we just added really recently a few months ago, right?).

We discussed abut that on https://github.com/popperjs/popper-core/issues/823. They are providing support for the current and the previous major version. This means that v1 would receive security coverage until v3 has been released. We don't know exactly when that the v3 is planned to be released. I sent @FezVrasta email through the Drupal.org contact form to try to find out.

My vote is definitely to upgrade to v2 in 9.0. Adoption in Drupal contrib is going to be minimal, since only Drupal 8.8 shipped with it ~1.5 month ago.

Asked the author/maintainer for upgrade path docs:

Congratulations! :)

For those projects that depend on Popper 1, it'd be super valuable to have explicit "Upgrading from Popper 1 to Popper 2" docs. AFAICT that doesn't exist yet (I looked at https://github.com/popperjs/popper-core/releases/tag/v2.0.0, https://popper.js.org/docs/v2/tutorial/ and in this blog post). It could make an *enormous* difference in terms of Popper 2 adoption: without clear docs that cover both the most commonly encountered backwards compatibility breaks that need to be overcome and warn about the harder edge cases, many projects will probably stick with Popper 1. This would increase the maintenance burden too.

In any case: congratulations, awesome work, and *thank you* for this very valuable contribution! We're looking forward to hopefully updating Drupal from Popper 1 to 2 soon :)

— https://dev.to/wimleers/comment/kk77

The author responded that he's happy with v2 and he believes that there will be no need for introducing breaking changes at least for a while.

Started working on updating the library. The behavior is slightly different so we have to do some additional work to smoothen that.

I had to turn on the GPU acceleration to make this work. When that was turned off, the positions were rendering completely off. I'm wondering if this is a regression in the upstream package or something else.

It also seems like that they don't provide pre-packaged version of the library so I had to build it by hand: https://github.com/popperjs/popper-core/issues/967.

Hey there, thanks for the nice words about Popper!

@atomiks just added a migration guide to our documentation, you can find it here:
https://popper.js.org/docs/v2/migration-guide/

I think it will be useful to help you upgrade to v2.

The GPU acceleration issue is not expected, if you are able to reproduce it in our sandbox would you please file a bug in our repository?

https://codesandbox.io/s/popper-2-playground-hgjuz
https://github.com/popperjs/popper-core/issues/new/choose

@lauriii looking at your patch, I think you should be able to get rid of the custom `preventOverflow` options, the defaults should work way better than they used to.

Also, I think your custom modifier should live in the "write" phase.

Isn't there a place where to review PRs such as GitHub/BitBucket? I'd love to help with this.

@FezVrasta Thank you for your feedback so far, and thank you for the generous offer to help us finish the upgrade!

You can install Drupal with the following commands:

git clone --branch 9.0.x git@git.drupal.org:project/drupal.git
cd drupal
composer install
php ./core/scripts/drupal quick-start demo_umami

This should work assuming all of the system requirements are met: https://www.drupal.org/docs/8/system-requirements.

You can then apply the patch by running:

curl https://www.drupal.org/files/issues/2020-01-24/3108402-9.patch | git apply --index -

If you need any help with this, feel free to reach out to me on Drupal Slack.

Thanks, I'm sorry but I think I didn't explain myself, I wanted to offer help by reviewing the patch/PR, unfortunately I don't have bandwidth to work on the code :-(

Don't you have any web interface to leave comments on the patch files? So that I can drop notes where needed? Is it just that plain text file?

@FezVrasta You can install dreditor browser plugin which will allow you to give line by line review in a Github type of fashion: https://chrome.google.com/webstore/detail/dreditor/dhdpoembhlojpmehepead....

Tagging as a requirement for Drupal 9.0-beta1.

I removed a lot of the custom refinements since they are not needed anymore. The functionality is not much closer to what it was on Popper.js 1. However, for some reason the toolbar is positioned to the bottom some times which could use some more research. This can be seen on this gif:

May you try to disable the “adaptive” option as explained here?

https://popper.js.org/docs/v2/migration-guide/#13-transitions

Thank you @FezVrasta! This solved the problem described in #18. I also disabled the popper tethering which takes this very close to the experience before upgrading.

I suspect the failures are all due to Popper animations not yet having completed when the failing assertion is executed. We'll probably need to wait for a particular event prior to executing the assertion?

This is how it looks like in the test. I tried to fiddle with the page manually and it's not a race condition type of issue. For some reason, on that page the toolbar is positioned incorrectly:

I noticed another issue as well; the altAxis setting pushes the toolbar down on a situation where it would go outside of the boundary. However, in this situation, it should flip below the element rather than stay on top of it.

We will also have to figure out how to polyfill Array.prototype.find, Promise and Object.assign for IE 11.

This adds the required IE 11 polyfills.

If you encounter positioning problems and you think they are a bug in the library, please open an issue in the Popper repository, we definitely want to fix them.

#24.1 Tests are failing because the disabling of CSS animations in FunctionalJavascript tests (probably unnecessarily) includes disabling transform: rules. Took me a moment to recall I filed this a few months ago: #3082602: Remove transform rule from css_disable_transitions_test

#24.2 Was addressed by adding a bit more config to the flip: modifier. I'm not 100% sure altAxis even needs to be set to true, but assuming it's there for a good reason I'm just not aware of - this fixes it.

Changes in #28 look great! Good job tracking down those problems 👏

#28.2 The altAxis is needed for untethering the toolbar from the reference element so that the toolbar can be displayed after scrolling away from the reference element. I opened an upstream issue for this to see if there's a bug in the library, or if the documentation needs some extra clarity: https://github.com/popperjs/popper-core/issues/989.

Besides #28.2, we're ready for review ✨

It seems like #28.2 is solved by the Github issue. They explained that in our use case the altAxis is needed because when top/bottom placement is used, the main axis is x. To enable tethering on y axis, we need to enable prevent overflow modifier on the alt axis as well.

Some questions:

1. Should we have a change record linking to the Migration Guide - https://popper.js.org/docs/v2/migration-guide/? for any modules that might have been using Popper v1 in Drupal 8? I'm not sure if it's possible to do a BC layer for popper considering how much has changed with its data model.
2. Do we need more manual testing in supported browsers before this is RTBC?

---

I reviewed the patch in #28, and I didn't find any issues with it. The code for Drupal 9 is more straightforward now that we don't need the logic to adjust the position (from #18).

1. +++ b/core/core.libraries.yml
   @@ -95,6 +95,11 @@ drupal.autocomplete:
   +drupal.array.find:
   
   @@ -227,6 +232,11 @@ drupal.message:
   +drupal.object.assign:
   
   @@ -602,13 +612,26 @@ picturefill:
   +es6-promise:
   
   +++ b/core/misc/polyfills/array.find.es6.js
   @@ -0,0 +1,56 @@
   + * This is needed for Internet Explorer 11 and Opera Mini.
   
   +++ b/core/misc/polyfills/object.assign.es6.js
   @@ -0,0 +1,37 @@
   + * This is needed for Internet Explorer 11 and Opera Mini.
   

   Having these in core is a big win for me.

   Do you think that we will still support Internet Explorer in Drupal 10? Or if proxy browsers will support these features in Drupal 10 - #2119299: Make sure Drupal 10 works with mobile proxy browsers?

   Still time to do that some time some time before 9.[n - 1].x in a follow-up issue.

2. +++ b/core/core.libraries.yml
   @@ -95,6 +95,11 @@ drupal.autocomplete:
   +    misc/polyfills/array.find.js: { weight: -20 }
   
   @@ -227,6 +232,11 @@ drupal.message:
   +    misc/polyfills/object.assign.js: { weight: -20 }
   

   I confirmed that there isn't a need to add license here because the polyfill code off of MDN is in the public domain.

I started writing a change record: https://www.drupal.org/node/3112670.

I'm wondering if we should be concerned about multiple major version compatibility. Given the scope of changes in Popper.js, it doesn't seem possible for the same code to support both, Popper.js 2.0.0 and 1.16.0. Should we consider backporting this to 8.9.x for that?

Do we need more manual testing in supported browsers before this is RTBC?

I think this could be useful.

@lauriii re backport, #3074267: Replace jQuery UI position() with PopperJS added popper.js to replace jquery.position, but to keep backwards compatibility, jquery.position was deprecated and kept. Is it possible (useful?) to add popper.js 2 to Drupal 8 while keeping popper.js 1 included/supported as well? Also unless this gets an exception it even means that popper.js 1 would need to stay as not deprecated given that we wanted to deprecate everything until Drupal 8.8.

Thank you for the change record, @laurii. I added a testing matrix to the issue summary.

Is it possible (useful?) to add popper.js 2 to Drupal 8 while keeping popper.js 1 included/supported as well?

I think including both versions would be problematic on a user's site. It would probably break or one version would override the window global. Either Popper.createPopper wouldn't exist, which would break core's use of v2, or the Popper constructor wouldn't exist, which would break the functionality that added v1.

We do have libraries-override, but we would really need a libraries-conflict, or some way to block loading a library if something that conflicts wants to be loaded. That might block some functionality from working at all. If I want to enable quickedit, then I may not be able to do so if another module is enabled that is using a conflicting library. Something. like #92233: Modules in conflict - conflicts[], breaks[], brokenby[] field in modules .info file but for libraries as well.

The broken site on Drupal 9 update scenario:

- Johanna the dog baker groomer has gone through steps 1-3 to checked Upgrade Status, updated the contributed modules on her site, updated custom module and/or custom theme including JavaScript deprecated properties and functions.
- Then, on step 4, Johanna updates to Drupal 9 and finds that the front-end of the site has broken features.
- Most likely this would delay Johanna from immediately updating Drupal 9 until there's a Drupal 9 compatible module/theme available (the Drupal upgrade waiting game problem). This would probably mean a new major version of the module/theme for those that depend on Popper.

1. If there was a policy exception to allow v2, then. I think there's probably a better expectation for a user to need to make changes in a major version (9.0.0) than in a minor version (9.1.0 or 8.9.0).
2. A bigger maintenance burden would be to fork and create a UMD version that wraps Popper as Popper.Popper including updates any time v2 updates. I think more effort would need to go into that than potential fixes.
3. Come up with a libraries conflict and php code that prevents extensions from being enabled that have conflicting libraries. This would require PHP code and testing.

I opened: #3112800: Library dependencies are not taking library versions into account.

I guess a module willing to support both, 8.9.x and 9.0.x include code that integrates to both, Popper 1.x and 2.x. They could determine based on if Popper.create exists whether they should use Popper 1.x or 2.x compatible code.

I guess a module willing to support both, 8.9.x and 9.0.x include code that integrates to both, Popper 1.x and 2.x. They could determine based on if Popper.create exists whether they should use Popper 1.x or 2.x compatible code.

.

I think this could work. I'd be most comfortable with this if there were something in the codebase making this apparent to reduce the chances of a developer cursing for hours/days before happening upon the CR. Maybe a comment with a link to the CR in core.libraries.yml?

Maybe we could add the error that is thrown when Popper.js 2 is initialized with Popper.js 1 code to the change record. This would probably make the change record visible in Google and could help people find the migration instructions. Comment might work as well, but at least to me, that wouldn't necessarily be the first thing I would be checking.

Bumping to critical since this blocks the beta.

Re #37 +1 on adding the PopperJS error's to the CR.

Sounds like there isn't opposition to it, but my line of thinking regarding the comment in core.libraries.yml is that I figured most developers that would be using this library wouldn't have the library name memorized and would see the comment when they check core.libraries.yml to confirm the name (if only to be reminded it's core/popperjs and not core/popper)

1. 👍 The manual testing done by @lauriii means I feel confident I don't need to repeat that.
2. 👍 The test coverage flag changes are sensible.
3. 🐛
   

   +++ b/core/core.libraries.yml
   @@ -95,6 +95,11 @@ drupal.autocomplete:
   +drupal.array.find:
   +  version: VERSION
   +  js:
   +    misc/polyfills/array.find.js: { weight: -20 }
   +
    drupal.batch:
      version: VERSION
      js:
   @@ -227,6 +232,11 @@ drupal.message:
   
   @@ -227,6 +232,11 @@ drupal.message:
        - core/drupal
        - core/drupal.announce
    
   +drupal.object.assign:
   +  version: VERSION
   +  js:
   +    misc/polyfills/object.assign.js: { weight: -20 }
   
   @@ -602,13 +612,26 @@ picturefill:
   +es6-promise:
   +  version: "4.2.8"
   +  license:
   +    name: MIT
   +    url: https://github.com/stefanpenner/es6-promise/blob/v4.2.8/LICENSE
   +    gpl-compatible: true
   +  js:
   +    assets/vendor/es6-promise/es6-promise.auto.min.js: { weight: -20, minified: true }
   

   These additions are not mentioned in the issue summary nor in the change record. I think the polyfills merit a separate change record, perhaps even one per polyfill to improve discoverability and linkability.

   Where license information is missing, we need to add it.

#40: Should we link to https://developer.mozilla.org/en-US/docs/MDN/About#Code_samples_and_snip... and mark the code as CC0 based on that? However, this is not the actual license since the license they are using doesn't require a license text.

Updated CR with V1/V2 information, created new CRs for the polyfills, and added license information per the suggestion in #41. I did some research to see if there were any issues due to it not being the actual license and couldn't find anything conclusive. Easy enough to change if it's not what we ultimately want.

Opened support request in the LWG issue queue: #3114527: How to document license of code that has been copied from a code snippet to ask how we should document the CC0 license code that has been copied.

On #3114527-4: How to document license of code that has been copied from a code snippet, @gisle recommended documenting the license in @file docblock of the file. I implemented that on this patch. I'm also adding issue credit for him on this issue.

Almost ready to RTBC

1. In the time since this issue was created there have been three updates - Popper is now on 2.0.6 (nice to have a library that is so actively maintained!)
2. I noticed the es6-promises library is not in alphabetical order in core.libraries.yml if you go by the es6. However, It is in order if read by post hyphen so I'm wondering if this is due to a standard I'm just not aware of. My preference is to see it after the "d" libraries but if that contradicts an active practice that's no problem.
3. I did the majority of the writing on the 4 CRs and releae notes so while I can RTBC the patch, I'd prefer if someone came in and double-checked those and provide edits where needed so they're in good shape for any committer that looks at this.

Thank you for the review @bnjmnm 👍 I made few minor modifications to the draft change records after which they look ready to me.

Confirmed the file is identical:

 if [ $(md5 -q ~/source/popper.js/dist/umd/popper.min.js) == $(md5 -q ~/Sites/d9/core/assets/vendor/popperjs/popper.min.js) ]; then echo "same"; else echo "different"; fi
same

Code was re-reviewed (was reviewed extensively when investigating the issues in #25

Also reviewed the approach of having the V1/V2 errors being present in the CR so developers can easily track down a solution regarding how to make modules/themes that are compatible with both versions.

Setting to RTBC because the test failure in #47 is unrelated.

+1 RTBC.

The Popper.js change record looks good to me. The polyfill ones are simple, but I don't think it needs to be explained that much. Unless someone is not familiar with front-end web development and doesn't know that they might need to include these polyfills for other things.

We probably don't need to prematurely deprecate the polyfills, but most likely they should be removed in Drupal 10 which hopefully will only support browsers that have this functionality. Should we add a follow-up to consider removing these or is there a follow-up browser support issue for Drupal 10?

Should we create issues in the projects we know are going to be affected - ie those listed on http://grep.xnddx.ru/search?text=Popper&filename=&page=

I opened issues in all projects that have a Drupal 8 release with references to Popper.js. ✌️

A lot of the affected modules and themes were related to Bootstrap which depends on Popper.js 1. They are working on upgrading to Popper.js 2 here https://github.com/twbs/bootstrap/issues/29842

Committed 479b652 and pushed to 9.0.x. Thanks!

The changes to our actual JS look good and are all to do with the differences of the v2 API.

I've reviewed where the polyfills have come from and concur that this seems the simplest way to support ie11.

I guess we'll have to watch the changed JS tests for any random fails that come from enabling animations but hopefully that won't happen.

I tested the upgraded interactions and there was something that was awkward for me - it's possible that the test editing controls get stuck over the text which makes editing very difficult - but the same problems happen with v1 so it is not introduced here. Here's a link to a gif of what happens.

diff --git a/core/.eslintignore b/core/.eslintignore
index 21c8a024f7..52972ceb70 100644
--- a/core/.eslintignore
+++ b/core/.eslintignore
@@ -5,3 +5,5 @@ node_modules/**/*
 !*.es6.js
 modules/locale/tests/locale_test.es6.js
 !tests/Drupal/Nightwatch/**/*.js
+misc/polyfills/array.find.es6.js
+misc/polyfills/object.assign.es6.js

I added the two polyfills to the list of things we don't test with our linting rules because it's not our code and they fail badly. If we want to make them pass then I think we can address that in a follow-up.