Block weights reset for all blocks in a region breaks placement relative to other visibility groups

@oheller so just be clear you are just viewing the block listing not saving the form?

I'm seeing a similar challenge, not a bug in Block Visibility Groups but a workflow problem stemming from how core's drag and drop block placement behaviour sets block weights. I'm not sure if the issue I'm seeing is that described here, so I'll detail it.

Issue

Core's drag and drop block placement behaviour may interfere with Block Visibility Groups.

Specific problem

When using drag and drop to reorder blocks, all blocks on the form (not only a specific block that was moved) may be assigned a new weight. This works okay when only core is in play and a single form is used to reorder all blocks, since reassigned weights retain their relative ordering. When editing only a specific block visibility group, however, the fact that all of that group's blocks may be assigned a new weight means that, while their internal ordering is okay, their ordering relative to blocks placed globally or via other block visibility groups may have changed.

The problem arises from variations on the following steps:

• Assign some sitewide blocks.
• Create one or more block visibility groups and assign them blocks.
• Bring up the main block layout page with "All blocks" selected for "Block Visibility Group". Note the relative ordering of blocks.
• For "Block Visibility Group", select one of the groups you created previously. Add a new block and drag and drop it for ordering. Save.
• Return to the main block layout page with "All blocks" selected for "Block Visibility Group". Result: not only the new block, but others as well may have shifted in relative position. Source of problem: weights for all blocks in the block visibility group were changed, so their position relative to other blocks on the site has changed.

Workaround

A workaround is: whenever using any block layout screen, ensure "Show row weights" has been selected before making any changes, so that all block ordering is done manually through explicitly selecting block weights.

I can confirm having this issue. I'll set some global blocks as well as some section or page specific blocks. If I make a change to the section/page blocks not on the ALL-GROUPS filter, when I hit save the weights will be very different for the global blocks.

Got this issue too. It is the only critical issue that prevents me from using this module on every site I create.

Patch attached. I've updated the summary with details. Briefly, we need to set a tabledrag option so weight is reset only for the row that was dragged.

Removing a stray debug line.

@nedjo thanks for the patch!

There is some problem with undefined indexes caused by this tests.

Also we should have a test proving this is problem and the patch fixes it.

Could do a test where we should loop through and add a bunch of blocks and confirm this problem happens?

@tedbow

I'll look into the test failures.

For test coverage, there are some core tests we could draw on that use JS drag and drop such as:

• BookJavascriptTest::testBookOrdering()
• FieldLayoutTest::testEntityForm()

The test failures seemingly were triggered by iterating the tabledrag array by reference. Removing that.

I also had wrong the change we need to make here. The 'relationship' determines what element is used as reference, so 'sibling' is correct. What we apparently need to change is the 'action'. An action value of 'order' affects all elements in a group. The action 'match', used with the relationship 'sibling', should be what we're looking for. It's the same combination as is used with the region select. There, the logic seems to be: when I drop a row, I may have brought it from a different region, so set the row's region value to match that of a sibling in the region where I dropped it. What we're looking for with weight is the same.

But note that if this patch works, it will produce a degradation in functionality. If all rows in a region are weight 0 and I drag one of them, it will get 0 as a weight and so remain unchanged in its relative ordering. But this may be preferable to the current behaviour.

All of that said, I'm not certain this is working for me when I test manually.

Setting to needs review to see if the test failures are addressed. If so, will set back to needs work for the tests.

Okay, that passed anyway. Setting back to needs work.

Assuming we get this patch working, maybe the way to go would be to introduce a radio-type configuration option.

Help text:
Select your preferred drag and drop behaviour for blocks. The Drupal core default can cause challenges in that blocks in one block visibility group may be reordered relative to those in other groups.

Block drag and drop behaviour on block
[ ] When a block is dragged and dropped, only the dragged block is given a new weight
[ ] Drupal core default: when a block is dragged and dropped, all blocks in a region get new weights
[ ] Disable drag and drop: all block ordering is by manually setting weights

