Problem/Motivation

Yet another small step in jQuery UI Dialog replacement.

Proposed resolution

Let's remove special jQuery UI _focusedElement and _focusTabbable from dialog Ajax and use focusable js for this.

CommentFileSizeAuthor
#20 3538177-nr-bot_qve60nz7.txt662 bytesneeds-review-queue-bot
#6 3538177-nr-bot_7a441nvz.txt846 bytesneeds-review-queue-bot

Issue fork drupal-3538177

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

finnsky created an issue. See original summary.

finnsky’s picture

finnsky commented
Title: Remove jQuery UI _focusedElement and _focusTabbable from dialog » Remove jQuery UI _focusedElement and _focusTabbable from dialog Ajax

finnsky opened merge request !12847

finnsky changed the visibility of the branch 3538177-remove-jquery-ui to hidden.

finnsky’s picture

finnsky commented
Status: Active » Needs review
Issue tags: -JavaScript +JavaScript
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
Issue tags: -JavaScript +JavaScript
StatusFileSize
new 3538177-nr-bot_7a441nvz.txt846 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Drupal\FunctionalJavascriptTests\Ajax\DialogTest seems to be failing which seems related.

finnsky’s picture

finnsky commented

I would probably have approach review before.

finnsky’s picture

finnsky commented
Issue summary: View changes
finnsky’s picture

finnsky commented

All works same as with jquery UI
https://github.com/jquery/jquery-ui/blob/7571b739fbe227a9f47c1bbbd1d9f18...

but without :)

please review.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Full disclosure wasn't 100% how to best test this one. So I went with the view approach

Applied the MR.
Edited the generic content view
opened dialogs
Resized my screen
Closed dialogs
Saved them
Whatever I could think of

And it still seems to be working as before. So going to take a swing and mark it.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs review

haha yeah views is a good way to know if we broke the JS usually.

We already take over jquery ui _focusTabbable to make it use tabbable library: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/dialog/d... how is this MR different?

finnsky’s picture

finnsky commented

Well, I'd say that in that file you're still working with a jQuery instance.

Here, pure vanilla does the same thing. And when the time comes, we can simply replace the classes and get a native dialog.

smustgrave’s picture

smustgrave commented

@nod_ thoughts on that?

smustgrave’s picture

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

Going to go on a limb and say feedback was addressed @nod_ still would like your thoughts though.

finnsky’s picture

finnsky commented

Rebased.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3538177-nr-bot_qve60nz7.txt662 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
smustgrave’s picture

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

Going to restore status..

  • longwave committed 3f9882e3 on 11.x 
    refactor: #3538177 Remove jQuery UI _focusedElement and _focusTabbable...

  • longwave committed 62adbdfd on main 
    refactor: #3538177 Remove jQuery UI _focusedElement and _focusTabbable...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.x-dev
Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

I read through dialog.jquery-ui.js and https://github.com/jquery/jquery-ui/blob/839ff8fae666e44d2633db81b1ae548... and this change matches the existing behaviour and makes sense to me.

As a followup should we deprecate the _focusTabbable() override somehow? Or is the idea that all of dialog.jquery-ui.js can go away once these conversions are done?

Committed and pushed 62adbdfd4cb to main and 3f9882e3de9 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !12847

finnsky’s picture

finnsky commented

@longwave
We can add deprecations message right here
https://git.drupalcode.org/project/drupal/-/blob/main/core/misc/dialog/d...

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

This MR/change triggers random failures, see https://git.drupalcode.org/issue/drupal-3566890/-/jobs/8878676

Status: Fixed » Closed (fixed)

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