Problem/Motivation

When we redesigned the place block UI, we changed where blocks live before they are placed in a region. On a standard install, the disabled region is now empty.

In the usability study we observed some users looking for blocks in this region, likely due to previous experience with the Drupal 7 UI.

Proposed resolution

Remove the 'disabled' region
Replace with a 'disabled' block state, so the blocks stay in the region they are, but are visually styled to be disabled.
Add either a new "Enable" or "Disable" link to the "Operations" links on the Blocks Layout page depending on the state of the individual block.

Remaining tasks

None

User interface changes

Remove the 'disabled' region
Replace with a 'disabled' block state, so the blocks stay in the region they are, but are visually styled to be disabled.

API changes

None

Data model changes

None

Files: 

Comments

darol100’s picture

I like the disable region, but I can see how this can make new user confuse.

the blocks stay in the region they are, but are visually styled to be disabled.

I think is the way to go, in order to fix this issue. However, what I did not see in this issue is a enable/disable button. We would have to provide an easy way (button) to enable the or disable from this page.

tim.plunkett’s picture

We need some guidance on what the visual representation of a disabled block would be (when left in its region).
The API for disabling a block is already in place.

tim.plunkett’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
10.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,275 pass(es), 26 fail(s), and 0 exception(s). View
23.82 KB

Well here is some shoddy CSS just to demonstrate what needs to be done.
I've also appended (disabled) to the label just for now.

tim.plunkett’s picture

Here is a picture showing the working enable/disable links.

Status: Needs review » Needs work

The last submitted patch, 3: 2513534-block_ui-3.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,328 pass(es). View
9.12 KB

Here are some test fixes.

darol100’s picture

Status: Needs review » Reviewed & tested by the community

@tim.plunkett,

Great work, this patch makes the block page looks way better than the current block page. This patch looks good to me.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Postponed
FileSize
18.31 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2513534-7.patch. Unable to apply patch. See the log in the details link for more information. View
35.37 KB

#2508547: Placing a block with same machine name of region causes region to disappear in admin UI fixes a bug with regions in the Block UI and refactors some code, so postponing on that.

Also while this patch is functionally sound, I just made up the actual UI changes and I'm sure @bojhan or @LewisNyman will want to have their say.

LewisNyman’s picture

@tim.plunkett Nice one, thanks. I'll have a chat with Bojhan and post a patch.

tim.plunkett’s picture

This is essentially blocked on #2514998: Reduce fragility in the monolithic BlockListBuilder, but if @LewisNyman comes back with a patch sooner, we can try to work around it.

darol100’s picture

Bojhan’s picture

This is amazing, the visual styling is a little bit off - but I like that we can just use text for the state.

@Lewis lets experiment a bit with the table style.

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
18.28 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,109 pass(es). View

Reroll, still needs better CSS than #999/#eee :)

Gábor Hojtsy’s picture

Issue tags: +Needs beta evaluation

I agree the new concept makes a lot of sense and it looks sane. Given this is a normal task, committers will very likely request a beta evaluation (https://www.drupal.org/core/beta-changes).

tim.plunkett’s picture

Priority: Normal » Major

As far as update hooks are concerned, into which region will we put all of the blocks that are in "Disabled"?

lokapujya’s picture

Wouldn't this make the UI hard to use because the disabled blocks would clutter up the list? Imagine that you have 100 disabled blocks. Now you want to drag one block to a different region. The disabled blocks may make it so you need to scroll the screen to drag the block.

Edit: We could always add a hide disabled blocks feature later.

tim.plunkett’s picture

Issue tags: +Needs tests
FileSize
19.03 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,136 pass(es). View
768 bytes

In IRC we discussed preventing the user from updating if there are any blocks left in the disabled region.
However, once they update the code, it's too late, the blocks are gone from the UI.
Secondly, once they fix that, there is *nothing left to update*, so the update hook is pointless.

This patch tries a different, less ideal, approach.
It simply disables each block, and places them into the system_default_region()
However, it uses the Entity API directly, aren't we not supposed to do that? I know I saw another issue discussing this recently.

Gábor Hojtsy’s picture

@tim.plunkett: Yeah update hooks are supposed to use as little of the API as possible, **especially** where hooks may be invoked, events may be fired, etc. Because those will run the new code assuming the new structures of things, while the data is not yet updated. While for this change itself that looks like does not make sense to consider, people will run several updates at once and may run multiple version updates in one batch (if not keeping updated with each version independently). So other updates may introduce changes to data structure, etc. that this code will be problematic to run with.

tim.plunkett’s picture

FileSize
19.25 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,135 pass(es). View
1.19 KB

Here's an version using configFactory directly, but still using entity.query

LewisNyman queued 19: 2513534-block-19.patch for re-testing.

LewisNyman’s picture

Issue summary: View changes
FileSize
504 bytes
19.29 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,158 pass(es). View
475.49 KB
450.16 KB
98.7 KB

I've tweaked the colors to align with the Seven styleguide. The entire row has opacity set so all the UI elements fade into the background a little.

None of this styling applies on hover, because the UI element can still be interacted with (contrast with a disabled form element, which can't)

And yes, this passes AA contrast requirements ;-)

Bojhan’s picture

If it passes WCAG AA, then this looks good to me :)

