Remove drupal.tabbingmanager's jQueryUI dependency

Too late to do this in 9.0.x now, so moving to 9.1.x. Thanks!

"Confirm that tabbingmanager only needs jquery.ui for ':tabbable'" I can confirm that :)

The library looks good, but as you can see the maintainer proposed you the maintainership :p

1. +++ b/core/assets/vendor/tabbable/index.umd.min.js
   @@ -0,0 +1,6 @@
   +* tabbable 5.1.0
   

   Let's update this to the latest version

2. +++ b/core/core.libraries.yml
   @@ -619,6 +619,24 @@ sortable:
   +    url: https://github.com/davidtheclark/tabbable/blob/5.1.0/LICENSE
   

   We should link to the license itself usign raw.githubusercontent.com.

3. +++ b/core/core.libraries.yml
   @@ -619,6 +619,24 @@ sortable:
   +    assets/vendor/tabbable/index.umd.min.js: { weight: -1 }
   

   Did you have to build the library manually? If you did, let's add documentation that documents exact steps that need to be taken.

4. +++ b/core/core.libraries.yml
   --- /dev/null
   +++ b/core/misc/jquery.ui.tabbable.shim.es6.js
   
   +++ b/core/misc/jquery.ui.tabbable.shim.es6.js
   @@ -0,0 +1,12 @@
   +    tabbable: function(element) {
   +      return isTabbable(element);
   +    },
   

   Shouldn't this be marked as deprecated?

1. +++ b/core/core.libraries.yml
   @@ -624,6 +624,24 @@ sortable:
   +tabbable.shim:
   

   I'd call that one tabbable.jquery, not sure I'd keep "shim", in any case by itself it's not very descriptive.

2. +++ b/core/modules/system/tests/modules/tabbable_test/tabbable_test.info.yml
   @@ -0,0 +1,5 @@
   +description: 'Module for the testing the :tabbable selector'
   

   The tests are only for the shim right? might be better to specify it

3. +++ b/core/misc/collapse.js
   --- /dev/null
   +++ b/core/misc/jquery.ui.tabbable.shim.es6.js
   

   can probably call that one tabbable.jquery.es6.js, like you said it's not tied to jQueryUI

Works well, behaves as expected on IE11. We save a few KB of JS on most pages not having to load base jquery ui

Made the changes suggested in #12. Please review

The shim file is missing file, and library names need to be changed everywhere.

1. It seems like we have at least ~10 usages of :tabbable so we should open issue for removing those and deprecating the shim.

2. +++ b/core/misc/jquery.ui.tabbable.jquery.es6.js
   @@ -0,0 +1,22 @@
   + * Defines a backwards-compatible shim for the jQuery :tabbable selector.
   

   Nit: jQuery UI :tabbable selector. This is not specific to jQuery UI but this is a BC layer for the jQuery UI feature.

3. +++ b/core/misc/jquery.ui.tabbable.jquery.es6.js
   @@ -0,0 +1,22 @@
   +      // jQueryUI :tabbable selector does not. This is due to this element type
   

   Nit: s/jQueryUI/jQuery UI

4. Not a strong preference but I think it would be nice to keep shim in the file name. This would be consistent with jQuery Cookie and would explicitly document that the file is just a shim between jQuery and Tabbable.

Ok with me.

Since we are adding a new library, we should do a dependency evaluation.

version 5.1.3 went out last week too: https://github.com/focus-trap/tabbable/releases/tag/v5.1.3

The dependency evaluation looks good ✨

+++ b/core/misc/jquery.tabbable.shim.es6.js
@@ -0,0 +1,23 @@
+      // considers a details element without a summary tabbable. The jQueryUI

+++ b/core/tests/Drupal/Nightwatch/Tests/tabbableShimTest.js
@@ -0,0 +1,193 @@
+// Test to confirm the :tabbable shim returns the same values as jQueryUI :tabbable would.

Nit: s/jQueryUI/jQuery UI

Looking good, still works on dialogs and all :)

