Problem:

When submitting a form via AJAX, such as in a CTools Modal, Drupal's AJAX script keeps a reference to the form around to be able to attach behaviors to it again after running all commands returned from the server. Problems arise when the form itself is removed by one of the commands. In this case, the command was modal_dismiss. The form is removed from the document, but it's not completely gone yet because Drupal still holds a reference to it. That reference is then passed on as Drupal.attachBehaviors(this.form, settings) (often with all of Drupal.settings since closing a modal generates no new settings command), tricking all module behaviors on the page into thinking the form elements are to be used. Modules attaching their behaviors must now check whether the context argument they're given is actually part of the document, and if any elements they may be looking for as outside that context can be reached at all.

Background:

This issue was found while debugging a reopened/repurposed issue in Wysiwyg module - see #1757684-25: Ajax error on Add another item (#25 and onwards).
I've posted a workaround patch for Wysiwyg in the previously mentioned issue, another for fixing this at the CTools level, but I would prefer if Core didn't try to attach behaviors to elements no longer part of the document in the first place. ;)

The steps I reproduced this with:

  1. Start with a Drupal installation having Wysiwyg, CTools, and Panels (enable Panel nodes and the In-Place Editor), and an editor library. I'll use CKEditor (3) here since it gives an easy to trace error.
  2. Assign an editor profile to any format, say Filtered HTML.
  3. Create a Panel page, any layout, make sure the In-Place Editor is enabled and view the node.
  4. Open the browser's JavaScript console
  5. Click "Customize this page" at the bottom of the window.
  6. Click the "+" icon to add new pane to any area
  7. Click "New custom content" and the WYSIWYG editor should appear.
  8. Type anything into the editor and click "Finish"
  9. An error is thrown by the editor after the modal closes: Uncaught TypeError: Cannot call method 'getEditor' of undefined
  10. Breaking on the error, walk up the stack until you get to ajax.js, line 410. This call gets passed a reference to the form which CTools just removed from the IPE modal as part of the modal_dismiss command just a few lines above.