tim.plunkett’s picture

Looks great, thanks @LewisNyman!
Now we just need update tests and a beta eval.

Manjit.Singh’s picture

Assigned: Unassigned » Manjit.Singh
Issue tags: +Needs reroll
Manjit.Singh’s picture

Assigned: Manjit.Singh » Unassigned
Issue tags: -Needs reroll
FileSize
19.46 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,015 pass(es), 230 fail(s), and 104 exception(s). View

rerolling a patch

Status: Needs review » Needs work

The last submitted patch, 25: remove_the_disabled-2513534-24.patch, failed testing.

Status: Needs work » Needs review

The last submitted patch, 8: 2513534-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: remove_the_disabled-2513534-24.patch, failed testing.

legolasbo’s picture

tim.plunkett’s picture

Yes, except this is an incremental fix, that is much much more.
Also we'd need to solve the upgrade path anyway.
Let's keep working on this.

legolasbo’s picture

@tim.plunkett,

I agree we should continue working on this aswel. Just wanted to explain why I added the relationship.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.67 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,664 pass(es). View
1.48 KB

Fixed the reroll.

Bojhan’s picture

Awesome, does this need anything else?

bill richardson’s picture

Works as expected ( Only issue i had was that it was not immediately obvious how to disable block -- i just clicked the configure button and found no way to disable block ) . Maybe the expand part of the configure button could be highlighted more.

LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
305.07 KB
469.99 KB

@bill richardson That's a good point. We should be offering a disable option on each individual block config form as well. I guess a checkbox would be consistent with with menu items?


legolasbo’s picture

Another option would be to add a checkbox to the vertical tabs at the bottom. Which would be in line with node's published checkbox and keep the form clean and simple.

tim.plunkett’s picture

No, this belongs on the listing page. People will get used to it. Menu links and nodes are not config, they don't treat "disabled" the same way, and are not analogous.

This is blocked on an upgrade path.

LewisNyman’s picture

@tim.plunkett Maybe not in code, but I don't know if a user perceives any difference between the two? Every other action you can perform on a block in a block listing you can also perform on the config form.

tim.plunkett’s picture

Search pages, filter formats, and views all have their disable functionality solely on the listing page. Blocks are to be the 4th.

I would say that menu links are wrong, and should be changed.

And nodes cannot be disabled, only unpublished, completely unrelated.

LewisNyman’s picture

Issue summary: View changes

Ok cool, as long as we aren't introducing inconsistency, I wasn't sure. I've added the remaining tasks to the issue summary.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.71 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,457 pass(es). View

Still needs tests.

tim.plunkett’s picture

FileSize
19.71 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/block/block.install. View

Still needs tests.

Wim Leers’s picture

This is looking great! Zero nits :)

tim.plunkett’s picture

We need test coverage for double escaping, it's currently broken and possibly fixed by #2568045: Make it impossible to double escape with #plain_text

Status: Needs review » Needs work

The last submitted patch, 43: 2513534-block-disabled-43.patch, failed testing.

The last submitted patch, 43: 2513534-block-disabled-43.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Needs upgrade path
FileSize
1.81 KB
21.01 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,378 pass(es), 140 fail(s), and 0 exception(s). View

