Problem/Motivation

We added a shim for jQuery tabbable and added overrides for various parts of jQuery UI dialog to remove usages, however it looks like one was skipped. We need to add an additional override in order to be able to deprecate and then remove the shim.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#31 3296098-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3296098

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:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Title: Removal :tabble usage in dialog.js » Removal :tabbable usage in dialog.js

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

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

andypost
he/him
Primary language Russian
commented
Issue tags: +JavaScript
Related issues: +#2158943: Add a native dialog element to deprecate the jQuery UI dialog
catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Critical
Issue tags: -JavaScript +JavaScript

I think this is causing #3420713: Nightwatch tests fail with "unsupported pseudo: tabbable" after jQuery 4 update and hence blocking jQuery 4, bumping to critical.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Closed (outdated)

#6 is wrong, maybe the last :tabbable usage got fixed in the meantime.

pretty sure the nightwatch error on jQuery 4 is the shim itself, so we should close this in favour of removing the shim. Marking as outdated.

catch’s picture

catch
he/him
Primary language English
commented
Status: Closed (outdated) » Active

Nope #6, is right... MR coming up.

catch opened merge request !6899

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
commented

linting failed

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Yeah I've messed something up between copying the method over/tabbable conversion/linting fixes.

finnsky made their first commit to this issue’s fork.

finnsky opened merge request !7370

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

It was easier for me to start from scratch. Please review

finnsky’s picture

finnsky commented
Status: Needs review » Needs work

Something wrong happends with off-canvas
https://gyazo.com/b264ef37eb621d29a8ffb47807817494

finnsky’s picture

finnsky commented

This Fix doesn't look like `hot` anymore

https://jqueryui.com/upgrade-guide/1.12/#deprecated-dialogclass-in-favor...

Pushed fix for off-canvas. But dialog.js still has this option

* @prop {string} [dialogClass='']

Probably better to test it

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

If the class placement changes are good, the Drupal\Tests\system\Functional\Ajax\OffCanvasDialogTest test also need the same fix you already did in core/tests/Drupal/Tests/Core/Ajax/OpenOffCanvasDialogCommandTest.

catch changed the visibility of the branch 3296098-removal-tabbable-usage to hidden.

finnsky’s picture

finnsky commented

dialogClass

is attached in ignored code:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/assets/vendor...

so probably all places with deprecated dialogClass should be rewritten(checked at least)

finnsky’s picture

finnsky commented

I've replaced dialogClass usage. We also need to deprecate it in dialog.js somehow.
Some dialogs works wierd. Good testing required here.

catch’s picture

catch
he/him
Primary language English
commented

Down to a single fail in the pipeline in workspaces now, but it looks like workspaces is already updated in the MR?

I think we should probably go ahead here with a CR once that is fixed? The changes look fine, the main issue is notifying contrib about the dialogClass change, but breaking contrib with 11.x alphas because we updated to jQuery 4, is more notification for contrib than we currently have which is nothing. We can even grep contrib and open individual issues or open a rector/project update bot issue to look for it..

finnsky’s picture

finnsky commented

I've checked. Test is correct. Workspace module not attaches class. And broken now

Class appears in inline dialog options. but seems not rendered well.

https://gyazo.com/a701b6f34ee13beb41bfe61794dc63f2

Seems error in off canvas backend.

catch’s picture

catch
he/him
Primary language English
commented

I think we should commit this to unblock the jQuery 4 update, and then open a follow-up to figure out what to do with dialogClass. It seems to me that we should remove it from our 11.x fork altogether, and trigger a deprecation (if possible) when it's used in 10.3.x - but not sure how feasible that actually is between jQuery UI and our JavaScript deprecation support.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems @catch has done the review

Opened #3440676: [backport] Trigger a JavaScript deprecation error for dialogClass in forked dialog.js per #25.

Will go ahead and mark.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Looked at this a bit more and the PHP-level deprecation should be straightforward, I've pushed a commit for this.

Assuming that's OK, after 11.x commit, we'll want a 10.3.x MR here with the changes to the dialog command, without any of the js changes.

I think we can keep #3440676: [backport] Trigger a JavaScript deprecation error for dialogClass in forked dialog.js open for what to do with dialogClass in the forked dialog js itself.

catch opened merge request !7483

catch’s picture

catch
he/him
Primary language English
commented

The 10.3.x bc/deprecation layer is working, in that tests pass - apart from a couple of test failures due to the new deprecation being triggered.

So if we clean up the dialogClass usages in the 10.3.x branch the same as the 11.x branch, we should get to green on 10.3.x too. Waiting for a full run to finish that only has those deprecation failures before doing that though.

catch’s picture

catch
he/him
Primary language English
commented

Just realised after doing all that, there is probably no harm in backporting the dialog.js changes to 11.x, given that the deprecation/bc layer works. However we can maybe go ahead with the current approach then open a js backport issue, since that's less urgent.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3296098-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
Issue tags: +no-needs-review-bot, +jQuery 4
nod_’s picture

nod_
Primary language French
Location Lille
commented

couple of questions in the 10.3 branch, the 11.x is good to go IMO

amateescu’s picture

amateescu commented

Posted a few comments on the 11.x MR. Otherwise this looks good to me :)

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback from @nod_ appears to be addressed.

Updated the CR slightly to include a before/after snippet, I find it helpful but feel free to remove if it's clutter.

  • nod_ committed 66c3914d on 11.x 
    Issue #3296098 by catch, finnsky, smustgrave: Removal :tabbable usage in...

nod_ closed merge request !7370

  • nod_ committed 94bfd9fa on 10.3.x 
    Issue #3296098 by catch, finnsky, smustgrave: Removal :tabbable usage in...

nod_ closed merge request !7483

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 66c3914 and pushed to 11.x. Thanks!

Feels weird to commit 2 different patch for 2 branches that's what it is :)

Status: Fixed » Closed (fixed)

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