Follow-up to #2809427: update from jQuery UI 1.11.4 to 1.12.1 forgot to list some new JS files in *.libraries.yml

Here is the progress we did in #2906737 as a starting point.

Tested, work

I can confirm this works.
Tested using the case in #2906737: jQuery UI checkboxradio and selectmenu errors following 8.4 update to jQuery 1.12

#2925997: position-min.js issue since 8.4 upgrade has been committed to 8.6 so do we need anything else in this patch?

I have tested this, worked for jQuery UI checkboxradio and selectmenu errors following 8.4 update to jQuery 1.12 I have test on Drupal core upgraded to version 8.5.1

I have tested this patch on the latest version 8.6.x-dev of Drupal and it works.

The one thing that sticks out is that we have untestable bug. Is there anyway we can determine this and future proof this so if the dependencies change we'll know in the future?

I don't think this patch will solve all possible dependencies. As it stands right now, if I include just the base jquery.ui library and try to use ui.labels it will fail because ui.escape-selector won't be included.

One solution would be to add ui.escape-selector as a dependency of jquery.ui. But that just includes more bloat for 99.9% of all sites that don't need it.

The better solution is to break out all of the components that are currently listed in the jquery.ui "js" array into separate library entries (including escape-selector and anything else that is missing), and then rigorously go through each library and list its exact, first-order dependencies.

With any luck this could even lead to savings in code for pages that only require a few small portions of jquery.ui.

This won't be important in the "long term" since jQueryUI will be marked deprecated in 8.8 and removed from core in Drupal 9,
https://www.drupal.org/project/drupal/issues/3051352

but it would still be important to fix since some of these asset libraries are broken in core now.

One solution would be to add ui.escape-selector as a dependency of jquery.ui. But that just includes more bloat for 99.9% of all sites that don't need it.

The escape-selector-min.js file is only 368B, that doesn't seem like too much bloat. Adding it to jquery.ui core seems right to me since several of these widgets depend on it and it's a more general purpose utility.

The better solution is to break out all of the components that are currently listed in the jquery.ui "js" array into separate library entries

I agree with this approach for form-reset-mixin-min.js which is required on just (?) the form element widgets, I found it also is needed for Checkboxradio as well as Selectmenu.
It should be split out into a utility lib like jquery.ui.widget, jquery.ui.mouse or jquery.ui.position which was also suggested in #2.

The patch in #3 applied cleanly to 8.8.x for me, I rolled another with the escape-selector added to jquery.ui and form-min as a separate utility asset library.

I like the direction of #13, the remaining things I spotted should be fairly easy to address since I think all the missing dependencies are already defined as utility libs.

I combed through every jQuery UI file added in core.libraries.yml and listed the remaining missing dependencies. Theres also info about what problems may be caused by their absence (which in most cases explains why their absence went unnoticed for so long).

jquery.ui.button

Missing: controlgroup widget

Docs
Impact: Using the jQuery UI buttonset() method will result in an error. Per the docs jQuery UI docs

.buttonset() is bundled with .button(). Although they are separate widgets, they are combined into a single file. If you have .button() available, you also have .buttonset() available.

Missing: checkboxradio widget

Docs
Impact: using the button() method on checkbox or radio elements will result in an error.

jquery.ui.effects.puff

Docs

Missing: effect-scale

Impact: This effect does not work at all

jquery.ui.effects.scale

Docs

Missing: effect-size

Impact: This effect does not work at all

jquery.ui.menu:

Docs

Missing: position

Impact: Error on opening menu. This has likely not surfaced as jquery.ui.menu is not directly used anywhere in core. When it is used, it's as a dependency of a library that also declares core/jquery.ui.position as a dependency

jquery.ui.mouse

Docs

Missing: ie

Impact: occasionally-unpleasant mouse behavior a for those using IE8 and earlier.

jquery.ui.sortable

Docs

Missing: ie

Impact: Was not able to reproduce but it is a very edgy edge case. The ie method used in a conditional that also requires the item being sorted to be a direct child of <html>. If this condition is true it sets the sorting-container coordinates to 0,0.

jquery.ui.touch-punch

Docs

Missing: mouse

Impact: This shouldn't even initialize without mouse. Core only uses it on the CKEditor config page, which also loads mouse from another library.