I posted an additional question to https://github.com/focus-trap/tabbable/issues/136#issuecomment-722612968 since part of what we want to ensure is safe/coordinated disclosure for our production dependencies. I didn't add the usual question about whether they would notify us of an upcoming release because in the JavaScript ecosystem that's sort of like asking for unicorns.

#27 was answered here: https://github.com/focus-trap/tabbable/issues/136#issuecomment-723484998

This all seems pretty good to me - main thing is the maintainer is responsive and friendly.

Agreed; +1.

1. +++ b/core/assets/vendor/tabbable/README.txt
   @@ -0,0 +1,15 @@
   +Drupal core uses the UMD build of tabbable.
   +To create this build:
   +Navigate to the root directory of the tabbable library.
   

   Is there a reason why we're not adding a build step into package.json? - like we've got for jqueryui.

2. +++ b/core/misc/collapse.es6.js
   @@ -50,6 +50,12 @@
   +        // If this polyfill is in use, the browser does not recognize the
   +        // summary as a tabbable element. This must still be set to a negative
   +        // tabindex to prevent the isTabbable() method from the tabbable library
   +        // from returning true.
   +        $summary.attr('tabindex', '-1');
   

   Are we concerned that other contrib / custom JS is going to have to do the same thing? Or to ask this question the other way around do we always need to load the shim if we load the new tabbable library.

   I guess not because if something is relying on jquery's tabbable then they would be depending on jquery.ui and that loads the shim.

3. +++ b/core/modules/system/tests/modules/tabbable_shim_test/src/Controller/TabbableShimTestController.php
   @@ -0,0 +1,26 @@
   +  }
   +}
   \ No newline at end of file
   

   ---------------------------------------------------------------------------------------------------------------------------------------------
   FOUND 2 ERRORS AFFECTING 1 LINE
   ---------------------------------------------------------------------------------------------------------------------------------------------
    26 | ERROR | [x] Expected 1 newline at end of file; 0 found
    26 | ERROR | [x] The closing brace for the class must have an empty line before it
   ---------------------------------------------------------------------------------------------------------------------------------------------
   

Setting to needs review for point 1. I'd address 3 on commit if point 1 turns out to be moot.

for 1. we don't need to launch a build step ourselves. When doing a yarn add tabbable, the node_modules/tabbable/dist/ folder will hold all the files we need (index.umd.min.js and index.umd.min.js.map).

We got to change the way we manage js dependencies because it's hard to keep them up to date. If I'd change anything I'd remove the readme because I don't think we should be building the lib ourselves for this one, broader topic though #2658650: [META] Optimize Frontend Workflow in Core Development, #2873160: Implement core management of 3rd-party FE libraries

The sniffer errors pointed out in #30.3 were addressed in #33, and the reroll cleanly applies to latest 9.2.x. Looks like this is good to be back at RTBC.

+++ b/core/core.libraries.yml
@@ -624,6 +624,24 @@ sortable:
+  version: "5.1.3"

I think this looks great except there's a new release of tabbable. Feel free to move this back to RTBC after updating to the latest version of the library.

Made the update suggested in #37. Please review and RTBC.

Previous patch didnt have the changes for some reason. Here's the update.

+++ b/core/assets/vendor/tabbable/index.umd.min.js
@@ -0,0 +1,6 @@
+* tabbable 5.1.3

We also have to update the library using the instructions in the tabbable README which can be found in the patch.

I think we still need a change record. We should at least mention that there's now a new library that can be used for checking whether an element is tabbable or not.

Needs a reroll because #3113400: Deprecate more jQuery UI library definitions landed.

Should we remove jQuery UI tabbable files from core as part of the patch since we don't have any references to it anymore after this patch?

fwiw I agree with leaving it in. In the event that someone is using it directly somehow, it's better for them to find out when they port to Drupal 10 than a random minor release, plus all the points in #47.

I don't have strong opinion to either direction but I thought it would make sense to remove the file since that's the process we followed in #2550717: [JS] Replace jQuery.cookie with JS-cookie and provide a BC layer too. In my opinion, it would be fine to draw a line that a specific file is not an API but a library is.

We also removed jQuery UI components in #3087685: Remove deprecated jQuery UI components and fork remaining source code into core without having tests or rolling a new release, so if we wanted to do so here, we probably could.