@nedjo so this is not an issue if this module is not installed correct?
I mean if you are just using core and you have large amount of blocks this behavior doesn't happen, correct?

The behaviour is the same whether you have this module installed or not.

With core, you only have a single form where you're reordering blocks. So in that case, the fact that blocks are reordered relative to each other in a given region is exactly what you want.

With this module, we're working with only a subset of the blocks on the site. So when I edit a visibility group's blocks and they're all reassigned weights, that can break their ordering relative to blocks that I didn't edit (i.e., ones in another visibility group, or global blocks if I've selected not to see them when I work with my visibility group).

Even if the patch I've written works, it will lead to problems. If all blocks are assigned a weight of 0 by default, then dragging them around will have no effect, since they'll always get a 0 value from their siblings.

Possibly, what we need to do is ensure we always have all blocks on the site present, but either (a) only blocks in the current visibility group are visible or (b) only blocks in the current visibility group are draggable.

Or, more radically, we simply remove the tabledrag behaviour from these forms. Manually setting weights will work, though it requires some practice and comparison between weights in different visibility groups.

Possibly, what we need to do is ensure we always have all blocks on the site present, but either (a) only blocks in the current visibility group are visible

Specifically, rather than unsetting IDs in BlockVisibilityGroupedListBuilder::getEntityIds(), we would load all blocks but hide those that should not display. That way, possibly, although only the current group's blocks would be visible on a visibility group block editing form, all blocks from all visibility groups plus global would be present on the form and so their weights would be reassigned just as they are in core's single form.

Yeah but if you dragging the blocks around and user doesn't know they changing relative to other blocks(because they are hidden) from the user perspective wouldn't pages where even this visibility group doesn't show be affected?

One way to solve this would be to show all blocks but for blocks that aren't in the current group

• grey them out
• Don't show region drop down or operations
• Show "Current group"
• Add a "Move to this group" link

It seems other wise you are always going to be sorting blocks in relation to blocks you can't see.

"Move to this group" could either reload the page or do ajax call update the block condition and refresh the row with it as part of the group.

Then maybe delete the "Show Global Blocks" checkbox because it would no longer apply.
You could possible add a "Show only current group blocks and global block" checkbox but show warning about possible side effects for page that more than 1 group showing at a time.

@tedbow

wouldn't pages where even this visibility group doesn't show be affected?

They'd be affected in that their blocks' weights would change, but as long as their relative ordering didn't change, that would be okay. Basically, it should work just like core's block display form.

So say we have the following blocks on the site in two visibility groups, A and B:

A1 (weight 0)
B1 (weight 1
A2 (weight 3)
B2 (weight 4)
B3 (weight 5)
A3 (weight 6)

Editing visibility group B, I see:

B1 (weight 1
B2 (weight 4)
B3 (weight 5)

The A blocks are there, just not visible.

I drag B2 to above B1. All block weights are reset, as follows:

A1 (weight -10)
B2 (weight -9)
B1 (weight -8)
A2 (weight -7)
B3 (weight -6)
A3 (weight -5)

It's okay because their relative placement hasn't changed.

Here's a draft proof of concept of what I described in #18 and #21.

All blocks are always present, but we visually hide those that aren't editable in the current visibility group. Drag and drop reassigns weights of all blocks in the region (global plus those in all visibility groups) while retaining their relative ordering. Seems to work on my local testing.

As well as tests, we still need:

• Handling for the case that a given region has blocks but they're now all invisible. Need to change relevant class from 'region-populated' to 'region-empty'.
• Optimizing?

I'm adding a draft test, but it's not working yet (fails before we get to the part that tests for this issue).

With a non-empty patch this time ;)

See the handbook page PHPUnit Javascript testing tutorial for details on how to run JS tests.

@nedjo sorry it has taken me so long to get back I tried to reroll the patch

+++ b/src/BlockVisibilityGroupedListBuilder.php
@@ -217,6 +217,16 @@ class BlockVisibilityGroupedListBuilder extends BlockListBuilder {
+
+    // Visually hide blocks not shown in this visibility group.
+    $entity_ids = $this->getEntityIds();
+    $group_entity_ids = $this->getGroupEntityIds();
+    foreach ($entity_ids as $entity_id) {
+      if (isset($form[$entity_id]) && !in_array($entity_id, $group_entity_ids)) {
+        $form[$entity_id]['#attributes']['class'][] = 'visually-hidden';
+      }
+    }
+

This is the part that didn't apply. So you might want to check I got this right.

It now fails earlier in the new test. Not sure why

Looks like the test has a problem. After setting block weights directly, BlockVisibilityGroupsJavascriptTest::testBlockOrdering attempts to retrieve the name="blocks[][weight]" select input field value — but "null" returns if nothing has been explicitly selected:

      // Place blocks in group 1.
      $block3 = $this->placeBlockInGroup('system_powered_by_block',  $group1->id(), ['weight' => 3]);
      . . .
      $page = $this->getSession()->getPage();
      $weight_select1 = $page->findField("blocks[{$block3->id()}][weight]");
      . . .
      $this->assertEquals($block3->getWeight(), $weight_select1->getAttribute('value'), 'block3 ' . $block3->getWeight());

Test says:
1) Drupal\Tests\block_visibility_groups\FunctionalJavascript\BlockVisibilityGroupsJavascriptTest::testBlockOrdering
block3 3
Failed asserting that null matches expected 3.

Rather than setting the weights directly, perhaps the test should set the weights through the form. I'll give that a shot.

Correction. That should have been:

 $this->assertEquals($block3->getWeight(), $weight_select1->getValue(), 'block3 ' . $block3->getWeight());

which *does* return the proper value. However, another block does not (block4).

It appears this issue is due to the limited range of weight options in the form select when a specific Block Visibility Group is selected on the Block layout page. The problem can be reproduced by setting weights directly rather than using drag and drop. with "Show row weights" selec.

Steps to show problem

1. Create a block visibility group, say "Test Group," with no group blocks placed in any region.
2. With "Block Visibility Group" set to "Global Blocks," place two or more global blocks in a region.
3. Set "Block Visibility Group" to "Test Group" with "Show Global Blocks" checked.
4. Place a couple of Test Group blocks in the same region.
5. With at least 4 blocks placed on page, set weight of Test Group blocks to 1 and 3 with global blocks unique values.
6. Uncheck "Show Global Blocks"
7. Recheck "Show Global Blocks"

In the above sequence, the Test Group block that was set to a weight of 3, is reset to a weight of -1, and that change persists, say, when "All Blocks" is selected for "Block Visibility Group." It seems that checking "Show Global Blocks" does an implicit "Save blocks." In particular, if step 7 is to select "All Blocks" rather than check "Show Global Blocks," then the as-set weights remain unchanged. Also, in the following sequence, the weight-3 block is also reset:

1. Create a block visibility group, say "Test Group," with no group blocks placed in any region.
2. With "Block Visibility Group" set to "Global Blocks," place two or more global blocks in a region.
3. Set "Block Visibility Group" to "Test Group" with "Show Global Blocks" checked.
4. Place a couple of Test Group blocks in the same region.
5. With at least 4 blocks placed on page, set weight of Test Group blocks to 1 and 3 with global blocks unique values.
6. Uncheck "Show Global Blocks"
7. Save blocks (no changes necessary)
8. Select "All Blocks"

Analysis and patch

The problem arises because Drupal\block\BlockListBuilder::buildBlocksForm() sets the #delta for the form weight select by counting the blocks shown on the Block layout page:

    $entities = $this->load();
    . . .
    // Weights range from -delta to +delta, so delta should be at least half
    // of the amount of blocks present. This makes sure all blocks in the same
    // region get an unique weight.
    $weight_delta = round(count($entities) / 2);

The load() method calls $this->getEntityIds(), which is overridden in BlockVisibilityGroupListBuilder to return only the group block ids when not showing global blocks.

In the attached patch, the problem is addressed by reseting the #delta value in BlockVisibilityGroupListBuilder::buildBlocksForm(). The patch adds test Drupal\Tests\block_visibility_groups\Functional\BlockVisibilityGroupsWeightTest, which fails in 8.x-1.x and passes with the patch.

Dispatcher ran test as simpletest, but this is PHPUnit test. Hum . . .

No, missing PHPDoc @group directive.

Removed inappropriate dependency on devel modules. Third time the charm?

Rather than setting the weights directly, perhaps the test should set the weights through the form.

Thanks for picking up this old issue!

There may indeed be a bug related to the weight delta. But I'm pretty sure that's a different bug than the one I originally noted, which occurs when using drag and drop to reorder blocks within a block visibility group and specifically is about resetting weights relative to blocks that are not on the form. This bug cannot be directly reproduced without dragging. The workaround I suggested, and have used successfully, is to disable drag and drop.

@nedjo Granted, and good point. It's not clear whether the weight issue resolves the issue as stated, but it does cause a problem with the relative order of group blocks to global blocks, so needed to be fixed to address the issue. I'm creating an additional test that exercises the issue more directly and sets the order by dragging rather than setting weights. We'll see.

By the way, the test originally submitted fails before reordering anything because it does set weights directly — to invalid values.

Failed asserting that '-3' is greater than '3'.

If there are N blocks on the page, weights are constrained to the range -N to N by BlockListBuilder, and those outside the range are reset when the form is rendered by core without regard to preserving order. (This may be an issue for the core blocks module, but since the UI doesn't allow weights outside the range, it may not be worth the bother.) As written, the test sets 6 weights to 1..6, thus 4..6 get reset.

Correction: For N blocks the allowed range of weights is -M to M where M is round(N/2). For 6 blocks, that's -3 to 3.

The attached file adds test FunctionalJavascript\BlockVisibilityGroupsOrderTest with no other changes, and it will fail.

The test sets up by placing eight blocks — three global, two in Group A, and three in Group B — in the following order (lighter to heavier): G1, G2, G3, B1, A1, B2, A2, B3. The following tests are performed:

• testSetup() makes sure that setUp() did what was intended. (should pass)
• testWeightRange() checks that blocks are allowed the full range of weights when "Block Visibility Group" is set to something other than "All Groups".
• testBlockReorderByWeightShowGlobal() swaps A1 and A2 weights with "Group A" selected and global blocks shown.
• testBlockReorderByWeightShowGlobal() swaps A1 and A2 weights with "Group A" selected and global blocks hidden.
• testBlockReorderByDraggingShowGlobal() swaps A1 and A2 by dragging with "Group A" selected and global blocks shown.
• testBlockReorderByDraggingShowGlobal() swaps A1 and A2 by dragging with "Group A" selected and global blocks hidden.

The tests check the order of Group A and others independently. That is, swapping A1 and A2 should result in the order [A2, A1] while the order [G1, G2, G3, B1, B2, B3] should not change in any test. However, the relative order of Group A blocks to hidden blocks *may* change when dragging.

It isn't clear to me that this test is sufficient. Hidden blocks are not *always* reordered when reordering group blocks; the test case is just one where it is. Are more cases needed?

This patch adds the weight "fix" reported in message #33. The test fails in my local, but that *might* be due to the issue reported in https://www.drupal.org/node/2769825.

This patch has the fix toBlockVisibilityGroupListBuilder provided by @nedjo. Since that removes the getIdentityId() override, weight ranges are set by the full block count rather than just those visible. That addresses the issue in #33 in a cleaner way and adds other changes which seem to address the issue. The test passes in my local.

@jfmacdonald this really looking good. I like that we are testing this multiple ways.

1. +++ b/tests/src/FunctionalJavascript/BlockVisibilityGroupsOrderTest.php
   @@ -0,0 +1,568 @@
   +  public function testSetup() {
   +    $expected_order = ['g1', 'g2', 'g3', 'b1', 'a1', 'b2', 'a2', 'b3'];
   +    $this->getBlockLayoutPage('ALL-GROUP');
   +    $this->assertBlockOrder($expected_order);
   +  }
   

   This could be moved inside setUp(). It would save a having an extra test method which is another Drupal install.

2. +++ b/tests/src/FunctionalJavascript/BlockVisibilityGroupsOrderTest.php
   @@ -0,0 +1,568 @@
   +    $this->assertBlockOrder(['a2', 'a1'], $msg);
   +    $this->assertBlockOrder($this->stable_order, $msg);
   

   Why aren't all 4 tests that change the order ending with $this->assertBlockOrder($this->stable_order, $msg);
   This should always pass, correct?

3. +++ b/tests/src/FunctionalJavascript/BlockVisibilityGroupsOrderTest.php
   @@ -0,0 +1,568 @@
   +  protected function setShowGlobal($check) {
   +    $checkbox = $this->getSession()
   +                     ->getPage()
   +                     ->findField('block_visibility_group_show_global');
   +    if ($checkbox) {
   +      if ($check) {
   +        $checkbox->check();
   +      }
   +      else {
   +        $checkbox->uncheck();
   +      }
   +    }
   +  }
   

   Checking this checkbox will actually reload the page. I think some how we should be waiting for page to reload. Or we could get weird behaviour when the testbot is slow and we proceed before the page has reloaded.

4. Also we should probably be checking that blocks we think are visually hidden are actually hidden.

Not there yet, but uploading work in progress. Having trouble with block rows showing up that shouldn't be. Ted's points in #49 addressed as follows:

1. testSetup included in setUp as recommended.

2. Regarding

   Why aren't all 4 tests that change the order ending with $this->assertBlockOrder($this->stable_order, $msg); This should always pass, correct?

   The other tests pass a stronger test in which this condition is also true. When only the group blocks are visible when dragging, we can only assert that the order of hidden blocks is preserved and that the group blocks are reordered as expected. But the order of group blocks to hidden blocks is depends on the state of things and cannot be guaranteed by the module.

3. Seems that Mink's check() and uncheck() methods do not trigger the "onchange" attribute on the checkbox that triggers the page load, so the test added an explicit form submit. Also, added an assertJSCondition() with a jQuery() callback that simply returns true after the DOM is ready. Not sure about this.
4. Yeah, they aren't! Some blocks are visible when they shouldn't be. This is the work-in-progress part.

Corrected test for visually hidden blocks, and now it passes in local. Patch updated in 2875752-51.patch.

Latest patch is not compatible with Drupal 9, whereas the module will soon be (#3131294: Drupal 9 compatibility):

vendor/bin/drupal-check web/modules/contrib/block_visibility_groups/
37/37 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Line tests/src/FunctionalJavascript/BlockVisibilityGroupsOrderTest.php
------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
14 Class Drupal\Tests\block_visibility_groups\FunctionalJavascript\BlockVisibilityGroupsOrderTest extends deprecated class Drupal\FunctionalJavascriptTests\JavascriptTestBase:
in drupal:8.6.0 and is removed from drupal:9.0.0.
Use \Drupal\FunctionalJavascriptTests\WebDriverTestBase instead
89 Call to method setUp() of deprecated class Drupal\Tests\BrowserTestBase:
in drupal:8.6.0 and is removed from drupal:9.0.0.
Use \Drupal\FunctionalJavascriptTests\WebDriverTestBase instead
------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

[ERROR] Found 2 errors

I rebased against 2.0.x and fixed tests.

This issue is still present with the current 2.0.4 version of the module.

The MR fixes the problem and all blocks retain their relative order, even after using drag and drop, within a single visibility group.

I see there is now a 2.1.x branch, so I am not sure if there should be a new MR created for it.

This looks like fantastic work but one question— is there any reason a person using a screenreader should have to hear about all the blocks we hide from view?

That is, in:

   $form[$entity_id]['#attributes']['class'][] = 'visually-hidden';

Shouldn't we use just "hidden" here?

Committed with the mentioned change. Cherry-picked to 2.1.x. (I need an explanation of the difference between the branches and probably we should do one big catch-up and stick with one branch.)