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
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | image_2022-05-04_135951337.png | 194.2 KB | taran2l |
Issue fork drupal-3278625
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
Comment #2
taran2lSo, seems like :tabbable is being used by
Then
Todo:
_createWrappermethodComment #3
catch@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.
Comment #4
taran2l@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?
Comment #5
taran2lComment #6
taran2lComment #7
catchThere 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?
Comment #8
taran2l@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/tabbablelibrary. 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.Comment #9
catch@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.
Comment #10
taran2l@catch yay!
Based on that (imho) the todo list:
:tabbablein dialog.js:tabbableare being added by future vendor updates. Could be a part of https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/scripts/js/...Comment #11
longwaveComment #12
catchComment #13
catchWe 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.
Comment #14
catchReparenting to Drupal 11.
Comment #16
andypostComment #17
catch#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
Comment #18
catch#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
Comment #20
catchThis 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.
Comment #21
catchWe 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.
Comment #23
xjm+1 on this approach for the reasons described in #20 and #21.
Comment #24
nod_This works for me, 10.3.x deprecation looks good, and 11.x removal is good too.
Comment #25
catchIf 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!
Comment #26
longwaveI 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!
Comment #31
xjmSomething ate @longwave's credit for committing this; fixed now.
Comment #32
nod_credit doesn't happen automatically for commit if you set it fixed before the commits show up on the issue :)
Comment #34
xjmAmending attribution.