Get rid of jQuery in dialog events.

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.

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 #2158943: Add a native dialog element to deprecate the jQuery UI dialog to solve this? Or can we go further by triggering a native event on window?

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?

@finnsky 😍 love it!

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

Something in core listening to the old events?

If so, it works 😁

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

Tests are sadly still failing.

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

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.

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

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

Looks like this needs a rebase.

rebased

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.

sorry, comment by mistake

@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). I do think it's important to keep the BC layer around for the whole D11 support window

So yeah, should be in 11.x and 10.3.x otherwise it'll be hard to remake a patch that cherry pick cleanly.

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

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

[EDIT] This may be due to browser cache on multiple dev site rebuilds. I will file a real ticket if the issue persists.

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

nvm