Contextual links broken because of JS error

It's a simple patch to correct a logic error in an if statement. I just ran into this bug a moment. The patch resolves it.

Doesn't this suggest that contextual.js is being loaded on a page with no contextual links? Is this an outcome of a bigger problem?

+++ b/core/modules/contextual/contextual.jsundefined
@@ -78,7 +78,7 @@ function adjustIfNestedAndOverlapping ($contextual) {
-  if ($contextuals.length === 1) {

Admittedly this does seem brittle...

If you look at where this has been called... initContextual adds the class contextual so I have no clue how $contextuals.length is less than 1

Can we get some instruction to recreate?

I managed to recreate by doing this...

• Install standard profile
• Create an article - promoted to frontpage
• Visit frontpage - can't contextual links

So the commit that introduced the issue was 5aced02... ie. #2004286: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class

#2/#3: initContextual() should not be called unless there actually *is* a contextual link, so that's no logic error, that's intentional strictness.

This happens *only* on pages with Views, where views-contextual.js is not being loaded when it should be. This does not happen on e.g. node/1.
The fact that a JS error occurs because views-contextual.js has not been loaded while it should be, is a good thing IMO.

Root cause

Debugging this indeed led to 5aced02 i.e. #2004286: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class being the cause. More specifically, as of that commit, views_preprocess_html()'s logic that adds views-contextual.js is no longer fired, because $variables['attributes']['class'] no longer includes the contextual-region class.

So this is in fact a whole cascade of things. The fact that the JS error was never noticed in #2004286 is rather puzzling, because it happens on the front page, even without creating any content. It suggests no or far too superficial manual testing was done for that patch.

Solution

I tried to find a solution.

When views_preprocess_html() gets fired, then #2004286 causes $variables['page'] to already be HTML rather than a render element, hence preventing the dynamic changing of the class attribute (because it checks for if (!empty($variables['page']['#views_contextual_links'])) {, which can't work if $variables['page'] = '<div>html here</div>'). If I change it to views_preprocess_page(), then it is able to properly detect the presence of Views' contextual links, but the changes are ignored, they never make it into the final HTML.

I tried to devise work-arounds using hook_page_build() and hook_page_alter(), but failed.

AFAICT there is no clear solution. #2004286 introduced this mess, along with an always-visible messages area: #2004286-76: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class. I think #2004286 should simply be reverted. Before it can be committed, it should first fix the problems it causes elsewhere.

Alex rolled back the other patch, closing this one.

I noticed this problem in several d8 projects and the patch would fix this error :/

Thanks to @nod_ for this patch - Updated the patch to be working with release 8.0.1.

Great!

@yannickoo, @pmchristensen can we get new steps to reproduce and find out why - as @Wim Leers has said in #6 this error is a sign of something else being wrong.

Yeah, this is minor at best, because nobody seems to encountering it. We need proper steps to reproduce. And the issue summary is outdated since this was repurposed in #8.

We now have JavaScriptTestBase, so now it should be possible to write a test that fails.

Even though it's older, I'm betting this is a duplicate of #2551373: contextual.js and quickedit.js should fail gracefully, with useful error messages, when Twig templates forget to print attributes

Thank you for the hint @jcnventura! It's funny to compare the patches, the only difference between them:

+
+    // Prevent errors if the necessary entity markup is missing.
+    if (entityElement.length === 0) {
+      return;
+    }
+

How should we proceed with this issue now? Mark this one as duplicated and try to get the other pach from #2551373: contextual.js and quickedit.js should fail gracefully, with useful error messages, when Twig templates forget to print attributes in?

Path "2771361-7-do-not-attach-ajax-to-non-present-views.patch" fix my issue.
When will be merged please?

#2551373: contextual.js and quickedit.js should fail gracefully, with useful error messages, when Twig templates forget to print attributes was committed now, I'm not sure if this isn't just a duplicate of that issue at this point.

Try to see if latest 8.4.x fixes your problem as well.

Agreed with Berdir!

Thanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.

As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)".

Since there was no additional information provided since the issue was postponed or #2551373: contextual.js and quickedit.js should fail gracefully, with useful error messages, when Twig templates forget to print attributes was suggested as duplicate, I'm marking the issue "Closed (duplicate)". If anyone can provide information on why this issue is different, please add complete steps to reproduce the issue (starting from "Install Drupal core"), document those steps in the issue summary and set the issue status back to "Active".

Thanks!