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
| Comment | File | Size | Author |
|---|
Issue fork drupal-3535051
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
Comment #2
herved commentedComment #3
herved commentedComment #4
herved commentedComment #5
herved commented-- comment moved to IS --
Comment #6
herved commentedComment #8
herved commentedComment #9
herved commentedStatic patch for composer
Comment #10
murphyw commentedMy Drupal version is 11.2.0
Comment #11
himanshu rajI 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:
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).
Comment #12
larowlanThere 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!
Comment #13
herved commentedComment #14
smustgrave commentedCan we get a test case showing the bug please.
Comment #16
herved commentedMR rebased, test coverage added.
Comment #17
herved commentedoops I think I removed I guard I shouldn't have https://git.drupalcode.org/project/drupal/-/merge_requests/12677/diffs?c...
Comment #18
herved commentedGuard 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):
Comment #19
quietone commentedThe 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
Comment #20
herved commented