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.

CommentFileSizeAuthor
#60 2551373-60.patch4.15 KBdrpal
#60 interdiff-2551373-59-60.txt1.08 KBdrpal
#59 2551373-59.patch4.31 KBdrpal
#59 interdiff-2551373-57-59.txt2.75 KBdrpal
#57 2551373-57.patch3.4 KBdrpal
#57 interdiff-2551373-55-57.txt2.25 KBdrpal
#55 2551373-55.patch3.42 KBdrpal
#55 interdiff-2551373-51-55.txt2.29 KBdrpal
#52 interdiff-2551373-46-51.txt5.08 KBdrpal
#51 2551373-51.patch3.44 KBdrpal
#51 interdiff-2551373-46-51.patch5.08 KBdrpal
#50 quickedit_broken_node_template_after.png155.35 KBWim Leers
#50 quickedit_broken_node_template_before.png206.72 KBWim Leers
#48 after_applied_patch.png72.86 KBprajaankit
#48 before_appled_patch.png70.59 KBprajaankit
#46 2551373-46.patch3.78 KBdrpal
#46 interdiff-2551373-35-46.txt5.09 KBdrpal
#35 interdiff.txt767 bytesdrpal
#35 2551373-35.patch2.05 KBdrpal
#32 2551373-32.patch1.91 KBdroplet
#21 js-errors-2551373-21.patch1.05 KBdanlinn
#20 js-errors-2551373-20.patch1.05 KBdanlinn
#18 js-errors-2551373-18.patch1.11 KBdanlinn
#16 js-errors-2551373-16.patch1.05 KBruthleopold
Screen Shot 2016-07-24 at 12.18.14 PM.png40.01 KBruthleopold
js-errors-2551373-11.patch1.05 KBruthleopold
#2 js-errors-2551373-2.patch1017 bytesBerdir
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,550 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

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.

droplet’s picture

Status: Needs review » Needs work
Issue tags: -Novice, -js-novice

Add a test has multiple Quickedit (e.g., frontpage with 2 node), and remove the required markup in either one. Then, access if another one can edit quickedit or not.

drpal’s picture

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

@droplet

I don't quite understand what you mean.

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.

I couldn't find any way to actually capture that error from the browser console. Additionally, since this doesn't actually throw an error to the screen (no JS messages yet), it doesn't feel like something that should be wrapped up with a functional test. This is more in the realm of unit tests since this doesn't really effect the user.

droplet’s picture

Issue tags: +Needs tests, +JavaScriptTest

@drpal,

contextual.js and quickedit.js should fail gracefully

We still need a test for it. We don't terminate whole quickedit or contextual script to run.

I couldn't find any way to actually capture that error from the browser console.

We could do it later. Theoretically, a custom `window.onerror` will work. set up it before this test to run and access the Var again after..etc

droplet’s picture

Actually, this is a bug? nope?

(Needs rerolls)

droplet’s picture

Furthermore,

I thought the best deal will push the error log into `Drupal.contextual`

`Drupal.contextual.logger = () => { //... }`

then, we able to mock it with our custom function in tests. It also able to extend with 3rd parties loggers. Our code will be neater :)

DuneBL’s picture

Patch #35 failed on 8.4

patching file core/modules/contextual/js/contextual.js
Hunk #1 FAILED at 108.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/contextual/js/contextual.js.rej
patching file core/modules/quickedit/js/quickedit.js
Hunk #1 FAILED at 366.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/quickedit/js/quickedit.js.rej
drpal’s picture

Assigned: Unassigned » drpal

I'll reroll this.

From #41

We could do it later. Theoretically, a custom `window.onerror` will work. set up it before this test to run and access the Var again after..etc

Lets perhaps open a followup to develop a window mock that could be included in test environments that would allow us to capture things like this.

drpal’s picture

Assigned: drpal » Unassigned
FileSize
5.09 KB
3.78 KB

Rerolled.

DuneBL’s picture

@drpal: many thanks, patch apply cleanly!!

prajaankit’s picture

Hi drpal,

I have the same problem , i applied your patch, its give new error in console.

please see in attached image

prajaankit’s picture

I don't Know i am doing Something wrong.

I solved this issue by

  if (typeof entityElement.get(0) === 'undefined') {
      return;
    }

.

In Quickedit.js

Thanks

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
206.72 KB
155.35 KB

It's unfortunate that this doesn't add tests, but we've been blocking this patch on test coverage for almost 2 years now. @drpal explained in #40 why test coverage is not worth it. I'm not sure I fully agree, but I do know this has been bothering many many sites over the years. Let's be pragmatic.

Before
After
+++ b/core/modules/quickedit/js/quickedit.es6.js
@@ -366,14 +366,24 @@
+      throw Drupal.t('The field [data-quickedit-field-id="@fieldID"] parent attribute [data-quickedit-entity-id] is missing.', {

This should not be translated. Use Drupal.formatString() instead.

More importantly, the message doesn't really make sense: it's A) inaccurate, B) unhelpful. It should be accurate and point to a solution.