@lauriii #3087685: Remove deprecated jQuery UI components and fork remaining source code into core was only committed to 9.0.x and #2550717: [JS] Replace jQuery.cookie with JS-cookie and provide a BC layer provided a BC layer. I agree with @catch that we should leave the file in place in a minor release (but indicate that it's deprecated) and only remove it in 10.0.x.

What would we do if there was a security vulnerability in code in jQuery UI components that are not being used by core anymore? If we assume people could be using it, shouldn't we also provide security coverage for it? How are we planning to handle testing those components that are not used by core? All very hypothetical at this point, especially with the tabbingmanager but if we set the precedent in this issue, there could be other jQuery components that are riskier.

The patch in this issue provides BC layer similar to #2550717: [JS] Replace jQuery.cookie with JS-cookie and provide a BC layer so I believe they are comparable. Only difference is that this patch requires making some modifications to jQuery UI code.

Seems like there's still reference to tabbable from jQuery UI dialog widget which probably should be removed. That also raised a question; should we introduce intergration tests to jQuery UI widgets that are using shims? That could be useful if we ever have to make changes to the shim.

I don't have strong concerns on leaving the references in AMD definition but I thought it would be consistent with what we have done so far, and would leave the files in a bit cleaner state. I pinged @nod_ to see what he thinks.

We do have extensive unit tests for the shim. Regardless of that, it is useful to have some integration tests to ensure that the integration between different units works. The test could be very minimal, just proving that the integration between the shim and jQuery UI works.

+++ b/core/tests/Drupal/Nightwatch/Tests/tabbableShimTest.js
@@ -0,0 +1,331 @@
+          const stuff = new Function(`return ${testActions}`)();
+          const expectedActiveElement = stuff($element);

Why do we need to create this self invoking function here? Maybe we could rename the variable to be more descriptive and add a comment

1. +++ b/core/core.libraries.yml
   @@ -22,6 +22,16 @@ ckeditor:
   +    assets/vendor/css-escape/css.escape.js: { weight: -2 }
   

   Polyfills have usually used weight: -20. Is there any particular reason for it to be different for this?

2. +++ b/core/core.libraries.yml
   @@ -126,6 +135,7 @@ drupal.autocomplete:
   +    - core/le.jquery.shim
   

   This should probably be core/tabbable.jquery.shim.

3. We should dependency evaluate CSS.escape too.
4. Can we write another change record about the changes to our library definitions? These changes could have an impact on themes that are overriding these libraries using libraries-override.

Maybe I missed it but is there manual testing needed for this? I searched the comments and didn't see how it was tested manually if it was done. Or, does the automated tests cover everything?

I think this is ready for a review from another committer. I reviewed the patch one more time, tested it manually using the steps provided in #71 and reviewed the CR's, and all looked good for me 👌

I reviewed the patch, checked the change notices, went through the step in #71 with IE11 (except the ckeditor from the offcanvas, which i didn't find how to trigger to be honest, even with the steps 🤷) And it's all good, RTBC +1

I reviewed the patch again, and it looks all right, I followed the instructions in # 71. RTBC +1

Discussed with @xjm and agreed that some of the libraries changes are potentially disruptive. Tagging for release manager review to give them a chance to give feedback.

Adding more background for #76.

I discussed #51 with @xjm to make sure that she would be fine with the current approach. That comment documents a recommendation from the release managers to keep the files previously referenced by the libraries in the code base until the next major release. That comment references #2550717: [JS] Replace jQuery.cookie with JS-cookie and provide a BC layer and states that it was acceptable to remove the files there because there was a shim that provided full BC. Because this issue is using the same approach, I suggested on #52 that we would use the same approach here.

As we discussed this, I mentioned that I believe that the changes to library definitions made by this issue (and other jQuery UI related issues), have much higher potential of being disruptive than removing files from core to some users. The libraries system uses file paths as the identifier for alters and overrides.

For example, before #3113400: Deprecate more jQuery UI library definitions and this issue, someone could have replaced the jQuery UI tabbable with custom code with the following configuration:

