Contextual links button is always rendered even when no links are available (with warm client-side cache)

The patch drupal_core_contextual-empty_contextual_filters-2650910-8.1.x-dev.patch at #4 works.

The patch looks good but as this is a javascript change and we have no test infrastructure in place we need some screenshots as test evidence.

Hi, Please refer the below screenshots

Admin user with the contextual links

Authenticated user with Contextual link Permission enabled - Before patch
The Link icon exist without sub-links

Authenticated user with Contextual link Permission enabled - After patch
The Link icon not exist without sub-links

Hi,
Created the patch for D8.2.x-dev version. Below i have attached the before and after screenshots and it works fine

Before Patch:

After Patch:

+++ b/core/modules/contextual/js/contextual.js
@@ -42,6 +42,9 @@
+    if ($(html).find('a').length === 0) {
+      return;
+    }

This is wrong. This won't work if the content contains any link, including non-contextual links

You're right. Sorry. I misread the code (it's been a long time since I've seen this!).

But… your code is still misleading. The right thing to do is checking if html is empty or not. Because if no links are accessible, then html will be the empty string. Verify it in your debugger.

Also, #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary has landed, so now we can write test coverage for cases like this! Look at core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest for an example.

Cool :) Thanks! And thanks for helping to fix this!

If the user do not have permission to access the associated links then contextual links are empty.
It is better to check if contextual links are not empty after we get them from the sessionStorage.

var html = storage.getItem('Drupal.contextual.' + contextualID);
        if (html !== null && html.length > 0) {
          // Initialize after the current execution cycle, to make the AJAX
          // request for retrieving the uncached contextual links as soon as
          // possible, but also to ensure that other Drupal behaviors have had
          // the chance to set up an event listener on the Backbone collection
          // Drupal.contextual.collection.
          window.setTimeout(function () {
            initContextual($context.find('[data-contextual-id="' + contextualID + '"]'), html);
          });
          return false;
        }

As it is checked after the Ajax request:

$.ajax({
          url: Drupal.url('contextual/render'),
          type: 'POST',
          data: {'ids[]': uncachedIDs},
          dataType: 'json',
          success: function (results) {
            _.each(results, function (html, contextualID) {
              // Store the metadata.
              storage.setItem('Drupal.contextual.' + contextualID, html);
              // If the rendered contextual links are empty, then the current
              // user does not have permission to access the associated links:
              // don't render anything.
             if (html.length > 0) {
                ...
                for (var i = 0; i < $placeholders.length; i++) {
                  initContextual($placeholders.eq(i), html);
                }
              }
            });
          }
        });

Adding related issue since this issue does not have good Google juice.

Patch in #18 works as expected.

the code that handling `storage.setItem` & `storage.getItem` should be same logic. Unbalance coding confused developers. Also considering the shortened way to check value exist: `if(var) { ...}`.

The patch in #18 is exactly right.

