Problem/Motivation

9.2.x added jquery.tabbable.shim which provides bc for tabbable, with a deprecation message.

core/drupal.autocomplete and core/drupal.dialog both depend on the library to provide that bc.

The library itself isn't deprecated in 9.x yet.

Steps to reproduce

Proposed resolution

I think we have three options, roughly in order of preference/how easy they're likely to be:
1. Remove the shim in 10.0.x, without a 9.x deprecation, if we can't find any contrib modules that depend on it - basically consider it @internal. (this isn't possible to do cleanly because core depends on it)
2. Deprecate the library in 9.x for 10.x (by directly referencing the file from dialog and autocomplete, or suppressing the deprecation message in tests) and remove it from 10.x
3. Remove the usages from dialog and autocomplete in 10.x, and then deprecate the library in 10.1.x for removal in 11.0.0

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 image_2022-05-04_135951337.png194.2 KBtaran2l

Issue fork drupal-3278625

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

catch created an issue. See original summary.

taran2l’s picture

StatusFileSize
new194.2 KB

So, seems like :tabbable is being used by

  • jQuery Joyride
  • jQuery UI Dialog

Then

Todo:

  1. Drop tabbable dependency for jQuery UI Autocomplete
  2. There is one more usage in dialog, that is not overwritten, see _createWrapper method
  3. Remove/replace jquery joyride library, or overwrite usage of tabbable there (the same approach as with jQuery UI Dialog)
catch’s picture

@Taran2L are you looking at 9.4.x? Tour has been switched to use Shepherd in 10.0.x, so shouldn't be an issue there #3051766: Deprecate and replace jQuery Joyride (for tours), although the 9.4.x issue makes deprecating the library a bit harder.

We've unforked jQuery UI back to core/assets/vendor, and there's lots of references to :tabbable again, so it's possible we actually can't remove this after all, and need to defer it until #2158943: Add a native dialog element to deprecate the jQuery UI dialog is done.

taran2l’s picture

@catch, yep was looking D9.4, sorry.

So Joyride is out of question then; and https://git.drupalcode.org/search?search=%3Atabbable&nav_source=navbar&p... gives only one usage in jQuery UI Dialog -> https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/assets/vend...

If that is correct, then this single usage has slipped from our sight in #3179734: Refactor uses of the :tabbable selector and deprecate it (where other usages has been overwritten to rely on core/tabbable), and just needs to be added and then shim can be removed safely

Am I missing something?

taran2l’s picture

taran2l’s picture

catch’s picture

There seem to be additional usages in one more method in dialog, here: https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/assets/vend...

The issue with this is because it's (now) vendor code, we can't directly edit it. We could maybe move the tabbable shim as a file under the dialog library definition instead of it being its own library?

taran2l’s picture

@catch, those usages are covered by https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/misc/dialog..., and by covered I mean that this method has been overwritten, i.e. replaced by our own method that is using core/tabbable library. This work has been completed in #3179734: Refactor uses of the :tabbable selector and deprecate it. Somehow, one usage is still there ... maybe it was added afterwards by vendor update.

catch’s picture

@Taran2L ohhhh now I get it! So we need to override another method to refactor away :tabbable usage, then we can deprecate from there, makes sense now.

taran2l’s picture

@catch yay!

Based on that (imho) the todo list:

  1. Replace the remaining usage of :tabbable in dialog.js
  2. Deprecate tabbable shim library in 9.x and remove in 10.0.x
  3. (Probably) It would be a good idea to set up a check whether more usages of :tabbable are being added by future vendor updates. Could be a part of https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/scripts/js/...
catch’s picture

Issue summary: View changes
catch’s picture

Title: Remove/deprecate jquery.tabbable.shim » [PP-1] Remove/deprecate jquery.tabbable.shim
Version: 10.0.x-dev » 10.1.x-dev

We missed 9.4.x with this one. I've opened #3296096: Update tabbable deprecation messages to 11.x to update the deprecation message for removal in 11.0.0.

Also opened #3296098: Removal :tabbable usage in dialog.js for the remaining usage.

I think this issue needs to be for 10.1.x now, and postponed on removing that last usage.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

catch’s picture

Title: [PP-1] Remove/deprecate jquery.tabbable.shim » Deprecate jquery.tabbable.shim
Status: Postponed » Active
Issue tags: -JavaScript +JavaScript

#3296098: Removal :tabbable usage in dialog.js has a clean MR here, so I think we should try to deprecate this in 10.3.x for removal in 11.0.0 or 12.0.0

catch’s picture

#3296098: Removal :tabbable usage in dialog.js landed. Could we deprecate in 11.0.0 for removal in 12.0.0 here? We can't deprecate in 10.3 without backporting more of #3296098: Removal :tabbable usage in dialog.js (possible but extra work), or I guess this could wait until 11.1 if we don't want to deprecate new things in 11.0

catch’s picture

Priority: Normal » Critical

This is breaking on jQuery 4, which is blocking Drupal 11, bumping to critical. Don't see why we would fix something that's no longer used in 11.x and should have been deprecated years ago as a blocker to releasing Drupal 11. Working on an MR for the library deprecation.

catch’s picture

Status: Active » Needs review

We can do this in 10.3.x without backporting all the JavaScript changes to 10.3.0 if we add the library deprecation message to deprecation-ignore.txt. Given there has been a valid replacement for this library since Drupal 9.2, I think that might be fine.

xjm’s picture

+1 on this approach for the reasons described in #20 and #21.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

This works for me, 10.3.x deprecation looks good, and 11.x removal is good too.

catch’s picture

Title: Deprecate jquery.tabbable.shim » Deprecate/remove jquery.tabbable.shim

If we wanted to remove the deprecation skip later, we coudl do #3440676: [backport] Trigger a JavaScript deprecation error for dialogClass in forked dialog.js in 10.3.x by backporting more of #3296098: Removal :tabbable usage in dialog.js, that would in turn allow us to remove the tabbable shim dependency from the dialog library in 10.3.x and then the deprecation wouldn't get triggered. However I think our time is much better spent on #2158943: Add a native dialog element to deprecate the jQuery UI dialog and friends so we don't have to keep forking more and more of jQuery UI to more and more places.

Thanks for the quick reviews here, can't wait to get rid of this!

longwave’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

I think this is the most pragmatic thing to do at this point, seeing as we should have removed this some time ago and we can spend effort better elsewhere.

Committed 7a21759 and pushed to 11.x. Thanks!
Committed b3872f8 and pushed to 10.3.x. Thanks!

  • longwave committed b3872f84 on 10.3.x
    Issue #3278625 by catch, Taran2L: Deprecate jquery.tabbable.shim
    

  • longwave committed 7a21759c on 11.x
    Issue #3278625 by catch, Taran2L: Remove jquery.tabbable.shim
    

xjm’s picture

Something ate @longwave's credit for committing this; fixed now.

nod_’s picture

credit doesn't happen automatically for commit if you set it fixed before the commits show up on the issue :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

Amending attribution.