Incremented the update hook. Created the updateTest (nothing in it); Ran out of time.

Status: Needs review » Needs work

The last submitted patch, 48: 2513534-48.patch, failed testing.

The last submitted patch, 48: 2513534-48.patch, failed testing.

tim.plunkett’s picture

Issue tags: +rc target
lokapujya’s picture

Status: Needs work » Needs review
FileSize
747 bytes
21.43 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,855 pass(es), 4 fail(s), and 0 exception(s). View

fix one of the failing tests by rerolling the tests for some changes in head, possibly from the disabled block issue.

Status: Needs review » Needs work

The last submitted patch, 52: 2513534-52.patch, failed testing.

The last submitted patch, 52: 2513534-52.patch, failed testing.

catch’s picture

This looks more rc deadline/minor version target to me.

joelpittet’s picture

Assigned: Unassigned » webchick
Issue tags: +Needs product manager review

Assigning to @webchick as this is Drupal product facing to see if it can be "rc eligible" or target.

lokapujya’s picture

Fix failures.

lokapujya’s picture

Status: Needs work » Needs review
lokapujya’s picture

lokapujya’s picture

Status: Needs review » Needs work

Found a small bug. Not sure if we need to fix it. Needs work anyway for the upgrade test.

A block that is enabled before running the upgrade can become disabled.
condition: A block is placed more than once. It is enabled in a region and the second is disabled.

webchick’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: -rc deadline, -Needs product manager review

While I definitely still think we should do this, I can't see how we could possibly do this now, during RC vs. in a minor release. It breaks UI, strings, documentation, adds an update hook, all kinds of things.

Moving to 8.1.x unless I'm missing something.

webchick’s picture

Assigned: webchick » Unassigned
lokapujya’s picture

Added some progress on an upgrade path test.

lokapujya’s picture

Status: Needs work » Needs review
lokapujya’s picture

lokapujya’s picture

Tests are not running for this issue.

tim.plunkett’s picture

The issue is marked for 8.1.x, which has 2+ year old code, so tests don't run.
If you'd like, feel free to upload patches to an issue like #2374115: IGNORE: Patch testing issue just to get test results.

almaudoh’s picture

Status: Needs review » Needs work
almaudoh’s picture

Status: Needs work » Needs review

Will testbot pickup the patches...?

The last submitted patch, 63: 2513534-61.patch, failed testing.

The last submitted patch, 65: 2513534-65-update-test-only.patch, failed testing.

imrancluster’s picture

Hello everyone,

I found 'disabled' section in "Manage form display" and "Manage display". For some content types we may have to create more fields. Can we build something this new feature for those area?

tim.plunkett’s picture

@imrancluster that is unrelated to the Block UI

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.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

Re-rolled against 8.2.x to start work on remaining tasks

Manjit.Singh’s picture

FileSize
330.31 KB

I can still found the regions that had not block assigned. Please check the screenshot.

block

correct me if i am wrong.

tim.plunkett’s picture

@Manjit.Singh I don't understand, that is 100% supposed to happen.
We're only removing the "Disabled" section of this page.

tedbow’s picture

FileSize
39.22 KB

@Manjit.Singh I am new to this issue but from reading of the issue summary and looking at the code this issue is not to remove regions from the Block Layout page that currently don't have any blocks assigned to them but rather to remove Disabled region from the block layouts page.

Here is an unpatched 8.1.x sight layout page
disabled region

Regions with no blocks should still show up. That way you still have the "Place Block" button.

tedbow’s picture

Removing non-relevant task and tags.

There already is an upgrade path block_update_8004 and tests for the upgrade path, BlockRemoveDisabledRegionUpdateTest

I am looking over the code but looks like there no remaining tasks and this code be RTBC.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

This looks good. Changing to RTBC!

I really like this change!

The last submitted patch, 8: 2513534-7-combined-do-not-test.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/block/src/BlockInterface.php
@@ -15,11 +15,6 @@
-   * Denotes that a block is not enabled in any region and should not be shown.
-   */
-  const BLOCK_REGION_NONE = -1;

Okay one more thing to discuss.
Are we allowed to remove this constant in 8.y.x?

