Get rid of jQuery in dialog events.

Seems some cleanup needed in
core/misc/dialog/dialog.position.js

Current Functional Javascript Test failure seems related to core/modules/block/js/block.js:139
https://i.gyazo.com/1176e9dd6dd6554a6652f61ef36721e4.gif

Not to this MR

Only rebased to get tests to run again,

As we thought was random.

Applied the MR and edited a view, used layout builder, and media library without issue, believe that would cover the js files.

Still needs sign off from the javascript maintainer but from what I see the changes seem fine. Eventually is there any kind of rule or check we could add?

Was told that can mark issues RTBC that are waiting on submaintainer so thats what I'm going to do.

Rebased

Just ran into this in Homebox 3.x implementation:
Tried to replace

jQuery(window).on("dialog:beforecreate", function (event) {
        jQuery("body:first").addClass("homebox-sidebar-open");
      });
      jQuery(window).on("dialog:beforeclose", function (event) {
        jQuery("body:first").removeClass("homebox-sidebar-open");
      });

by

      window.addEventListener("dialog:beforecreate", function () {
        // Add the 'homebox-sidebar-open' class before creating the dialog
        document.body.classList.add("homebox-sidebar-open");
      });
      window.addEventListener("dialog:beforeclose", function () {
        // Remove the 'homebox-sidebar-open' class before closing the dialog
        document.body.classList.remove("homebox-sidebar-open");
      });

BUT that's not possible: https://stackoverflow.com/questions/25256173/can-i-use-jquery-trigger-wi...

So would be super cool to have this fixed or at least also trigger the vanilla JS event.

The attached MR will not solve this, as it still uses jQuery .trigger() and .on()!
So I guess we need to wait for 📌 Add a native dialog element to deprecate the jQuery UI dialog Needs work to solve this? Or can we go further by triggering a native event on window?

@Anybody hi! thank for review.

Yes this is small MR and only cares about event parameter. Not listener or event type.

This is how we can move forward with small steps.

What are the BC implications here? I assume anyone with a listener will have issues with the changes here.

Can we do this in a BC compatible manner?

Perhaps dispatch different events without jQuery and deprecate the old event?

@larowlan

Please take a look at the last commit! This is what you mean.

@Anybody it will also work for your question.

@finnsky 😍 love it!

Hate to do it but seems to have caused test failures.

Seems exact reason of test failures is this deprecation message

Something in core listening to the old events?

If so, it works 😁

Added condition to check if `dialog:beforecreated` listener presents on page.
Only in this case old event will triggered. And deprecation message will appear.

It was 20 minutes Adventure! :)

I had to replace all usages to new listeners because seems core not allows to keep deprecated messages.
Checked all scripts/events manually. All fine Seems autotests also passed.

Please review!

Some new tests failure gonna fix. Review still needed.

I like this a lot.

One major concern is the renaming of the event, I think we can avoid having to rename it because that would be a pain for contrib to deal with.

I'd like to remove only the .trigger call, not addevent listeners yet. That will allow people to use dom events without breaking their existing code. Then we can remove jquery.on afterwards.

tags

Thank you for review. Gonna rework it according feedbacks.

RE: #28

We cannot keep Deprecation messages in current core code. Because of pipeline failures.
So that's why i had to rework core `.on()` Seems it is minimal change we can provide in one MR.

Tests are sadly still failing.

Probably that were random tests failures. Now seems passed on everything. Sending back to review.

Impressive work @finnsky! Back to RTBC. All feedback seems addressed.

Let's review again. I forgot to rename one event.

it was just dialogAfterclose -> dialog:afterclose change. so keep in RTBC. Test still passed.

finnsky → changed the visibility of the branch 11.x to hidden.

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions.

The issue summary includes a list of API changes. If that is up to date this needs a change record.

Leaving at RTBC.

Added some IS updates to be aligned with current state.

It's solid work, a couple of comments. Mainly about keeping the settings value alterable and using a subclass of Event instead of CustomEvent everywhere.

Setting NW for feedback and the change record.

few things left, the change record needs more details

Thank you! Fixed feedbacks

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.

This looks great, thanks @finnsky for such a thorough solution

Looks like nod_ has been reviewing this along the way, It looks good to me but I'll defer to his expertise.

Removing the Needs Framework Manager tag

Switching to NW to address the (admittedly superficial-ish) feedback on the MR. I really like how this is approached - I've witnessed several attempts over the past few years to implement something like this and this approach not only solves the problem, but it does it pretty elegantly. The change record covers the necessary info, too, so I'm removing that tag

Resolved feedbacks.
Please rereview :)

@bnjmnm could you please review again? i just rebased

@bnjmnm @nod_

Thank you for reviews!
Seems we are ready to go. I'll request some tests there.

Looks like this needs a rebase.

rebased

It seems now has random failure in tests.

reran the failing test, green now.

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.

I was involved with the code so I wanted to have another person RTBC so that I could commit but it doesn't seem to help get that in. It's a 11.x commit only.

Replace $(window).on('dialog:beforecreate') with window.addEventListener('dialog:beforecreate').

While $(window).on('dialog:EVENTTYPE') remains backward compatible, listeners will receive deprecation messages. Additionally, a deprecation script checks if the old dialog:beforecreate is present in $._data(window, 'events'), triggering a deprecation message if found."

But checked MR, changes looking fixed now

@Pravesh_Poonia I'm not sure what you mean by "But checked MR, changes looking fixed now" . Your text looks automated/generated, could you elaborate as to what you were trying to do? Thanks

One small comment on the MR about the deprecation target - left a suggestion to change it to 10.3.0, but then I saw @nod_ say it's an 11.x commit only, it's not entirely clear to me why we wouldn't backport the deprecation to 10.3 too, generally we've avoided adding deprecations specific to the .0.0 of a new major so that modules don't get new warnings immediately after adding major version compatibility.

I'm not qualified to review the details of the js here, but general +1 on the approach. Just had serious pain with dialog and the jQuery 4 update so any small steps we can take to simplifying it will help.

I was being too cautious limiting to 11.x (was still provisional at the time, might be part of the reason too).

So yeah, 11.x and 10.3.x otherwise it'll be hard to remake a patch

This is great!

Committed to 11.x

Will also commit to 10.3.x, but would first like to run tests on that combo as I've encountered a few JS patches that had unexpected backport surprises

awesome, there was a conflict in libraries.yml on the backport, opened a MR for 10.3.x

10.3.x pipeline passed now

Committed and pushed 99af345889 to 10.4.x and 66faba3a71 to 10.3.x. Thanks!

Thanks! Published the change notice, does it need release note?

Deprecation is triggered automatically and we have a change record, there's nothing here that we need to notify site owners about in the release note that I can see.

This is throwing fatal JS errors and breaking contrib modals:

Uncaught TypeError: event is undefined
    handle https://xxxx/core/misc/dialog/dialog-deprecation.js?v=10.3.0-dev:12
    jQuery 7
    openDialog https://xxxx/core/misc/dialog/dialog.js?v=10.3.0-dev:83
    showModal https://xxxx/core/misc/dialog/dialog.js?v=10.3.0-dev:101
    attach https://xxxx/modules/contrib/moderated_content_bulk_publish/js/moderated_content_bulk_publish.js?semf9b:120
    jQuery 2

It's also broken Bootstrap Modal, looking for event.dialog where instead the fix is to look for $event.dialog.

This also seems to break the Contextual links, please check https://www.drupal.org/project/drupal/issues/3458067 🐛 Contextual links disappear intermittently leading to console errors Active