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.

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