Problem/Motivation

If you use hook_contextual_links_view_alter to make a link use ajax via a modal it does not work.

This is because Drupal.behaviors.AJAX.attach is fired before the contextual links are dynamically rendered to the page.

I found this in #2729413: Open "Configure Blocks" contextual links in a Modal with a simplified (quick edit) form
That issue contains a small fix to block.js but is probably not the right place for it as it would only solve it for blocks.

This problem will probably also affect the new issue #2762505: Introduce "outside in" quick edit pattern to core which introduces offcanvas tray pattern that will use contextual links and ajax.

Proposed resolution

Abstract the code to a helper function
Call it again after contextual links are attached.

Remaining tasks

N/A

User interface changes

N/A, except things work now

API changes

N/A

Data model changes

N/A

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tedbow created an issue. See original summary.

tedbow’s picture

FileSize
1.88 KB

Here is patch that simply adds a test module that provides a contextual link that also uses Ajax. The link will not actually use Ajax for now because of the bug.

Will work on patch that fixes it.

tedbow’s picture

Status: Active » Needs review
FileSize
4.78 KB

Ok here is a patch that actually fixes the problem.

tedbow’s picture

+++ b/core/modules/contextual/tests/modules/contextual_ajax_test/contextual_ajax_test.module
@@ -0,0 +1,21 @@
+  drupal_set_message('fffff');

Whoops!

Status: Needs review » Needs work

The last submitted patch, 3: 2764931-3.patch, failed testing.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

nod_’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript
FileSize
3.79 KB
2.37 KB

Improved fix. Haven't tested but works in theory.

tedbow’s picture

@nod_ nice solution! I guess this could also solve other problems where contextual links don't work with other behaviors too!

Right now there is test module but no test that actually uses it. I manually enabled and tested with the test module.