Why the error?

  1. Modal opens -> Form is loaded -> behaviors get attached (using settings from the AJAX response) -> Wysiwyg finds a format selector and attaches the editor inside a .once('wysiwyg',...) call. Fine so far
  2. Form is submitted -> Behaviors are told to detach (serialize) -> Behaviors detach again (unload) -> Wysiwyg removes the editor and the 'wysiwyg-processed' class -> The modal and form are removed. Still doing fine.
  3. Drupal triggers attaching behaviors on the lingering form reference again (this time without any settings from the AJAX response, but they've been merged into Drupal.settings) -> Wysiwyg uses .once('wysiwyg', ...) and finds the same format selector, thanks to context -> Wysiwyg passes the id of the field associated with that format selector to the editor library -> The editor library attempts to locate the field by id -> OOOPS! Not so fine anymore...

Fixing the cause:

To prevent these problems, it would be nice if Core checked that the form it's about to attach behaviors to is actually part of the document first. This is quite easy to do and should have no side-effects. Any behavior which previously didn't throw errors would still have had all their hard work undone as soon as the form reference went out of scope anyway.

Note: I've not looked into D8 or D6 yet.

Files: 

Comments

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +JavaScript, +needs backport to D7
FileSize
1.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,230 pass(es). View

I can reproduce this, it is contributing to the problem described at #1401038: Wysiwyg not showing, Node Form in a Ctools Modal.

Here is a reroll for D8.

tim.plunkett’s picture

Status: Needs review » Needs work

Hmm, I recently had this cause problems on Firefox.

TypeError: Value does not implement interface Node.

This comes from jQuery.contains, and it seems that FF stricly enforces that it must be a DOM object.

$.contains(document, this.form[0]) works fine in FF...

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,501 pass(es), 2 fail(s), and 0 exception(s). View
1.23 KB

Actually, that makes some sense.
jQuery.contains() expects two DOM elements.
http://api.jquery.com/jQuery.contains/ is VERY explicit about the first argument being a DOM element, but it does say the second should be as well.

It seems that Chrome is more liberal with this, but FF is being strict.

This code has changed from D7 to D8, it's now this.$form to clarify that its a jQuery object, and the following lines in the attachBehaviors call use this.$form.get(0), which is exactly what we need here.

Not as sure what to do with D7, since I think the param passed to attachBehaviors is technically also wrong...!

Status: Needs review » Needs work

The last submitted patch, 3: ajax-form-2215857-3.patch, failed testing.

gmercer’s picture

We are re-rolling the patch in #3 to be up to date with 7.39 of core.

gmercer’s picture

gmercer’s picture

Sorry got that last patch wrong. Try try again.

gmercer’s picture

Wow. Had a crazy interaction with another patch we are using. Sigh. So sorry about all the noise.

olli’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2215857-9.patch. Unable to apply patch. See the log in the details link for more information. View

Here is a reroll for 8.0.x-dev.

olli’s picture

andypost’s picture

Issue tags: +Needs manual testing

mgifford queued 9: 2215857-9.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2215857-9.patch, failed testing.

marabak’s picture

FileSize
1.19 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View

i have rerolled de patch #9 against 8.0.x

marabak’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: 2215857-14.patch, failed testing.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

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

cferthorney’s picture

See attached rerolled patch from #14. This was rerolled against latest 8.2.x, please let me know if 8.3.x would be more suitable.

cferthorney’s picture

Status: Needs work » Needs review

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.

stella’s picture

Tested it against 8.2.x. It applied cleanly and fixed my problem with #2493945: Views filters dragging in the grouping view is broken

stella’s picture

Status: Needs review » Needs work

Actually there's still something a bit wonky with views grouping.

Base scenario: have views exposed filters. I want some of them AND-ed and some OR-ed.

Scenario 1 (works):

  1. Add all the filters to the view.
  2. Click on 'rearrange filters' button
  3. Add a new filter group
  4. Move some of the filters to the new group, put an 'OR' on the second group
  5. Save
  6. Everything is saved as expected.

Scenario 2:

  1. Add all the filters to the view.
  2. Click on 'rearrange filters' button
  3. Add 2 new filter groups
  4. Move the filters to the two new groups - I have an OR on the 2nd group, AND on the 3rd (shouldn't matter)
  5. Save
  6. Some of the filters are moved, some refuse to move.

FYI, I had 6 filters in total - 2 fixed ones in the first group, and 2 exposed filters in each of the other 2 groups. Each group was 'AND'-ed together, but within the group the fields were not necessarily.

Adding the groups one at a time, saving and then repeating, didn't seem to have any effect.

droplet’s picture

Issue tags: +Needs JS testing
michielnugter’s picture

I updated the patch to fix the issue with views, there was another check requiring the form-is-in-dom validation.

Also, I posted a work-in-progress test in #2493945: Views filters dragging in the grouping view is broken which I propose to move here (see attachments). It uses the views filters dragging to validate the patch is working. I marked the other issue as a duplicate of this one as this fix (after my addition) also fixes that issue.

Note: this patch requires the fix in #2717629: Add filter group for a View fails to pass.

I'm not sure with the test yet. I'd like other opinions (@droplet?) on how to proceed:

- The incorrect behavior is only reproducable in specific places in Drupal core. Is writing a test (like the current one using views filter dragging) based on a specific case ok?
- It's a problem with ajax.js which currently does not have any functional JavaScript testing (right?). It's ideal to have it and include coverage for this patch. This however is quite a large untertaking and will force this patch to wait for a long time while it's nice to have it in. Maybe accept it for now (and clean up the test) and open a new issue for the ajax coverage?

stella’s picture

There's still something awry with the views filter dragging between groups. It's still not behaving as expected with some filters movement not being saved. I haven't figured out a pattern yet, but might be something to do with their index within the group. For example I couldn't add a 2nd filter to a filter group at the bottom, but adding it to the top of the group worked.

stella’s picture

There's now also something else weird happening when I add 2 empty filter groups to views. The first gets added, but adding the 2nd one either causes the page to reload with my first new group removed, or reloads with no modal dialog open at all.

I also can't add any new groups (brand new clean view to be sure) and have had to revert back to the patch from #18 in order to get any groups.

stella’s picture

Ok I had an out of date patch for #2717629: Add filter group for a View fails. Updating that fixed the weirdness I was experiencing with the #24 patch. However, still experiencing the issue from #25.

My scenario: 5 filters (2 fixed, 3 exposed)

steps:

  1. Add 2 new filter groups.
  2. Leave the 2 fixed filters (filter #1 and #2) in group #1.
  3. Move filter #3 to group #2 (this is an OR)
  4. Move filters #4 and #5 to group #3

Result: everything is ok except for filter #5 which stays in group #1.

Caveat: if I move filter #5 to the end of group #3 it doesn't work as above, but if I move it to the top of the group, it stays in group #3. Then after a save, I'm able to move it to the desired position at the end of the group.

It's much better than without the patch, and is usable with some workarounds, but not 100% there yet.

michielnugter’s picture

As discussed on IRC: the views issue is split into a seperate issue as it's not directly related to this issue.

#2825446: Moving a filter to the end of a Filter group in filter rearrange doesn't work

slasher13’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
michielnugter’s picture

@slasher13 The included test is build based on the test in #2717629: Add filter group for a View fails and includes the test for that issue and will therefore fail here. Not sure how to proceed, I asked droplet on IRC to check this issue to see what the best approach is for getting this fix committed including the test.

michielnugter’s picture

Update for the patch as #2717629: Add filter group for a View fails has been committed.

The test should pass now.

Lendude’s picture

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
@@ -66,6 +66,20 @@ public function testFilterCriteriaDialog() {
+    // Validate dragging to an invalid location doesn't work.

Are we dragging to an invalid location?

Which of the three changes to ajax.js is covered by this test? Probably not all three right?

michielnugter’s picture

Thanks for the review.

I'll update the test with correct comments. Good point about the coverage. I checked which changes are tested and only 2 of the 3 changes are excuted (I placed breakpoints to verify).

I'm having some trouble with the third, this is a check in the error callback, I haven't figured out a way to make a request fail consistently yet.

michielnugter’s picture

I've been trying for a while now to trigger the error form the UI but no such luck.

If a complete coverage is to be attained I think a completely separate test for ajax.js with a test-module which contains all the required callbacks and forms is required. This however, is quite a large undertaking and maybe out of scope for this issue.

I'm very curious to see other opinions on this.

Patch contains a comment fix and incorrect indent fix.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

ericmulder1980’s picture

Patch no longer applies to 8.2.x (after 8.2.7. security update) and i need this for my composer workflow.

Status: Needs review » Needs work

The last submitted patch, 37: 2215857-Behaviors_get_attached_to_removed_forms-36.patch, failed testing.