I've been thinking about this a bit more and I'm becoming more fond of the suggestion from @Gribnif in #11

break out all of the components that are currently listed in the jquery.ui "js" array into separate library entries

Doing this would make core.libraries.yml double as a document that helps the jQuery UI deprecation process, making it very clear what components are still needed in core.

Determining the dependencies for each individual component shouldn't require too much additional effort, and I think it will save time in the long run. Having just gone through each component for #14, no single component has that many dependencies, so it should not hurt the readability of core.libraries.yml.

This is a much more comprehensive list, thanks @bnjmnm ! I was just focused on the few asset libraries I was working with.
After talking with you in slack I thought you were right and it makes sense to explicitly require each dependency and break all these different requirements out.

This also matches how jQuery UI's own bundle builder works, it automatically grabs the dependencies when you select a widget.

The core/jquery.ui explicitly loads the following:

• jquery
• data
• disable-selection
• focusable
• form
• ie
• keycode
• labels
• jquery-1-7
• plugin
• safe-active-element
• safe-blur
• scroll-parent
• tabbable
• unique-id
• version

I separated each of those files into their own libraries. All of the jQuery UI asset libraries require jquery and "version", as well as the common css files, so I put version into a jquery.ui.core library which everything else can depend on without getting the full package of jquery.ui

I searched the minified code for each of those js files to look for references to determine where they were required.

Then for the existing libraries I did the same search and checked against @bnjmnm's list.

Name followed by the list of libraries that depend on it:

I manually tested that these libraries still work for

• accordion
• checkboxradio
• controlgroup
• droppable
• progressbar
• selectable
• selectmenu
• slider
• spinner
• tooltip
• effects

However after discussing with @lauriii this morning- this many changes to core.libraries will certainly break BC,
I am posting the patch I made for discussion purposes but now I think we should probably take a more conservative approach to fix this issue rather than a big refactor.

I was hoping to find some jquery.ui files that we could separate out and mark deprecated but they have at least one dependent as seen above.

Going back to where #3 started, I took a different approach and added the dependencies missing without moving any files to new libraries. Added escape-selector per #11 to core/jquery.ui.
Went down @bnjmnm list again and added the files where needed, and compared to the list I made a couple comments back.

I think this new patch should maintain BC while fixing the issues with missing dependencies.

On the theoretical level, I strongly agree with @Gribnif's suggestion in #11 that the better solution would be to create individual asset libraries for each. But I worry about the practical implications, both in terms of potential subtle backwards compatibility breaks and how we are going to achieve sufficient confidence to make this change, especially when so little in Drupal core depends on this.

#14: wow, that is some epic combing! 🤯

As I dug in to the first example you list ("controlgroup widget is missing from jquery.ui.button"), that seems to contradict what the docs you linked say. Then I found https://api.jqueryui.com/button/, which kinda says what you're saying. What happened is that .button() used to work on input[type=radio] and input[type=checkbox]. That is now deprecated, and instead one must use the .checkboxradio(). Drupal 8.0.0 shipped with jQuery UI 1.11.4. #2809427: Update jQuery UI to 1.12 updated it to jQuery UI 1.12. Reading that issue again — where I was pretty actively involved — it's painful to see how little attention we paid to backwards compatibility. Fortunately, jQuery UI 1.12 retains backwards compatibility where possible: https://jqueryui.com/upgrade-guide/1.12/#api-redesigns. That is how calling .button() on input[type=radio] would still work … if we actually loaded the checkboxradio widget! #2809427-20: Update jQuery UI to 1.12 added the core/jquery.ui.checkboxradio asset library, but didn't add it as a dependency to core/jquery.ui.button to retain backwards compatibility.
Since it's been more than two years since #2809427, it's safe to say that anybody who needed that to work has already added the core/jquery.ui.checkboxradio asset library dependency manually. Same for core/jquery.ui.controlgroup. I don't think it makes sense anymore to add those as dependencies to core/jquery.ui.button at this time, since it's only for jQuery UI 1.11 backwards compatibility.

RE: broken effects: how did you discover this? EDIT: I suspect by inspecting the source, such as https://github.com/jquery/jquery-ui/blob/1.12.1/ui/effects/effect-scale....? :)

