Problem/Motivation

We have issues with JS errors caused by contextual module and quickedit module.

We seem to have some non-standard markup, missing wrappers/data attributes/classes and then the JS in those modules fails hard.

To reproduce, remove all attributes from nodes in bartik's node.html.twig:

diff --git a/core/themes/bartik/templates/node.html.twig b/core/themes/bartik/templates/node.html.twig
index 25144bf..5089563 100644
--- a/core/themes/bartik/templates/node.html.twig
+++ b/core/themes/bartik/templates/node.html.twig
@@ -72,7 +72,7 @@
     'clearfix',
   ]
 %}
-<article{{ attributes.addClass(classes) }}>
+<article>
   <header>
     {{ title_prefix }}
     {% if not page %}

Then clear caches and look at a node with quickedit/contextual links enabled.

As discussed on IRC, it is expected that it can't work properly.. but it should give me a helpful error in the console or ignore it.. but not break completely.

Proposed resolution

Fixing it seem to be relatively easy but I'm can't fully explain what markup exactly is causing it, at least not the contextual.js part. For quickedit, it seems to be a field that is not wrapped in an entity wrapper, which is the case for at least some block_content entities on my site, apparently.

Remaining tasks

User interface changes

API changes

Data model changes

Only local images are allowed.

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Status: Active » Needs review
FileSize
1017 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,550 pass(es). View
Wim Leers’s picture

For quickedit, it seems to be a field that is not wrapped in an entity wrapper, which is the case for at least some block_content entities on my site, apparently.

That could indeed explain it. But… isn't the real problem then that your site is apparently losing some attributes?

To be able to properly review this and get this patch or some improved version in, I really do need steps to reproduce.

(I wrote this comment two days ago, then forgot to post it, LOL.)

Berdir’s picture

Issue summary: View changes
Wim Leers’s picture

Title: Avoid JS errors in contextual.js and quickedit.js due to unexpected markup » contextual.js and quickedit.js should fail gracefully, with useful error messages, when Twig templates forget to print attributes
Issue tags: -Needs steps to reproduce +JavaScript, +DX (Developer Experience), +TX (Themer Experience)
Related issues: +#2033275: Contextual links broken because of JS error

Part of this patch is identical to #2033275: Contextual links broken because of JS error.

+1 for the issue summary.

Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Bug report » Task
Issue tags: +Needs manual testing, +Novice, +js-novice

Not an actual bug, but an improvement. So recategorizing as a task.

Also, 8.0.x is closed, so this can only go into 8.1.x and later.

Finally, this should be manually tested.

Berdir’s picture

Still running into this in pretty much every D8 project that I'm working on. Can't imagine that nobody else forgets to add those classes (or removes them on purpose) :)

dawehner’s picture

Issue tags: +JavaScriptTest, +Needs tests

I think they just disable quickedit and contextual links and call it a day :)

Anyway, we can add proper test coverage now.

Shaun Holt’s picture

What information to I need to supply to help fix this?

Drupal 8.1.6 core.

Latest update of Quick Edit and Contextual Links....

TypeError: $contextuals.eq(...).offset(...) is undefined
var firstTop = $contextuals.eq(0).offset().top;

context...v=8.1.6 (line 116, col 20)

ruthleopold’s picture

I tested the change and got the same error.

I tested the patch in comment 2 and found that it no longer applies. I rerolled it and no longer got errors. I have attached the patch.

ruthleopold’s picture

My attachment didn't work. Here's the patch.

ruthleopold’s picture

Okay, hopefully this time it will attach.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

danlinn’s picture

Here's an updated patch for 8.2.x:

Status: Needs review » Needs work

The last submitted patch, 18: js-errors-2551373-18.patch, failed testing.

danlinn’s picture

FileSize
1.05 KB

Better patch!

danlinn’s picture

Test this time?

Saphyel’s picture

Status: Needs work » Needs review

Let's try in this way!

blazey’s picture

Status: Needs review » Reviewed & tested by the community

We were experiencing the Cannot read property 'top' of undefined on one of our projects. It prevented the pencil icon from showing up. Patch from #21 fixed the problem.

andypost’s picture

Version: 8.2.x-dev » 8.4.x-dev
catch’s picture

Status: Reviewed & tested by the community » Needs work

This does the graceful failure, but not the error message - shouldn't there be a console.log or console.error in there?

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing

This happens quite a lot, and if it happens, then it happens always and possibly many times on a page then, seems like that would result in quite a lot of console messages.

