Replace Contextual Links BackboneJS usage with VanillaJS equivalent

I wonder if we can’t replace it with pure CSS, see https://css-tricks.com/solved-with-css-dropdown-menus/

#2,

the dropdown will or should be replaced with #1899236: Add new Splitbutton render element to eventually replace Dropbutton

I think we should do simple conversion first and tweaks later (easier review)

@droplet, it seems that splitbutton is still based on JS.

A CSS only solution wouldn't be accessible.

Looks like there's a merge request here for review

opened a new issue to not have to deal with missing branches and rebasing 500+ commits #3275400: Contextual backbone removal.

I'll probably post the patches here once it's good to go.

updated IS with API changes

Code from #3275400: Contextual backbone removal that I'll close.

This didn't apply anymore after, straight re-roll, reviewing the code locally

Part way done reviewing, will continue tomorrow

1. +++ b/core/modules/contextual/js/contextual.es6.js
   @@ -3,7 +3,7 @@
      const options = $.extend(
        drupalSettings.contextual,
   

   should we use Object.assign here while we're touching this file or would you prefer to leave that to one of the jquery eslint issues?

2. +++ b/core/modules/contextual/js/contextual.es6.js
   @@ -86,8 +85,6 @@
      function initContextual($contextual, html) {
        const $region = $contextual.closest('.contextual-region');
   

   the comment on this method still refers to 'sets up model and views' so we should be able to reword that now

3. +++ b/core/modules/contextual/js/contextual.es6.js
   @@ -113,29 +110,21 @@
        // Create a model and the appropriate views.
   

   similarly here, its just a 'model view' now rather than two things

4. +++ b/core/modules/contextual/js/contextual.es6.js
   @@ -156,59 +145,58 @@
   +                $context.find(`[data-contextual-id="${id}"]:empty`).eq(0),
   

   should we replace this with querySelector whilst we're here?

5. +++ b/core/modules/contextual/js/contextual.es6.js
   @@ -220,14 +208,18 @@
   +                  .forEach(($placeholder) => {
   +                    initContextual($placeholder, html);
   

   should we change the signature of initContextual now to not require a jQuery object as the argument?

   there's going to be some BC breaks here we can't avoid already, so should we bundle that change in here too and give us a way to remove jQuery without a further break?

   we can still init it as a jQuery object inside the function to avoid moving too much around in this issue

6. +++ b/core/modules/contextual/js/contextualModelView.es6.js
   @@ -0,0 +1,245 @@
   +      $(document).trigger('drupalContextualLinkAdded', {
   +        $el: $contextual,
   +        $region,
   +        model: this,
   

   we fire this here and in initContextual, (which calls this), which seems wrong - should we only fire it once?

7. +++ b/core/modules/contextual/js/contextualModelView.es6.js
   @@ -0,0 +1,245 @@
   +      this.$region.toggleClass('focus', this.hasFocus);
   +      this.$contextual
   +        .toggleClass('open', isOpen)
   +        // Update the visibility of the trigger.
   +        .find('.trigger')
   +        .toggleClass('visually-hidden', !isVisible);
   +
   +      this.$contextual.find('.contextual-links').prop('hidden', !isOpen);
   +      this.$contextual
   +        .find('.trigger')
   +        .text(
   +          Drupal.t('@action @title configuration options', {
   +            '@action': !isOpen ? this.strings.open : this.strings.close,
   +            '@title': this.title,
   +          }),
   +        )
   +        .attr('aria-pressed', isOpen);
   

   there's a lot of new jQuery here, should we be adding new jQuery code?

   All of this can be done with vanilla js

second half of review, onto manual testing now

1. +++ b/core/modules/contextual/js/contextualModelView.es6.js
   @@ -0,0 +1,245 @@
   +      const { isOpen } = this;
   +      const isVisible = this.isLocked || this.regionIsHovered || isOpen;
   +      this.$region.toggleClass('focus', this.hasFocus);
   +      this.$contextual
   +        .toggleClass('open', isOpen)
   +        // Update the visibility of the trigger.
   

   We call this a lot, should we keep a hash of the properties that can change and only do the work if they've actually changed?

2. +++ b/core/modules/contextual/js/toolbar/contextualToolbarModelView.es6.js
   @@ -0,0 +1,145 @@
   +        touchend: (event) => {
   +          event.preventDefault();
   +          event.target.click();
   +        },
   

   this is the same event body as on contextualmodelview, should we make it a static and use the same thing in both places?

3. +++ b/core/modules/contextual/js/toolbar/contextualToolbarModelView.es6.js
   @@ -0,0 +1,145 @@
   +      if (!this.announcedOnce && event.keyCode === 9 && !this.isViewing) {
   ...
   +      if (event.keyCode === 27) {
   

   event.keyCode is deprecated, so should we use event.code now or do we still care about IE11 (for now)

4. +++ b/core/modules/contextual/js/toolbar/contextualToolbarModelView.es6.js
   @@ -0,0 +1,145 @@
   +      const button = this.$el[0].querySelector('button');
   

   odd mix of vanilla and jquery here?

5. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/EditModeTest.php
   @@ -56,7 +56,7 @@ public function testEditModeEnableDisable() {
   -    for ($page_get_count = 0; $page_get_count < 2; $page_get_count++) {
   +    for ($page_get_count = 0; $page_get_count < 1; $page_get_count++) {
   

   why was this changed needed?

Manually tested as follows:

* Install umami
* Navigate to recipe
* Tab to edit button in toolbar
* Activate edit mode
* Confirm tabbing constrained to contextual links
* Confirm menu can be opened via keyboard
* Confirm links work via keyboard
* Confirm Esc key exits tabbing constraint
* Confirm works via mouse/hover
* Confirms menu can be opened and links can be clicked

Needs a reroll - should this be targeted at 10.0.x now, given that we can't remove the deprecated code until then anyway?

Re-roll patch created for 9.5.x, Please have a look.

Patch from #26 could be applied.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.

Careful reroll
The patch can be tricky to reroll. When rerolling this issue please make sure all the parts of the patch are kept (new files, new tests, all the changes to the different parts of the file, changes to es6 files ported to .js files, etc.)

Tried to re-roll for 10.1.x.
Created new files as well as deleted the ones which were deleted in #26.
Please review.

As per the comment #30 a reroll is required but in comment 31 can be seen that few files are missing as said in #32 so i updating the reroll patch and attaching the intra_diff file along with the patch, please review if needed something to modified please suggest and move back as needed.

Errors in patch

Fixed some of indentation error. Keeping it Needs work to fix PHPStan error

Re-roll patch #36 and Try to fix Cs issue

fix command failures #37

Try To Fix the #38 Patch.

@Prem Suthar, Interdiff is missing in #39, We should add that along with the patch. Its help to the reviewers to find the difference and what are the changes we have made from last patch. Also some Tests fails because some of the files are missing in the patch. Please compare with #26.

Re-roll patch needed for 11.x.

Re-roll patch created for 11.x, We have added some missing JS files according to #26 and not include es6.js files with reference of this document. Keeps as is "Needs work" because we have some unaddressed comments above.

#3484850: [PP-1] [meta] Deprecate Toolbar module will be unblocked soon, which will leave this as the last remaining usage of backbone in Drupal core. It would be great to be able to remove the backbone library from Drupal 12. Bumping this to major. Looks like the next step here is converting the patch to an MR.

Rebased the MR off 11.x

Fixed the pipeline so it would run but appears to have broken a few tests.

Fixed the bad reroll which could have been me but also could have been a previous iteration.
Seems to be working locally in manual testing.

Failing tests look valid

There were some underscore references left behind. Got rid of those because they're not needed in 2025 - we have native methods

Spot checked some tests locally and its looking good

There is a functional JS failure that looks relevant, I can't rerun it since a core committer created the branch.

Yep it's a real fail
Closing the contextual link list used to place focus on the trigger but that isn't happening now.
Tried a few things but they all end up in a loop
Will revisit this week

Dug some more into the final fail
The issue is that when the dialog is opened from the contextual link that uses ajax and a dlalog, when jQueryUI attempts this code

this.opener = $( this.document[ 0 ].activeElement );

we've already blurred the link and hence there's no focussed element to use as the opener, which means we can't return the focus.

Any ideas @bnjmnm?

Realized that the order events was being attached were different so re-ordered to ensure that ajax events are added first.
Also worked to reduce the amount of re-renders.
Then also tried using a targeted event (e.g. click.contextualLinks)

None of those helped

Tried to go back a few commits and fixed a problem with hovering out of a region and contextual links not closing, not sure it's the same issue as #51 but it was broken before and now it's not. Can you let me know how to reproduce the failure with dialog?

opened a new MR to not erase the work in case it's a different bug.

Also all the comments have been removed, would be good to keep them around, they help with understanding what's going on

could reproduce the problem with ajax using contextual_test module.

It seems it's the event delegation that used to be in backbone that was making things work. Somewhere it was switched to direct event binding and because there is so much re-rendering the event listeners were trashed and it caused issues apparently.

MR is green, need the comments added back for the various function we copy/pasted.

Added the docs back

This is green and should be pretty close, thanks @nod_ for the last fix

I can't content on the mr for some reason.

Some comments could still use cleanup:

// This uses proxy to look for added instances. Backbone also listened for
// reset and remove, which I'm unsure how do accomplish.

Otherwise looks good, JS is not my forte though.

Addressed feedback, ready for review again

Ok, I think this is ready.

I tested manually both in 11.x and this branch locally.
I tested block and view contextual links and tested with both mouse and keyboard. As far as I can tell both act exactly the same.

I searched for references to backbone, the only references remaining are for toolbar. There are no more references in contextual.

Javascript is not my forte as mentioned, but I am familiar with it, reviewing the code directly I see nothing that looks amiss.

As far as I can tell both larowlan and _nod have worked on this to a non trivial extent.

The only thing I didn't check that may be worth it is performance. I'm not super familiar with JS performance testing so I'll leave that up to a committer to determine if it's necessary.

_nod's comment in 56 make me think the change has been addressed as far as I can tell.

I added a CR too.

Committed d381523 and pushed to 11.x. Thanks!