RE: missing ie dependency: how did you discover this? EDIT: same suspicion as above :)

#15: I agree that would enable Drupal core to have more granular deprecations. I am not concerned about readability or overhead.

#16: 👏 I agree that making Drupal 8's asset library structure match the structure exposed in jQuery UI's own bundle builder would be a great idea! But this would exponentially increase my backwards compatibility breaks concern at the start of this comment. And wow, the grepping of the jQuery UI minified code base — that is at least equally impressive digging! 🤯🤯 At the same time, that is super scary. It means we are trying to reconstruct jQuery UI's dependency tree. Surely they must have their own dependency data? For example, you say that accordion depends on data. But https://github.com/jquery/jquery-ui/blob/1.12.1/ui/widgets/accordion.js contains zero occurrences of .data. So … are you sure this is correct? menu indeed depends on data: I do find .data occurrences in https://github.com/jquery/jquery-ui/blob/1.12.1/ui/widgets/menu.js. But AFAICT jQuery UI uses AMD to declare dependencies, so we should be able to just convert those exact same dependencies to Drupal asset library dependencies. Wouldn't that be much less risky?
On the other hand, the very first example I looked at seems to have incomplete dependency information: https://github.com/jquery/jquery-ui/blob/1.12.1/ui/widgets/menu.js's dependencies (and its dependencies etc) do not load jQuery UI's data module, yet the menu module definitely does use its functionality. 😨Can somebody please double-check this?

#17: I do think this is the right call probably, as tempting as it may be to make Drupal 8's jQuery UI asset library dependency tree match jQuery UI modules 1:1. The only long-term benefit to doing that would be to more precisely deprecate things in Drupal 8.8 and 9.x, which would be a very small benefit. The risk associated with doing this is fairly high. So +1 for a as-minimal-as-possible solution: it simply adds missing JS files, adds missing asset library dependencies and does not add more asset libraries. That means this only fixes existing APIs, without increasing API surface. 👍

Patch review:

1. +++ b/core/core.libraries.yml
   @@ -498,11 +499,14 @@ jquery.ui.button:
   +    - core/jquery.ui.checkboxradio
   +    - core/jquery.ui.controlgroup
   

   🤔👍 As described above, this is only necessary if we want to retain backwards compatibility with code assuming jQuery UI 1.11. But we haven't done that for several years. Still, this is probably the sensible thing to do, since we also haven't set uiBackCompat = false.

   Any dependent code that is conditionally using .button() or .checkboxradio() depending on the value of uiBackCompat would still need this change. People could still be adding more code that depends on jQuery UI and hence could run into this.

   So +1 for this.

2. +++ b/core/core.libraries.yml
   @@ -498,11 +499,14 @@ jquery.ui.button:
    jquery.ui.checkboxradio:
      version: *jquery_ui_version
      license: *jquery_ui_license
      js:
   +    assets/vendor/jquery.ui/ui/form-reset-mixin-min.js: { weight: -11, minified: true }
        assets/vendor/jquery.ui/ui/widgets/checkboxradio-min.js: { minified: true }
   

   🤔 #3 was adding escape-selector-min.js to core/jquery.ui.checkboxradio. #17 does not do that anymore and adds form-reset-mixin-min.js instead. Why is that? Based on https://github.com/jquery/jquery-ui/blob/1.12.1/ui/widgets/checkboxradio.js, we should be adding both.

   Ah, that's because this depends on core/jquery.ui, which now is also loading escape-selector-min.js. All good! 👍

3. +++ b/core/core.libraries.yml
   @@ -650,6 +654,7 @@ jquery.ui.effects.puff:
   +    - core/jquery.ui.effects.scale
   

   ✅ Verified that this is a correct addition: https://github.com/jquery/jquery-ui/blob/1.12.1/ui/effects/effect-puff.j...

4. +++ b/core/core.libraries.yml
   @@ -666,6 +671,7 @@ jquery.ui.effects.scale:
   +    - core/jquery.ui.effects.size
   

   ✅ Verified that this is a correct addition: https://github.com/jquery/jquery-ui/blob/1.12.1/ui/effects/effect-scale....