The probably is there is no JS test for contextual links(even with ajax. I tried before and didn't have success writing a test.

Do we write the contextual links tests for basic links and ajax in this issue?

nod_’s picture

Guess we should, but I just want that to get in so that the offcanvas don't duplicate code from ajax.js.

tedbow’s picture

Status: Needs review » Postponed
Related issues: +#2821724: Create Javascript Tests for Contextual Links

I created #2821724: Create Javascript Tests for Contextual Links
There is patch that tests contextual links. After that is committed it should be easy to adapt the test to also test ajax.

Postponing so we get that done first.

Wim Leers’s picture

I'd argue the problem is in outside_in.js. This code is problematic:

$(document).on('drupalContextualLinkAdded', function (event, data) {
    // Bind Ajax behaviors to all items showing the class.
    // @todo Fix contextual links to work with use-ajax links in
    //    https://www.drupal.org/node/2764931.
    Drupal.attachBehaviors(data.$el[0]);

    // Bind a listener to all 'Quick edit' links for blocks
    // Click "Edit" button in toolbar to force Contextual Edit which starts
    // Settings Tray edit mode also.
    data.$el.find(blockConfigureSelector)
      .on('click.outsidein', function () {
        if (!isInEditMode()) {
          $(toggleEditSelector).trigger('click.outsidein');
        }
      });
  });

That code should look at how Quick Edit does it: core/modules/quickedit/js/views/ContextualLinkView.js etc.

That intercepts the click event on the "Quick Edit" contextual link and then does whatever it wants. For example, in the case of Outside In: open a modal dialog. At some point, Quick Edit even had dynamic link text, like "Stop quick edit" after Quick Editing was started.

The HTML inside links in Contextual Links are not like the rest of the page.

So I'm not convinced this change is actually desirable.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

tedbow’s picture

Status: Postponed » Needs review
Issue tags: +Needs tests
FileSize
12.22 KB

@Wim Leers thanks for review. I lost track of this issue.

This is patch is starting from scratch but more similar to the approach in #3 then #7

The HTML inside links in Contextual Links are not like the rest of the page.

So I'm not convinced this change is actually desirable.

I trust you know the Contextual module much better than. So I think this from #9

</code> 


<code>
+++ b/core/modules/contextual/js/contextual.js
@@ -209,6 +209,12 @@
+      // Let other JavaScript react to the adding of a new contextual link.
+      $(document).once('contextual-ajax').on('drupalContextualLinkAdded', function (event, data) {
+        Drupal.attachBehaviors(data.$el[0]);
+      });

So I this from #9 is actually a bigger change than is actually needed to solve the problem of Contextual links not working with Ajax. This is basically also how Settings Tray is solving the problem.

As I said in #10

@nod_ nice solution! I guess this could also solve other problems where contextual links don't work with other behaviors too!

Of course now that I think about solving other problem also might introduce other problems.

If there is code using the attach behaviours functionality that does not get invoked currently on Contextual links that code would all of sudden get invoked on contextual link if we went with the solution in #7.
This could definitely cause unforeseen problem. With the lack of functional Javascript testing core I could see problems not being caught in the current tests. And of course this could also be considered a BC break.

So this patch explicitly only bind ajax links when contextual links added.

Since for links to work ajax you have explicitly add the use-ajax class then I would say would not accidentally cause any links to all of sudden be ajax links that should not be.

There was other logic outside_in.es6.js that had to be changed because it was relying on behaviours being attached to contextual links.
I have consulted the logic into new function prepareAjaxLinks().
It needs to be called twice:
on attach because it needs to work with regular links. This need will be removed in #2844261: Allow dialog links to specify a renderer in addition to a dialog type
Then on the drupalContextualLinkAdded event. This need will be remain but the logic prepareAjaxLinks() will be simpler after #2844261

tedbow’s picture

Assigned: tedbow » Unassigned
Wim Leers’s picture

Component: contextual.module » ajax system

I think I like this better. Will need review from a JS maintainer, not from me though. This is now firmly in the land of ajax system, not contextual.js.

P.S.: this reminds me of the fact that all Drupal behaviors are attached in alphabetical order — rather than in asset library dependency order. There's an issue for that somewhere.

drpal’s picture

Assigned: Unassigned » drpal
drpal’s picture

Assigned: drpal » Unassigned
Status: Needs review » Needs work
  1. +++ b/core/misc/ajax.es6.js
    @@ -268,6 +250,33 @@
    +   * @param $element
    

    Missing `type` in JSDoc.

  2. +++ b/core/misc/ajax.es6.js
    @@ -268,6 +250,33 @@
    +    $element.find('.use-ajax').once('ajax').each(function () {
    

    Arrow function.

  3. +++ b/core/misc/ajax.es6.js
    @@ -268,6 +250,33 @@
    +      const element_settings = {};
    

    Camel case

  4. +++ b/core/misc/ajax.es6.js
    @@ -268,6 +250,33 @@
    +      element_settings.progress = { type: 'throbber' };
    

    Set this when you create the object.

  5. +++ b/core/misc/ajax.es6.js
    @@ -268,6 +250,33 @@
    +      // For anchor tags, these will go to the target of the anchor rather
    

    Use `/** ... */` for multiline comments.

  6. +++ b/core/misc/ajax.es6.js
    @@ -268,6 +250,33 @@
    +      const href = $(this).attr('href');
    

    You'll end up getting rid of `$(this)` with the arrow function. Don't wrap it every time you need to use it.

  7. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -11,6 +11,41 @@
    +      .filter((instance) => {
    +        const hasElement = instance && !!instance.element;
    +        let rendererOffCanvas = false;
    +        let wrapperOffCanvas = false;
    +        if (hasElement) {
    +          rendererOffCanvas = $(instance.element).attr('data-dialog-renderer') === 'off_canvas';
    +          wrapperOffCanvas = instance.options.url.indexOf('drupal_dialog_off_canvas') === -1;
    +        }
    +        return hasElement && rendererOffCanvas && wrapperOffCanvas;
    +      })
    

    Don't we get rid of this logic in #2844261: Allow dialog links to specify a renderer in addition to a dialog type?

  8. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -11,6 +11,41 @@
    +        const hasElement = instance && !!instance.element;
    

    This is confusing. Why would we not have an `instance`?

  9. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -11,6 +11,41 @@
    +          rendererOffCanvas = $(instance.element).attr('data-dialog-renderer') === 'off_canvas';
    

    Use `|| false`

  10. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -11,6 +11,41 @@
    +        if (!('dialogOptions' in instance.options.data)) {
    

    No `in`.

  11. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -11,6 +11,41 @@
    +        instance.progress = {type: 'fullscreen'};
    

    Spacing.

  12. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -21,10 +56,7 @@
    +    prepareAjaxLinks();
    

    Needs a comment.

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.96 KB
12.41 KB

1. fixed
2. fixed
3. fixed
4. Fixed more properties to object creation that aren't conditional
5. fixed
6. fixed
7, 8, 9, 10, 11, . Yes this logic will all be changed in #2844261: Allow dialog links to specify a renderer in addition to a dialog type. For that reason and also because all the logic in prepareAjaxLinks() is preexisting code that is simply wrapped in a new function I would say changes inside prepareAjaxLinks() are out of scope.
12. Fixed

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.

droplet’s picture

1. Should we put it on ajax.js or dialog.ajax.js ?

2. Shouldn't reuse the new helper for `$('.use-ajax-submit').once('ajax').each(function () {`

About the outside_in, I think we need a quick fix first, it affected the performance. reattached about 9 times on default homepage:
https://www.drupal.org/node/2901667#comment-12226496

tim.plunkett’s picture

Issue tags: +blocker, +Blocks-Layouts

This blocks Layout builder.

tedbow’s picture

Re-rolled

tim.plunkett’s picture

#20.1

+++ b/core/misc/ajax.es6.js
@@ -46,26 +46,7 @@
-        element_settings.dialogType = $(this).data('dialog-type');
-        element_settings.dialogRenderer = $(this).data('dialog-renderer');
-        element_settings.dialog = $(this).data('dialog-options');

@@ -269,6 +250,38 @@
+        dialogType: $linkElement.data('dialog-type'),
+        dialog: $linkElement.data('dialog-options'),
+        dialogRenderer: $linkElement.data('dialog-renderer'),

Despite the presence of these dialog properties, this is generic non-dialog code. It should stay in here.

#20.2
No, since this is only about contextual links, not buttons

I think all we need now are tests?

tedbow’s picture

I think yes just tests.

Here is test for clicking an ajax dialog.

I had to use hook_page_attachments_alter() to attach the core/drupal.dialog.ajax

Is there a better way to do this? I tried hook_contextual_links_view_alter(and left in patch) but this doesn't work. Is it too late?

droplet’s picture

+++ b/core/misc/ajax.es6.js
@@ -269,6 +250,38 @@
+   * @param {jQuery} $element

Should it pass HTMLElement instead? It seems handier for a globally shared script used for different places and does not include jQuery in advanced usage. (e.g. code split might not include jQuery in exact file)

It's some quick and extracts the code to a function but I'm not sure if it's good to bound jQuery.once inside in a new API. Usually, in the coding, we expected calling a function will re-fresh all states again. thoughts?
(Considered sometimes we will not replace the whole .use-ajax HTML completely and re-bound to a new behavior)

This patch might violate Drupal commit policy, Per Scope Per Issue. If we split it, I think the ajax part need not a test and commit that straightly. [committers call :)]

tim.plunkett’s picture

Assigned: Unassigned » tedbow
Priority: Normal » Major
Issue tags: -Needs tests
FileSize
2.63 KB
15.13 KB

Agreed with the first part about HtmlElement, that makes sense to me.

Moving away from .use-ajax and .once() seems out of scope to me.

The removal of Drupal.attachBehaviors(data.$el[0]); in settings_tray.es6.js has to happen in this issue, since it is a direct workaround for this bug.
Not sure of the prepareAjaxLinks() change, assigning back to @tedbow for that.

Also, this has tests now!

droplet’s picture

Moving away from .use-ajax and .once() seems out of scope to me.

I thought it's not out of scope or we should split the ajax part into a new issue (in favor of not out of scope :S )

1. We're exporting a new public API. If we apply it to D8.4 and released, we cannot make a change to it anymore (usually)

2. Adding API to resolve a single bug in a non-private project and throw all potential problem away, it's always a bad idea.

3. Nothing complicated, we only have to make a simple decision:

Plan A:
bindAjaxLink -> no jQuery.once
bindAjaxLinks -> has jQuery.once, re-use bindAjaxLink internal

Plan B:
bindAjaxLinks -> Remove no jQuery.once, handle it in your module

4.

+++ b/core/misc/ajax.js
@@ -139,6 +123,28 @@ function loadAjaxBehavior(base) {
+    $(element).find('.use-ajax').once('ajax').each(function (i, ajaxLink) {

current bindAjaxLinks in patch forced you to pass the parent scope jQuery(target).parent(). It's a very bad programming design.

tedbow’s picture

Assigned: tedbow » Unassigned

The logic in prepareAjaxLinks() was moved from inside Drupal.behaviors.toggleEditMode.attach because the fix is no longer attaching behaviors to ajax links as they are added as the fix in settings_tray.js(previously outside_in.js) was . So if it stayed where it was it would not longer be invoked. Before #2844261: Allow dialog links to specify a renderer in addition to a dialog type this logic was called in 2 different places so it needed to be in a function. Now prepareAjaxLinks() is only called once but since we have to move it I think it make more sense still to put it into function.

tedbow’s picture

Issue summary: View changes
FileSize
673 bytes
14.99 KB

When I rerolled in #22 it was after #2844261: Allow dialog links to specify a renderer in addition to a dialog type. I remove logic dealing with

  1. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -11,6 +11,41 @@
    +        // @todo Move logic for data-dialog-renderer attribute into ajax.js
    +        //   https://www.drupal.org/node/2784443
    +        instance.options.url = instance.options.url.replace(search, replace);
    

    This logic actually got removed from the patch in #22 it was committed in #2844261: Allow dialog links to specify a renderer in addition to a dialog type instead of #2844261: Allow dialog links to specify a renderer in addition to a dialog type which is still RTBC.

  2. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -11,6 +11,41 @@
    +   * https://www.drupal.org/node/2844261
    

    So this todo needs to be removed as this is already removed from the this patch

phenaproxima’s picture

  1. +++ b/core/misc/ajax.es6.js
    @@ -270,6 +251,39 @@
    +  Drupal.ajax.bindAjaxLinks = (element) => {
    

    Should this be an arrow function? I ask because arrow functions bind to the lexical scope, and I'm wondering if that might not have undesirable side effects further down the line. (But, I am not a JavaScript samurai, so I defer to the JS experts on this one.)

  2. +++ b/core/misc/ajax.es6.js
    @@ -270,6 +251,39 @@
    +        dialogType: $linkElement.data('dialog-type'),
    +        dialog: $linkElement.data('dialog-options'),
    +        dialogRenderer: $linkElement.data('dialog-renderer'),
    

    Should there be any safeguards against these data not being present (explicit checks and/or default values)?

  3. +++ b/core/misc/ajax.es6.js
    @@ -270,6 +251,39 @@
    +        base: $linkElement.attr('id'),
    

    Same here -- what if the element has no ID?

    Additionally, it might be ever so slightly more performant to simply use the native ajaxLink.id property.

  4. +++ b/core/misc/ajax.es6.js
    @@ -270,6 +251,39 @@
    +      /**
    +       * For anchor tags, these will go to the target of the anchor rather
    +       * than the usual location.
    +       */
    

    This should probably use the // comment style.

  5. +++ b/core/misc/ajax.es6.js
    @@ -270,6 +251,39 @@
    +      Drupal.ajax(elementSettings);
    

    It's possible to reach this call without elementSettings.url and elementSettings.event. Will Drupal.ajax() be OK with that, or will bizarre side effects occur?

  6. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -148,6 +148,27 @@
    +   * Prepare Ajax links to work with off-canvas and Settings Tray module.q
    

    There's an extra 'q' at the end of this line.

  7. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -148,6 +148,27 @@
    +      .filter(instance => instance && $(instance.element).attr('data-dialog-renderer') === 'off_canvas')
    

    A possible micro-optimization here: You could use instance.element.getAttribute() instead, to reduce the overall reliance on jQuery.

  8. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -148,6 +148,27 @@
    +        instance.options.data.dialogOptions.settingsTrayActiveEditableId = $(instance.element).parents('.settings-tray-editable').attr('id');
    

    Probably out of scope, but once again I'm wondering -- what if the .settings-tray-editable parent has no ID?

drpal’s picture

@phenaproxima

- #30.1 - It shouldn't be an issue. Especially since we aren't using this.
- #30.2/3/8 - Maybe.

$(element).find('.use-ajax').once('ajax').each((i, ajaxLink) => {
  const $linkElement = $(ajaxLink);
  // ...
});

If there are no $linkElement items, then it won't really matter. However, if the data attribute is not present, it will return undefined

- #30.4 - It follows our multi-line comment style, https://github.com/airbnb/javascript#comments
- #30.7 - 👍

dawehner’s picture

+++ b/core/misc/ajax.es6.js
@@ -1338,4 +1352,18 @@
+  /**
+   * Bind Ajax contextual links when added.
+   *
+   * @param {jQuery.Event} event
+   *   The `drupalContextualLinkAdded` event.
+   * @param {object} data
+   *   An object containing the data relevant to the event.
+   *
+   * @listens event:drupalContextualLinkAdded
+   */
+  $(document).on('drupalContextualLinkAdded', (event, data) => {
+    Drupal.ajax.bindAjaxLinks(data.$el[0]);
+  });

❓Naively I would have expected that this event would have been added on the contextual links site.

  1. +++ b/core/misc/ajax.es6.js
    @@ -1338,4 +1352,18 @@
    +  /**
    +   * Bind Ajax contextual links when added.
    +   *
    +   * @param {jQuery.Event} event
    +   *   The `drupalContextualLinkAdded` event.
    +   * @param {object} data
    +   *   An object containing the data relevant to the event.
    +   *
    +   * @listens event:drupalContextualLinkAdded
    +   */
    +  $(document).on('drupalContextualLinkAdded', (event, data) => {
    +    Drupal.ajax.bindAjaxLinks(data.$el[0]);
    +  });
    

    it feels wrong that ajax.js knows abut contextual links. Otherwise we could at least add some documentation why this is needed here.

  2. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
    @@ -81,4 +89,13 @@ public function testContextualLinksClick() {
    +    $this->assertSession()->assertWaitOnAjaxRequest();
    

    Can we wait on some element (the contextual links) to appear?

  3. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -148,6 +148,27 @@
    +   * Prepare Ajax links to work with off-canvas and Settings Tray module.q
    

    $x

  4. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -148,6 +148,27 @@
    +        if (!('dialogOptions' in instance.options.data)) {
    +          instance.options.data.dialogOptions = {};
    +        }
    

    Is there a reason we could not use Object.assign or ... ?

drpal’s picture

@dawehner

About #32.4, Object.assign() requires a polyfill and using the spread operator would require another Babel plugin.

tim.plunkett’s picture

#30

1,4) Addressed in #31
2,3,5,7,8) These are all the original code, we're just moving it
6) Fixed!

#32

1) Very good point
2) There is probably a more specific element to wait for, but this is the same assertion used in the rest of this test method.
3) Fixed :)
4) Addressed in #33

