Remove contextual links in Seven

I'll start looking into this.

Hey Bojhan,
Do you know what the NID is for the views issue you referenced? I'd like to take a look at their solution

It looks like the only way to do this is hook_block_view_system_breadcrumb_block_alter() as the block module adds contextual filters with the comment: "All blocks get a 'Configure block' contextual link."

Sorry, @shellshocked59, thought you'd set this aside.

@mradcliffe

It looks like the only way to do this is hook_block_view_system_breadcrumb_block_alter() as the block module adds contextual filters with the comment: "All blocks get a 'Configure block' contextual link."

Hmm, so you think this should be handled in block module?

Attached is a crude patch to the contextual module for discussion's sake.

I have a patch that removes contextual links using hook_block_view_BASE_BLOCK_ID_alter();

I put this inside of the "contextual" module, but I'm open to suggestions on what a better location or approach for this fix would be. This current approach will hide contextual links for all blocks using "system_breadcrumb_block" as a base, not just breadcrumbs on seven. I'm unsure if this is desirable.

Thanks for working on this issue!

+++ b/core/modules/contextual/contextual.module
@@ -189,3 +189,11 @@ function _contextual_id_to_links($id) {
\ No newline at end of file

Against the Drupal coding standards there should be new line on the end of file.

I added the new line, thanks.

Bojhan I wasn't sure if needed to affect seven or all breadcrumbs. 2487025-9-SEVEN-ONLY.patch affects only seven, while 2487025-9.patch affects all breadcrumbs. Witch path would you like to proceed with?

function contextual_block_view_system_breadcrumb_block_alter(array &$build, \Drupal\Core\Block\BlockPluginInterface $block){
  if($build['#id'] == 'seven_breadcrumbs'){

Missing spaces before both the curly braces. Also missing space after the if before the opening parenthesis.

Also maybe instead of

function contextual_block_view_system_breadcrumb_block_alter(array &$build, \Drupal\Core\Block\BlockPluginInterface $block) {

We could include the BlockPluginInterface at the top of the file

  use Drupal\Core\Block\BlockPluginInterface;

and then the function parameter can be:

function contextual_block_view_system_breadcrumb_block_alter(array &$build, BlockPluginInterface $block) {

Both ways appear to be used in core, but having the use include at the top seems more readable.

Thank you for the recommendation. I added use Drupal\Core\Block\BlockPluginInterface; to the top of the file and added spaces as advised.

+++ b/core/modules/contextual/contextual.module
@@ -8,6 +8,7 @@
+use Drupal\Core\Block\BlockPluginInterface;

These should be in alphabetical order :)

Is there not some way to achieve form within Seven theme itself? Seems rather unfortunate to introduce knowledge about Seven theme to Contextual module.

Yes, it is not ideal. @shellshocked59 spent a few hours this afternoon trying to figure out a solution. template_preprocess_block wasn't going to work because of contextual_preprocess() iirc.

Addressed #14

@tstoeckle I agree, this shouldn't be done from the contextual module if possible

@mradcliffe I took another look at the solution using template_preprocess_block() and it works! I attached a patch here that works using this hook inside of seven.theme for a cleaner solution.

Patch works as intended RTBC

Patch works as intended RTBC

Sorry for the noob question, but is it helpful for me to attach screenshots and basically RTBC my own patches? Or should I do this but still mark it as "needs review"?

@shellshocked na the point of someone else testing is that they haven't worked on the code and its a fresh set of eyes. Before its committed it will be tested again.

@shellshocked59, thanks for following up on the issue with the new approach. Also, one thing that helps when uploading patches is to create an interdiff of the changes.

I reviewed the patch and found a couple of things. The patch has to remove from the render array items and classes added by the contextual module so that the contextual module's Javascript is not invoked if the contextual module is enabled.

1. +++ b/core/themes/seven/seven.theme
   @@ -82,6 +82,23 @@ function seven_preprocess_node_add_list(&$variables) {
   +  if ($variables['plugin_id'] == 'system_breadcrumb_block' && isset($variables['title_suffix']['contextual_links'])) {
   

   It would be nice to have a brief explanation of why we need to loop through the render array. I'm not sure if there is a follow-up issue here somewhere to fix or change the behavior so we don't have to alter in it in the future.

   Maybe @Bojhan has some thoughts if we should create a follow-up task?

2. +++ b/core/themes/seven/seven.theme
   @@ -215,3 +232,5 @@ function seven_library_info_alter(&$libraries) {
   +
   +
   

   Nitpick review: I don't think this change is necessary in the seven theme.

@aburrows, could you update the issue summary and add the screenshots into the issue summary? Dreditor's Embed functionality should be displayed for your attachments. Also we should probably add the approach that @shellshocked59 added from Comment #18.

I believe we also need a Beta Evaluation template because this is a Normal bug. I looked at the Allowed Changes for Drupal 8 Beta document, and we should review the issue priority, category, and patch to confirm the changes made.

An automated test would also be helpful to confirm that the selectors are not there when contextual module is enabled on a page with breadcrumbs. Perhaps in system module or contextual module, but I won't add the "Needs tests" at the moment.

@mradcliffe did you need me to make a change to the patch? I wasn't sure from your comment.

I've updated the issue to propose we should remove contextual links everywhere in Seven. See: #507488: Convert page elements (local tasks, actions) into blocks

Here's a patch to change remove contextual links from all blocks in Seven @LewisNyman

I changed:
if ($variables['plugin_id'] == 'system_breadcrumb_block' && isset($variables['title_suffix']['contextual_links'])) {
to
if (isset($variables['title_suffix']['contextual_links'])) {

I also removed some extra code at the bottom as requested by @mradcliffe and attached an interdiff this time.

I am going to attempt a code review of the latest patch, using these instructions: https://www.drupal.org/contributor-tasks/review

I will be testing this patch using these instructions: https://www.drupal.org/contributor-tasks/manual-testing

I'm going to run the beta evaluation (https://www.drupal.org/contributor-tasks/update-allowed-beta)

I'm working with @lweinmeister to update the issue summary with beta evaluation. We are sprinting today.

I have looked through the patch in #26 it looks like this patch fits within the scope of the issue(which seems to have changed slightly in #25). I don't know enough to be able to say that unset is the best way to implement this change but it seems slightly different from comments in mradcliffe's comment in #23. As a Nit, comments in the patch do not end with periods.

Here's an updated patch with periods added to the comments

I'm open to suggestions besides the unset() solution I used.

Beta phase evaluation

I removed the tags because beta evaluation was added in #33 and issue summary was updated in #25. Our sprint is finished and I hope this was helpful.

Setting to needs review because there is patch

1. +++ b/core/themes/seven/seven.theme
   @@ -80,6 +80,23 @@ function seven_preprocess_node_add_list(&$variables) {
   + * Disables contextual links for system breadcrumb block.
   

   AFAICT this applies to all blocks?

2. +++ b/core/themes/seven/seven.theme
   @@ -80,6 +80,23 @@ function seven_preprocess_node_add_list(&$variables) {
   +      if ($class == 'contextual-region') {
   

   Let's use ===.

Updated the comment to "Disables contextual links for all blocks." and changed to === for the comparison

+++ b/core/themes/seven/seven.theme
@@ -80,6 +80,23 @@ function seven_preprocess_node_add_list(&$variables) {
+    foreach ($variables['attributes']['class'] as $key => $class) {
+      if ($class === 'contextual-region') {
+        unset($variables['attributes']['class'][$key]);
+      }
+    }

Can't this be simplified by using \Drupal\Core\Template\Attribute::removeClass()?

Manually tested, works correctly. Only remark: contextual_page_attachments() still causes the contextual JS to be added. Do we want Seven to override that in seven_page_attachments(), therefore preventing contextual links from ever working? I don't think so, because 1) there could be use cases that we're missing here, 2) e.g. the Views preview shows front-end content (even though that should probably be an iframe…), and IIRC there was an issue not long ago about the contextual links of the previewed view not working.

Implemented what @wimleers suggested in #41. Not sure if there are remaining issues. Seems to work like a charm.

CONTEXTUAL LINKS BEFORE

CONTEXTUAL LINKS AFTER

If Wim is happy, I'm happy. Setting to needs work as we need someone to write some tests for this.

+++ b/core/themes/seven/seven.theme
@@ -80,6 +81,20 @@ function seven_preprocess_node_add_list(&$variables) {
+    $attribute = new Attribute(array('class' => $variables['attributes']['class']));
+    $attribute->removeClass('contextual-region');

Oh hrm… I thought this was already an Attribute instance?

Perhaps it's then better to revert back to what we had before, because otherwise we may be breaking subsequent preprocess functions?

Tests can be as simple as a request to /admin and assertNoRaw('data-contextual-id') + assertNoRaw('contextual-region').

@shellshocked59, just end your interdiffs with .txt and they won't get sent for testing. (or just not .diff or .patch)

+++ b/core/themes/seven/seven.theme
@@ -80,6 +81,20 @@ function seven_preprocess_node_add_list(&$variables) {
+    $attribute = new Attribute(array('class' => $variables['attributes']['class']));
+    $attribute->removeClass('contextual-region');

This isn't currently doing anything. You have to save the new attribute object into $variables. The link go away because of the unsets, but the class would stay. But, the correct thing to do would be to make a new attribute object using $variables['attributes'], because you otherwise lose the other attributes like id.

I agree that it doesn't quite smell right. I don't think it would break anything, though, since the theme should get processed last.

Wouldn't it be simpler to just remove it with an array_diff?

Coming from #2561557: Styling issue on context menu after local tasks become block after #507488: Convert page elements (local tasks, actions) into blocks went in. Reroll of #42. Is it worth adding a variable, so that we can enable, if we need?

The feedback in #45 still needs to be addressed, and this still needs tests.

Tests can be very simple:

$this->drupalGet('admin');
$this->assertNoRaw('contextual placeholder markup');
$this->assertNoRaw('contextual classes');

Fixed the preprocess block as suggested by #46.
Included the test as suggested by #48.

Helping mentor an issue review on this issue.

Testing the issue at #drupalconeur. Tested using SimplyTest.me. The contextual links don't appear with latest patch 49

Screenshot here

Thanks for you review @swetashahi! RTBC++

+++ b/core/modules/block/src/Tests/BlockAdminThemeTest.php
@@ -43,4 +43,42 @@ function testAdminTheme() {
+   * Check if contextual links are disabled in Seven theme.

s/Check if/Ensure (can be fixed on commit)

In the test, the array is using the short array syntax. There has not been any decision on how to officially write those when in used as a function parameter. Should the opening (left) bracket be on the same line as the function call and the closing bracket (right) be on the same line as the closing parantheses?

This is how it was for long array syntax:

Function(array(
  'value'
));

Should this be

Function([
  'Value'
])

I wanted to post this at home but had to run to catch the bus this morning. So excuse the brevity from phone issue posting.

I haven't yet had any coffee, but after a good bus ride thinking and waking up about this, I think that it would be better to conform to what we have in core already similar to long array syntax.

1. +++ b/core/modules/block/src/Tests/BlockAdminThemeTest.php
   @@ -43,4 +43,42 @@ function testAdminTheme() {
   +    $admin_user = $this->drupalCreateUser(
   +      [
   ...
   +      ]
   +    );
   

   So for instance in core/tests//Drupal/Tests/Core/Utility/LinkGeneratorTest.php we have

   $this->assertLink(array(
     'attributes' => array('href' => $expected_url),
   ), $result);
   

   I could not find a place where short array syntax was used like this elsewhere so I think this is a first. Probably best to comply with the way that long array syntax is done in the example above.

2. +++ b/core/modules/block/src/Tests/BlockAdminThemeTest.php
   @@ -43,4 +43,42 @@ function testAdminTheme() {
   +        'view the administration theme'
   

   This should have a trailing comma per https://www.drupal.org/coding-standards#array

Alright, I'll check the patch and update it to the long syntax.

Regards,

I think the short array syntax is fine, harings_rob. Just that the opening bracket should begin on the same line as the function call, and the ending bracket should be on the same line as the ending parentheses.

Patch as per #54, #58

Did another manual test of the patch on simplytest.me, and confirmed that I did not visually see contextual links. I think this is RTBC again. :-)

I also added a summary of all the contributions made to this issue. I hope I got it right.

Looks like a random test fail on old pifr bot.

I think this makes sense to do. I remember catching all manner of holy hell from the Views maintainers for the decision to remove contextual links from e.g. admin/content, though. OTOH I do see that for example you get contextual links in completely oddball places atm (see #42) so for now I think it's better UX-wise to do what this patch does and remove them wholesale. We can always selectively add more back contextual gears back later if we feel that is wise.

I guess my only question would be whether Seven is the right place for this logic, or whether it belongs in Contextual module for any admin theme. However, this approach runs the least risk of breaking something out there in the wild, so I think is probably the best way to go.

Committed and pushed to 8.0.x. Thanks!