Problem/Motivation

It looks like #3203920: Replace Contextual Links BackboneJS usage with VanillaJS equivalent caused a regression in specific scenarios.

After updating from D10.4 to 11.2, I see the following JS error:

Uncaught TypeError: Cannot set properties of undefined (setting 'textContent')
    at ContextualModelView.render (contextual.js?v=11.2.2:356:29)
    at new ContextualModelView (contextual.js?v=11.2.2:331:14)
    at initContextual (contextual.js?v=11.2.2:113:33)
    at contextual.js?v=11.2.2:167:13

This seems to happen when some HTML that contains contextual links are being relocated in JS.
On my project, it happens with mmenu JS external lib which builds the mobile menu.
It clones a ul (which contains blocks inside) and moves it at the bottom of the HTML, right before the closing body tag.
I did manage to reproduce with olivero, see below.

Steps to reproduce

- Install standard profile on 11.x branch
- Add the following behavior, anywhere

Drupal.behaviors.contextual_bug = {
  attach(context) {
    once('contextual_bug', '#block-olivero-views-block-who-s-online-who-s-online-block .block__content', context).forEach((element) => {
      // Move the who's online block content at the bottom, before body closing tag, replicating mmenu.
      const clone = element.cloneNode(true);
      document.body.appendChild(clone);
      element.remove();
    });
  }
}

(I place it in core/modules/contextual/js/contextual.js:537 at the bottom, but in the IIFE)
- Login as uid1, add the "Who's online" block in the content region
Note: why that block? because it seems there is a relation with big_pipe replacement and the block contains a .contextual-region element within it's content, both are required to reproduce the bug.
- Clear cache, hit the homepage (still logged in)
>>> The JS error is thrown

Proposed resolution

The code at core/modules/contextual/js/contextual.js:167 is attempting to get data-contextual-id elements that are not guaranteed to be there, in case these were relocated.

window.setTimeout(() => {
  initContextual(
    $context
      .find(`[data-contextual-id="${contextualID.id}"]:empty`)
      .eq(0),
    html,
  );
});

This line hasn't changed in #3203920 but the same happens before that.
The main difference is that all jQuery.each and selectors were silently skipping in initContextual and ContextualModelView before. But now, ContextualModelView::render was changed and hard fails on trigger.textContent assignment.
I suggest to add some defensive checks in a few places.

Remaining tasks

N/A

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3535051

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:

Comments

herved created an issue. See original summary.

herved’s picture

Issue summary: View changes
Issue tags: +JavaScript, +Contextual links, +Regression
herved’s picture

herved’s picture

Issue summary: View changes
Issue tags: -JavaScript +JavaScript
herved’s picture

Issue tags: -JavaScript +JavaScript

-- comment moved to IS --

herved’s picture

Version: 11.2.x-dev » 11.x-dev

herved’s picture

Issue summary: View changes
Status: Active » Needs review
herved’s picture

StatusFileSize
new2.07 KB

Static patch for composer

murphyw’s picture

StatusFileSize
new1.96 KB

My Drupal version is 11.2.0

himanshu raj’s picture

StatusFileSize
new204.61 KB
new94.21 KB
new35.84 KB

I was able to reproduce the issue by following the same steps as mentioned above:

1. Added the below JS snippet manually in the contextual.js file to simulate block movement:

Drupal.behaviors.contextual_bug = {
  attach(context) {
    once('contextual_bug', '#block-olivero-views-block-who-s-online-who-s-online-block .block__content', context).forEach((element) => {
      // Move the who's online block content at the bottom, before body closing tag, replicating mmenu.
      const clone = element.cloneNode(true);
      document.body.appendChild(clone);
      element.remove();
    });
  }
}

2. Placed the Who's online block in the Content region.
3. Reloaded the page.
At this point, I saw the following error in the browser console:

Uncaught TypeError: Cannot set properties of undefined (setting 'textContent')

Then I applied the patch from #10 and run composer install.
After applying the patch:

The error disappeared from the console.

###Patch / MR tested
Applied patch from #10

###Results:

JS console error disappeared after applying the patch.

### Environment
Drupal 11.2.x, PHP 8.3, MySQL 8.0, DDEV 1.25 (Docker + WSL2).

larowlan’s picture

Status: Needs review » Needs work
Issue tags: -Contextual links, -Regression

There are multiple places we call initContextual, we've updated one of those calls to check the element exists, but not the other.
But we've also updated the logic inside initContextual - I don't think we need both.
I'm inclined to make it check it from the outside rather than inside

Let's do that - thanks!

herved’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Can we get a test case showing the bug please.

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.

herved’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

MR rebased, test coverage added.

herved’s picture

Status: Needs review » Needs work
herved’s picture

Status: Needs work » Needs review

Guard restored and test case added.
Probing this issue with LLM assistance revealed three more regressions from the backbone removal, plus one older gap (all out of scope here, this fix is needed regardless).

LLM report below (fwiw, I have not verified myself):

The crash fixed here is one of four regressions from #3203920: Replace Contextual Links BackboneJS usage with VanillaJS equivalent: render() dereferences the trigger without checking, where the old AuralView.js used $trigger.each(), a no-op on an empty set. The other three follow, then one older gap.

1. The .trigger touchend handler calls touchEndToClick() with no argument, and the method reads event.preventDefault(). Firing touchend on a trigger throws TypeError: Cannot read properties of undefined (reading 'preventDefault'). The old VisualView.js bound the same handler to both touchend selectors, so both received the event.

2. contextualToolbarModelView.js reads Drupal.contextual.instances.count in four places. instances is an array, so .count is undefined. The old StateModel.js used contextualCollection.length.

3. updateVisibility() calls this.get('contextualCount'). The class has a contextualCount getter, not a get() method. this.get() was Backbone Model API. Since .count is always undefined, contextualCount never changes and updateVisibility() never runs.

4. Drupal.contextual.instances is only pushed to, never removed. The old Backbone collection was never removed from either, so this one predates the conversion.

The .eq(0) that produces the empty set predates the conversion too. The guard here is needed regardless of 1 to 4.

quietone’s picture

Title: [regression since 11.2] Uncaught TypeError: Cannot set properties of undefined (setting 'textContent') at ContextualModelView.render (contextual.js?v=11.2.2:356:29) » [regression] Uncaught TypeError: Cannot set properties of undefined (setting 'textContent') at ContextualModelView.render (contextual.js?v=11.2.2:356:29)
Issue tags: +Needs title update

The title should be a description of what is being fixed or improved. The title is used as the git commit message so it should be meaningful and concise. See List of issue fields. Tagging for a title update

herved’s picture

Title: [regression] Uncaught TypeError: Cannot set properties of undefined (setting 'textContent') at ContextualModelView.render (contextual.js?v=11.2.2:356:29) » [regression] JavaScript TypeError when a contextual link placeholder is moved or emptied
Issue tags: -Needs title update