1. User starts with a fresh browser tab. User loads page.
2. We (== contextual.js) check the cache, we do:
   

   var html = storage.getItem('Drupal.contextual.' + contextualID);
   if (html !== null) {
     // cache hit, yay!
   

   If there's a cache miss, then getItem() returns null.

3. We had a cache miss, so we must request it. We iterate over the results in the response, we do this for every requested contextual link placeholder: if (html.length > 0) { — and only if some HTML is returned for a particular contextual link (i.e. if the current user has access to any of the contextual links), we call initContextual(). This is the intent.
4. User navigates to another page.
6. We again check the cache:
   

   var html = storage.getItem('Drupal.contextual.' + contextualID);
   if (html !== null) {
     // cache hit, yay!
   

   Now we have a cache hit! So we call initContextual(). But… we call it blindly. We forget to check if the current user has access.

(Look at \Drupal\contextual\Tests\ContextualDynamicContextTest::testDifferentPermissions(), you'll see that an empty string being returned for a given contextual link ID is indeed intended to return the empty string when it is inaccessible.)

Conclusion: the patch in #18 is entirely correct.

So -1 for #22.

However, this still needs JS test coverage.

Actually, even better would be to test html !== '', because html.length > 0 would e.g. also allow arrays/objects.

Updated IS with #23's analysis.

Bumping to major in light of #2362435: When viewing a revision, the Quick Edit, Edit, and Delete contextual link operations are available, but should not be.

#2047671: Expanded contextual links look goofy when there are no items fixed this for the non-cached case. This is fixing it for the cached case.

What do you think about this condition like this: typeof html === 'string' && html.length > 0?

(And one question: Does the core prefer foo.length > 0 instead of just foo.length for readability reasons?)

Thank you for the patch, it works for me.

Patch in #29 fixed the problem of the contextual button displaying when no contextual links were available (Drupal 8.2.3). Thanks!

This looks *really* simple solution and the attached unpatched and patched screenshots show it is effective. Marking RTBC...

As per #23 this still needs test coverage. Thanks everyone!

Tests.

YAY!

This is a bug and the "benefits outweigh the disruption". We are still in 8.3 beta so I think this can be applied to 8.3

I was going to say I was too timid to change it but that's something I'm consciously trying to change!

Please correct if I'm wrong.

Nope, you're absolutely right. And even if 8.3.0 was already released, this would still be able to go in to 8.3, because it's simply a bugfix without other consequences.

I disagreed with #23, +1 for #22

First,
It patched the LINE 166 with:

if (typeof html === 'string' && html.length > 0) {

But LINE 195:

if (html.length > 0) {

Unless we make assumptions there, I don't know why we end up with 2 different conditions.

Second,

stringNull = null;

if(stringNull) {
 console.log('stringNull error')
}

stringEmpty = '';

if(stringEmpty) {
 console.log('stringEmpty error')
}

stringHTML = 'CORRECT';

if(stringHTML) {
 console.log('stringHTML correct')
}

Thrid:

window.sessionStorage.setItem('arrayTest', ['arrayItem']);

var isArray = window.sessionStorage.getItem('arrayTest');

console.log(`typeof isArray is ${typeof isArray}`)

Storage interface returns DOMString, or null.

also,

LINE 191:

              storage.setItem('Drupal.contextual.' + contextualID, html);

Should we save empty HTML then?

@droplet

Thanks for pointing that out. I updated the logic to check for a non-null element and that the element has a length.

#41: yes, saving the empty HTML is intentional: that means the current user cannot access those particular contextual links. It's pointless to re-request them over and over again. If you want to change that, then let's do that in a separate issue.

#42: See #24 for why I suggested checking if it's a string. It was typeof html === 'string' until #42, but I think html !== '' would have been okay too.
That's no longer being checked in #42. Are you sure that this is okay?

@Wim Leers

It's my understanding that the interface for window.sessionStorage is to always return a string when requesting an item. Here we are checking to see that html exists, not null, and that it has a length; which in weird JavaScript land, the result of checking the length of the string will result in a true case if that value is non-zero.

+++ b/core/modules/contextual/js/contextual.js
@@ -192,7 +192,7 @@
-              if (html.length > 0) {
+              if (html && html.length) {

Yes, but in this location, html is not read from sessionStorage. So it might be an object or an array — which is what I wrote in #24.

@Wim Leers

If there’s ever a possibility that the result of the AJAX call to contextual/render will return something that is not a string then we need to expand the checking.

I don't see how that can happen, so then this is fine. Still, by that same logic, we shouldn't even be modifying line 196. Because html cannot ever be null when parsing a successful AJAX response. (In other words: by that same logic, #23's argument is invalid.)

Anyway, this is fine. Let's get this in. Finally.

Still, by that same logic, we shouldn't even be modifying line 196. Because html cannot ever be null when parsing a successful AJAX response.

Yes, it is. (or a chance returned false or null?) Basically, I suggested if (html) {} but if we end up with current patch. It's still fine.

+++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -0,0 +1,42 @@
+    $this->assertSession()->elementNotExists('css', '.contextual button');
...
+    $this->assertSession()->elementNotExists('css', '.contextual button');

This test is only a negative test - is there any chance to add additional tests to ensure contextual links are there in the right conditions?

Fixing event tag :)

Adding an additional test step from #49.

Need to wait for ajax to complete.

• After some coversation at DevDays, I think waitForElement makes more sense.

1. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,60 @@
   +    // Grant Permissions
   

   s/Grant Permissions/Grant permissions to use contextual links on blocks./

2. Please upload a test-only patch (which should fail) along with the patch that includes the solution (which should pass).

Just updating status to match comments...

@Wim Leers

Thanks.

+++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -0,0 +1,60 @@
+    $this->assertSession()->elementNotExists('css', '.contextual button');
...
+    $contextualLinks = $this->assertSession()->waitForElement('css', '.contextual button');
+    $this->assertNotEmpty($contextualLinks);

I think it would better if you used the exact testing before and after you grant the permission.

Instead of using $this->assertSession()->elementNotExists
You can use $this->assertSession()->waitForElement in the first part of the test also. waitForElement() will return NULL if not found after the wait. So you can just check that it is empty.

That way we can be sure that element just doesn't exist because we have waited for it.

Fixes from #62.

Patch looks good. I tested it and working for me. Changing status to RTBC

Random test failure.

I tested the patch and it works for me.

1. +++ b/core/modules/contextual/js/contextual.js
   @@ -192,7 +192,7 @@
   -              if (html.length > 0) {
   +              if (html && html.length) {
   

   As mentioned on #47, this is out of scope change since successfull HTML response can never be null.

2. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,60 @@
   +use Drupal\user\Entity\Role;
   

   Unused use statement.

3. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,60 @@
   +}
   

   The closing brace for the class must have an empty line before it.

Incorporated feedback from #69.

Committed 488a6b1 and pushed to 8.4.x. Thanks!

I think we should backport this to 8.3.x - going to get a +1 from another committer because we're in RC for 8.3.0.

+1—it's a one-line clean change with test coverage to rid us of an ugly problem.

Committed 600e6db and pushed to 8.3.x. Thanks!