Maybe if we'd have the infamous dev/prod switch, then we could only do it in case we're in dev mode, but not on production sites. It will only happen for users with edit permissions, but still.

Setting back to needs review for more thoughts. I think it has been tested manually but we possibly still need automatic tests, but not quite sure how to do that.

catch’s picture

For tests I guess we'd need a testing theme without the markup and a js test using that theme? Seems quite a lot of test setup for a two-liner to be honest.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

I'm not the one who suggested we need tests for this. Yeah, I agree that it would be a lot of work. Setting back to RTBC then to get a decision on test coverage and my argument against a console.log() error.

Another argument is that doesn't actually need to be a bug in custom templates. You might have special blocks, nodes or so very you need very specific markup and don't care about contextual/quickedit, nothing wrong with that? I wouldn't want to constantly get errors then because of that.

xjm’s picture

Priority: Normal » Major
Issue tags: -Needs tests

We agreed this is a major issue.

geerlingguy’s picture

This is definitely major; I know quite a few sites have contextual links disabled because nobody could figure out why javascript was broken for admins but not anonymous users, but disabling the module seemed to fix the problem. I didn't even see this issue until yesterday, and I finally had a light bulb moment. It seems a lot of themers will blindly nuke the attributes or manually set them in the template for nodes, and I think this is the third project now where I was about to disable Context/Quick Edit, but I finally had time to look around for where the crazy mysterious error was coming from:

drupal.js:67
TypeError: undefined is not an object (evaluating 'entityElement
      .get(0)
      .getAttribute')
geerlingguy’s picture

Agreed on RTBC—the default behavior should be 'don't break all JS'. It would be nice if there were some extra ability to debug why contextual links are missing in some places... but I think this should be included as-is to stop the bleeding and make it so themers stop hating on the Contextual/Quick Edit modules so much.

Just tested the patch on a current D8 site build and it takes care of the fatal error and allows the rest of the page JS to work for any user with access to contextual menus.

Edit: also, posted on the topic on my blog, since I want to make sure I remember how to fix the problem (instead of just avoid the JS error): Preserve the ability to Quick Edit nodes when theming node templates!.

droplet’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.91 KB

Put this at early stage to shut it down fast:

if(!$(entityElementSelector).length) {
 return;
}

Either this line or this is trying to find the [data-quickedit-entity-id]

Totally new changes, no interdiff.

Berdir’s picture

+++ b/core/modules/quickedit/js/quickedit.js
@@ -366,14 +366,22 @@
+
+    if (!$entityElement.length) {
+      throw Drupal.t('The field [data-quickedit-field-id="@fieldID"] parent attribute [data-quickedit-entity-id] is missing.', {
+        '@fieldID': fieldID
+      })
+    }
+

that's exactly what I argued above that we shouldn't do, because this might be a valid situation, maybe you have some special block or so where you don't want to have a wrapper or something.

Can't comment on the other changes.

droplet’s picture

I'm fine with no error messages. But we need to document it somewhere. With this issue thread, I still spend quite a lot of time to figure out what's happening.

A valid situation should be handled differently. Either override the JS function or provide a way to skip it [data-quickedit-skip-id]. The best way is dropping the [data-quickedit-field-id].

drpal’s picture

Fixed a missing semicolon from #32 and added some documentation about what's happening.

geerlingguy’s picture

Status: Needs review » Reviewed & tested by the community

I've been hammering this with a fairly complex theme with over 100 templates, and it's immensely helpful having this error message in the console logs to help debug where the errors are coming from—I've been using the patch since the version in #21, and now I'm using the latest version and it's well-documented, simple, and makes working with quickedit (especially) not mind-numbingly-annoying.

RTBC here.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

The bug fix itself looks good for me. I agree with #34 that we should throw an exception instead of silently failing since it can be helpful for someone trying to figure out why quickedit is not working for them.

However, I do feel like that it would make sense to create test coverage for this since this is one of those edge cases that is difficult to remember to test while making changes. Quickedit already has a quickedit_test module, maybe we could use that to override a template?

drpal’s picture

Status: Needs work » Needs review

@lauriii

I think this is a bit harder to test than it seems. Since we are throwing an actual error to the console we don't currently have a way capture that information and validate that the error was thrown. We could introduce a drupal.test.js file that clobbers some of the built in console methods and would allow us to see what's going on with the messages. However I'm not so sure this issue is the place to do it.