Contextual links don't work with 'use-ajax' links

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

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

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.

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.

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.

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

This blocks Layout builder.

#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?

+++ 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 :)]

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!

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.

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?

@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 - 👍

+++ 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 ... ?

@dawehner

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

#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

🤘 All the JavaScript changes look good. 🤘

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 ...

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.

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

"CI Job missing"

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.

+++ 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.