libraries-override:
  core/jquery.ui:
    js:
      assets/vendor/jquery.ui/ui/tabbable-min.js: js/custom-tabbable.js

As a result of these changes, tabbable-min.js is no longer part of the library definition, meaning that any overrides to that file no longer take effect until they have updated to overriding an asset file that is defined in the library definition. I don't think it's likely that many sites would be impacted by this but I mentioned that if we are concerned by this, #3113400: Deprecate more jQuery UI library definitions would have much higher risk of being disruptive because it applies even broader scope of changes to the library definitions.

The same behavior impacts CSS and we have added BC layers to Stable to handle changes to CSS library definitions. See stable_library_info_alter for an example of that. Theoretically we could provide similar BC layer to JavaScript, but that would be more complex because we would have to check if a specific file is being overridden and then make some more involved changes to the libraries definitions based on that.

If we decide to keep the file in place, we should discuss what we would do if there was a security vulnerability in jQuery UI components that are not being used by core anymore and how are we planning to handle testing those components that are not used by core. It sounds like this is not unique to this issue, so maybe there's already precedent to this.

If we decide to keep the file in place, we should discuss what we would do if there was a security vulnerability in jQuery UI components that are not being used by core anymore and how are we planning to handle testing those components that are not used by core. It sounds like this is not unique to this issue, so maybe there's already precedent to this.

Correct; that is not blocking because we already are providing security coverage for the jQuery UI code in core.

There are quite a few references to tabbable in contrib. Will all these modules need to change their code? It's not clear to me from the change record on the addition of tabbable.

I checked the search results from #79.

Some of the results are unrelated uses of tabbable word for other purposes. Bootstrap theme is using it as a classname for targeting horizontal tabs which doesn't have any connection to jQuery UI :tabbable selector.

There are some results that are more related. Some of the search results are AMD module definitions from a copy of jQuery UI as well as some other jQuery UI components defining it as a dependency using AMD. However, Drupal is not using AMD for JavaScript module loading so these can be ignored.

I'm surprised how few references the :tabbable selector has in contrib. However, even these wouldn't have to be changed yet since the shim is not deprecated. This means the change record is only informing users that a new library is added so that developers writing new code could take advantage of the newly added library.

TL;DR is that I don't expect that any of the contrib modules listed in #79 would have to implement changes after this gets committed.

I was debugging an issue with mherchel with https://www.drupal.org/project/anchor_link module

While debugging, we used the chrome browser inspector tools network tab to Block request URL for various js files. When I blocked core/assets/vendor/jquery.ui/ui/tabbable-min.js the bug went away.

And a potential fix (using custom code) is https://www.drupal.org/project/drupal/issues/3065095#comment-14010068

We thought since this issue is refactoring things, that the bug with anchor_link module might be useful as a testing use case ... or inspire a test ... for the work on this issue.

If we decide to keep the file in place, we should discuss what we would do if there was a security vulnerability in jQuery UI components that are not being used by core anymore and how are we planning to handle testing those components that are not used by core. It sounds like this is not unique to this issue, so maybe there's already precedent to this.

I think this would be the equivalent of a security issue not exposed by core itself; only contrib relying on the deprecated file would be affected. So any potential security issue would only be against those contrib modules, and they could either fix it within the module itself or just replace the deprecated file usage with the correct/supported usage of the tabbingmanager library.

Edit: That said, #80 seems fine, so we're back to just whether to retain the file or not.

Based on #83 and #51 it seems like the current approach of leaving the file in would be fine to the release managers. I'm removing the needs release manager review tag since I was the only one pushing back on keeping the file, and it seems like all of the other concerns have been addressed. While I don't necessarily think that the current solution is ideal, I'm fine with it being the way it is as long as it helps us move forward on this. We can always remove the file later in #3201595: possibly remove jQuery UI tabbable files?.

Checked the latest patch, still working as expected. RTBC+1

This looks really good. One less use of jquery ftw. We've now managed to defer deleting the file and we can make that decision in #3201595: possibly remove jQuery UI tabbable files?.

Dependency evaluations are complete and we have a change record and release note.

Committed daf10a0 and pushed to 9.2.x. Thanks!