tedbow’s picture

Went through this and it looks like the reviews from @phenaproxima in #30 and @dawehner in #32 have been addressed.
So this looks good to me. Won't RTBC because I worked on this patch a bunch.

drpal’s picture

Status: Needs review » Reviewed & tested by the community

🤘 All the JavaScript changes look good. 🤘

dawehner’s picture

2) There is probably a more specific element to wait for, but this is the same assertion used in the rest of this test method.

Well, let's hope we don't run into new random failures ...

tim.plunkett’s picture

assertWaitOnAjaxRequest() should be more than sufficient, even if it is not very clear/semantic.
I opened #2915524: Make non-Settings Tray helper methods out of SettingsTrayJavascriptTestBase, and use them instead of assertWaitOnAjaxRequest() as a task to fix this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 2764931-contextual-34.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
15.64 KB

Conflicted with some eslint cleanup. Start the clock over on RTBC...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: 2764931-contextual-40.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

"CI Job missing"

  • webchick committed 18f322a on 8.5.x
    Issue #2764931 by tedbow, tim.plunkett, nod_, drpal, droplet, dawehner,...
webchick’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Needs work

OK, since this is a blocker to Blocks + Layouts and since the JS here is copy/pasta, going to "be bold" and commit this.

It does look like we could do with a follow-up for these existential questions:

+/**
+ * Implements hook_contextual_links_view_alter().
+ *
+ * @todo Apparently this too late to attach the library?
+ * It won't work without contextual_test_page_attachments_alter()
+ * Is that a problem? Should the contextual module itself do the attaching?
+ */
+function contextual_test_contextual_links_view_alter(&$element, $items) {

Committed and pushed to 8.5.x. Tried to cherry-pick to 8.4.x as well, but ran into conflicts.

alexpott’s picture

+++ b/core/misc/ajax.es6.js
@@ -1340,4 +1354,5 @@ else if (effect.showEffect !== 'show') {
+

+++ b/core/modules/contextual/js/contextual.es6.js
@@ -249,4 +249,19 @@ function adjustIfNestedAndOverlapping($contextual) {
+

Just to note that this is not as per the evolving JS coding standards and introduced a couple of regressions against #2914710: JS codestyle: padded-blocks in 8.5.x. Filed #2917303: JS codestyle: padded-blocks revisited to address this.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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