I suggest:
Drupal.formatString('Quick Edit could not associate the rendered entity field markup (with [data-quickedit-field-id="@fieldID"]) with the corresponding rendered entity markup: no parent DOM node found with [data-quickedit-entity-id="@entityID"]. This is typically caused by the theme's template for this entity type forgetting to print the attributes.

Besides this, the patch also no longer applies.

drpal’s picture

- Reroll 👍
- Adjust the message.

About the tests. I think this would be a great place for a unit test. This isn't really a "user feature". Without some wrangling of our existing JavaScript by overriding the browser .log method we can't actually capture the message; that's something we should figure out in a followup, not in this issue.

drpal’s picture

FileSize
5.08 KB

It's early.

The last submitted patch, 51: interdiff-2551373-46-51.patch, failed testing. View results

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/quickedit/js/quickedit.es6.js
@@ -359,7 +359,15 @@
+      throw Drupal.formatString(`Quick Edit could not associate the rendered entity field markup (with [data-quickedit-field-id="${fieldID}"]) with the corresponding rendered entity markup: no parent DOM node found with [data-quickedit-entity-id="${entityID}"]. This is typically caused by the theme's template for this entity type forgetting to print the attributes.`);

If this is going to use a JS template literal, then there's no point in using Drupal.format().

I don't have a strong opinion about which is appropriate to use. I'll let you decide, you're a JS maintainer :)

drpal’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
3.42 KB

- Throw an error, no use of Drupal.format()

Status: Needs review » Needs work

The last submitted patch, 55: 2551373-55.patch, failed testing. View results

drpal’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
3.4 KB

- throw a string. The only issue with this is that our eslint suggest that we don't throw literals.

Status: Needs review » Needs work

The last submitted patch, 57: 2551373-57.patch, failed testing. View results

drpal’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
4.31 KB

My mistake here. Between #46 and #51 things got broken.

drpal’s picture

Wim Leers’s picture

+++ b/core/modules/quickedit/js/quickedit.es6.js
@@ -359,14 +359,22 @@
-    let entityElement = $(fieldElement).closest(entityElementSelector);
+    const $entityElement = $(entityElementSelector);
...
+    let entityElement = $(fieldElement).closest($entityElement);

I wanted to RTBC, but I don't understand this anymore. I'd swear it used to make sense, but not anymore, sorry :(

Why do we have that second let entityElement = … at all?

This means that we first use a selector on the entire page, and decide to throw an error or not based on that.

But then we don't use the result of that, and do another DOM query instead… which means it can still result in null. Which means this can still fail.

Why aren't we keeping the original line, and checking if that is empty?

I have the feeling I'm asking something stupid, but still, I'd rather ask it now, before commit.

drpal’s picture

@Wim Leers

Looks like there's some weird naming going on here. We have $entityElement and entityElement.

Initially we check to see if there are any elements on the page that result from this selector, `[data-quickedit-entity-id="${entityID}"]`. That's where we throw the new error.

After that, we create let entityElement = $(fieldElement).closest($entityElement); and check to see if that returns anything. If none are returned we do some scope expansion looking for other elements, hence the let for entityElement.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I carefully re-read the code, read #62 and re-read the patch.

I see my mistake now. The selector and if-test that we use to decide to throw an error is what is able to guarantee that either of the two branches of code that follow it, will succeed. This makes sense again :)

(It's been a while since I looked at Quick Edit code, sorry for the hiccup!)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 60: 2551373-60.patch, failed testing. View results

andypost’s picture

lauriii’s picture

Issue tags: +Needs followup

Could someone open the follow-up for #51?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drpal’s picture

Wim Leers’s picture

  • larowlan committed 50a541b on 8.5.x
    Issue #2551373 by drpal, danlinn, ruthleopold, Berdir, droplet, Wim...

  • larowlan committed e1b5c64 on 8.4.x
    Issue #2551373 by drpal, danlinn, ruthleopold, Berdir, droplet, Wim...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 50a541b and pushed to 8.5.x.

Cherry-picked as e1b5c64 and pushed to 8.4.x.

I've spent hours trying to find templates that didn't output attributes....so looking forward to this one being in.

Wim Leers’s picture

OMG YAY!

#72: You'll still need to spend those hours hunting down those poorly written templates if you want Contextual Links & Quick Edit to work. But at least this change means all other JS can continue to work just fine, and you get helpful pointers in your browser console!

larowlan’s picture

Yeah, no blame on core at all here.

But at least this change means all other JS can continue to work just fine

yeah, that alone makes this fix worth its weight in gold.

andypost’s picture

Great, now it's possible to catch what's wrong with contextual!

larowlan’s picture