5. +++ b/core/core.libraries.yml
   @@ -709,12 +715,14 @@ jquery.ui.menu:
   +    - core/jquery.ui.position
   

   ✅ Verified that this is a correct addition: https://github.com/jquery/jquery-ui/blob/1.12.1/ui/widgets/menu.js#L26

6. +++ b/core/core.libraries.yml
   @@ -709,12 +715,14 @@ jquery.ui.menu:
   +    assets/vendor/jquery.ui/ui/ie-min.js: { weight: -11, minified: true }
   
   @@ -769,6 +777,7 @@ jquery.ui.selectmenu:
   +    assets/vendor/jquery.ui/ui/form-reset-mixin-min.js: { weight: -11, minified: true }
   

   🤔 Why the weight: -11? I think it's because core/jquery.ui already is loading them with weight: -11, and we need to ensure that that loading order continues to be respected.

   Can you confirm this?

7. +++ b/core/core.libraries.yml
   @@ -852,6 +861,8 @@ jquery.ui.touch-punch:
   +    - core/jquery.ui.mouse
   +    - core/jquery.ui.widget
   

   ✅ Verified that this is a correct addition: https://github.com/furf/jquery-ui-touch-punch/blob/master/jquery.ui.touc...

8. It'd be valuable to also run the https://gist.github.com/KayLeung/b6455829db44d51f31aff27f6408b891 script and confirm that it does not discover any additional missing dependencies.

NW only for my last point in my review, once that is addressed I think is ready to be RTBC'd.

Thank you for the very detailed patch review @Wim Leers, I'm glad to know you agree with this approach.

1. This is probably unnecessary but I also think it's better to be consistent here and include these anyway

6. I'd kept the weights they had in the core/jquery.ui, however I did more testing and it doesn't seem to make a difference whether these weights are specified here. Removed from checkboxradio, mouse and selectmenu.

8. This script splits out everything listed in the "define" block for each file. It gives you a result like @droplet proposed in #2 or what @Gribnif continued in #11. I looked over this list to make sure it didn't catch anything we had missed, but I didn't see anything new. It missed the effects that @bnjmnm caught in #14 as well as the declared (in a comment) dependencies of touch-punch which are noted in point 7 of #18.
I posted an excerpt of the jquery.ui asset libraries after running this script against current 8.8.x in case anyone else wants to double check.

RE: broken effects: how did you discover this?

I manually tested individual effects and can confirm that without "effect-scale" puff was broken. It is also listed in the AMD define, which is the main way these dependencies are verified/found.
The script by @droplet searches within that method: var match = data.match(/define\(\ ?\[([^\]]*?)\]/);

For example, you say that accordion depends on data.

This was a false positive when I was searching those minified files.

... do not load jQuery UI's data module, yet the menu module definitely does use its functionality. 😨Can somebody please double-check this?

This was confusing for me too, I think it is because jQuery has a "data" method. I stepped through starting at line 5214 of jquery-ui.js at https://jqueryui.com/menu/ and confirmed that use of "data" is the jQuery method, not the jQuery UI data selector.
To be more certain I grep'd for the data selector :data, it appears in

• dialog
• draggable
• droppable
• sortable

It is not specifically defined in dialog or droppable but these both depend on draggable which does define "data".

Newest patch just removes the unnecessary weights pointed out in 6.

8. I think I know why the script didn't reveal that the effect-puff was missing a dependency on effect-scale: almost every dependency is on something one level up (../something), but this one is a dependency on something in the same level (./something).

This was confusing for me too, I think it is because jQuery has a "data" method.

That makes sense, thanks for figuring that out!

Everything else in #20 made sense, thanks! I think this is ready :) Updated the title. The summary could still use an update.

WOW, this issue was certainly way more than I bargained for going in. :D Thank you SO much for all the wonderful sleuthing here!

I talked to @Wim.Leers about the idea of testing this somehow, per @alexpott in #9, but it would basically involve parsing JS code from PHP (eek!) and not only would this be messy, but probably not worth the effort since we're planning on moving away from jQuery UI to the extent possible.

It seems like earlier there were some possible BC concerns, but the current patch is just adding some missing requirements lines, effectively. Nevertheless, we can be conservative here and only commit to 8.8.x; it doesn't seem like this is hitting people in the field much despite this bug has existed since 8.4.x(!)

Committed and pushed to 8.8.x. Thanks!