Also this obsoletes #2724283: BlockInterface::BLOCK_REGION_NONE is integer, Block::$region is string

Wim Leers’s picture

#82: I wonder if we should keep that and then move this:

+++ b/core/modules/block/src/BlockListBuilder.php
@@ -366,12 +367,6 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
-      if ($entity->getRegion() == BlockInterface::BLOCK_REGION_NONE) {
-        $entity->disable();
-      }
-      else {
-        $entity->enable();
-      }

into Block::save(), which would then effectively retain BC?

tim.plunkett’s picture

Restoring the constant, deprecating it, and merging in the test coverage from #2724283: BlockInterface::BLOCK_REGION_NONE is integer, Block::$region is string.

tedbow’s picture

I think #83 makes sense because there could be custom that is trying to disable blocks currently by moving them to the disabled region.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
30.14 KB
2.15 KB

#83 I disagree, I don't think we should change Block::save that way.

We can just rely on the existing functionality of block_rebuild.

Manjit.Singh’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/block.install
@@ -124,5 +124,29 @@ function block_update_8003() {
 /**
+ * Disables blocks that are placed into the "disabled" region.
+ */

Seems like it should be "Disable blocks" rather than "Disables blocks".

tim.plunkett’s picture

Status: Needs work » Needs review

Leaving at needs review for a substantive review.

lokapujya’s picture

While in review, any thought regarding comment #16: The disabled blocks could clutter the UI? I don't want to delay progress, but it's something to consider.

tim.plunkett’s picture

Honestly, I consider disabling a block to extremely temporary. Creating new blocks is simple, so I would generally delete any block I left disabled for a significant period of time.

Maninders’s picture

Just review the #86 patch and it's working fine. 'Disabled' regions is no more available. Now enable, disable link is coming with every configure dropdown. Screenshot is attached for your reference.

Maninders’s picture

Status: Needs review » Reviewed & tested by the community
yoroy’s picture

Status: Reviewed & tested by the community » Needs review

Thanks for testing the patch @Maninders, but @tim.plunkett in #90 set it to "needs review" for a thorough review of the actual code changes. Setting back to needs review for that.

tedbow’s picture

Reviewing from #86

Seems like it should be "Disable blocks" rather than "Disables blocks".

@Manjit.Singh nice catch! I thought you were wrong but checked: https://www.drupal.org/coding-standards/docs#updateN
And says

Note that the first line should start with an imperative verb, so it will make sense to people running update.php.

+++ b/core/modules/block/src/Entity/Block.php
@@ -28,11 +28,14 @@
+ *     "disable" = "/admin/structure/block/manage/{block}/disable",

No trailing comma for annotations

+++ b/core/modules/block/block.module
@@ -144,17 +144,12 @@ function block_rebuild() {
+          drupal_set_message(t('The block %info was assigned to the invalid region %region and has been disabled.', ['%info' => $block_id, '%region' => $block->getRegion()]), 'warning');

In my patch I am changing this to not display this message if the BlockInterface::BLOCK_REGION_NONE was used to disable the block. It seems if this is just deprecated we should not show an error message.

Looks Great!

Here is patch with those small changes. It these are ok I think RTBC

tim.plunkett’s picture

Annotations can have trailing commas as of late 2013: https://github.com/doctrine/annotations/pull/11

That change related to the message needs test coverage. I'll fix that shortly.

tim.plunkett’s picture

I did more thinking on the change to block_rebuild in #94. I don't think we should add new usages of the constant. -1 is now an invalid region, I don't think it's wrong to display that. The functionality is the same as in HEAD regardless of the message.
Also restoring the trailing comma.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@tim.plunkett ok I see the point about the message. Looks good

I also have updated the annotations doc page to reflect the changes to trailing spaces https://www.drupal.org/node/1882526

RTBC!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/block/block.install
@@ -124,5 +124,29 @@ function block_update_8003() {
+  $block_ids = \Drupal::entityManager()
+    ->getStorage('block')
+    ->getQuery()
+    ->condition('region', -1)
+    ->execute();

I don't think we should use the entity API in hook_update_N(). I think this fix could be done in a post update and using the config entity API.

Also should we detect and handle blocks saved with REGION_NONE in the config entity - since nothing is supposed to have REGION_NONE anymore but once the update is run it is perfectly possible to save such a block through the API and then on a hook_rebuild it'll be moved to the default region.

tim.plunkett’s picture

+++ b/core/modules/block/block.module
@@ -144,17 +144,12 @@ function block_rebuild() {
+          $block
+            ->setRegion(system_default_region($theme))
+            ->disable()
+            ->save();

On hook_rebuild it will be moved to the default region but also disabled.

I'll look into the post_update change.

andypost’s picture

Also \Drupal::entityManager() is *deprecated*

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Working on this.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.41 KB
31.21 KB

@alexpott pointed out that due to block_rebuild(), we don't actually need an update path, just to trigger a cache flush by putting in an empty post_update hook. Additionally, block_rebuild was not handling disabled blocks in the wrong regions. Fixed that and the test coverage. Finally, added a Block::preSave() to address concerns about contrib/custom modules expecting $block->setRegion(-1)->save() to work even without a cache rebuild.

Status: Needs review » Needs work

The last submitted patch, 102: 2513534-block-102.patch, failed testing.

Bojhan’s picture

Super excited if we can get this into 8.2 :)

tim.plunkett’s picture

So we have a major problem with that addition of Block::preSave().
Migrate has a simple mapping of region names, but those differ for each theme. For example, Bartik no longer has a region named "footer".
Migrate has no ability to provide different mappings per theme.
Furthermore, it tries to migrate things for themes like Bluemarine, which no longer exists. system_region_list() will always fail for that in tests unless we ship with a fake theme named bluemarine, so not sure what to do there.

So either we're blocked on Migrate being fixed, or we go without the Block::preSave() fix.

The last submitted patch, 105: 2513534-block-104-option1.patch, failed testing.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Option 1 is blocked on #2753939: BlockRegion process plugin should be source theme-aware and also another issue that @phenaproxima hasn't opened yet either.
Option 2 is just reverting the one Block::preSave() hunk introduced in #102.

alexpott’s picture

I think that block migrations are fundamentally broken see #2753939-2: BlockRegion process plugin should be source theme-aware

We could just add @todo's to fix the migration tests because migrations don't make make sense when block_rebuild() is going to come along and change what you've just migrated.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

That's a reasonable workaround for me.

+++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
@@ -123,7 +124,9 @@ public function testBlockMigration() {
+    // @todo https://www.drupal.org/node/2753939 This block is the footer region
+    // but Bartik in D8 does not have this region.

@@ -138,7 +141,9 @@ public function testBlockMigration() {
+    // @todo https://www.drupal.org/node/2753939 The bluemarine theme does not
+    // exist.

+++ b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockTest.php
@@ -109,7 +110,9 @@ public function testBlockMigration() {
+    // @todo https://www.drupal.org/node/2753939 This block is the footer region
+    // but Bartik in D8 does not have this region.

The second line should be indented.
That can be fixed on commit.

alexpott’s picture

Issue tags: +Needs change record

We need a CR because as the recent patches show some tests for contrib might break.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

For #110

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Posted CR and restored RTBC status: https://www.drupal.org/node/2778489

tim.plunkett’s picture

Fixed those nits just to make it easier to get this in.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 113: 2513534-block-113.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
43.76 KB
899 bytes

Updating test coverage added recently.

  • catch committed e1ef487 on 8.3.x
    Issue #2513534 by tim.plunkett, lokapujya, tedbow, LewisNyman, Manjit....
catch’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x, thanks!

I'm not seeing anything that makes this time-sensitive for the 8.2.x beta (i.e. the issue summary does not mention anything), so leaving it fixed against 8.3.x for now.

Gábor Hojtsy’s picture

Published change record at https://www.drupal.org/node/2778489

chx’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

Issue tags: +8.3.0 release notes

This is a longstanding UX issue, maybe worth a release notes mention?

lokapujya’s picture

Disabled blocks go to the end of the list; They don't stay in order by weight. However, when you enable the block it goes back in order using the previous weight. Is that how it should be?

tim.plunkett’s picture

It is by design, \Drupal\block\Entity\Block::sort() does three things:

    // Separate enabled from disabled.
    // Sort by weight.
    // Sort by label.

Changing that would need a new issue.