Problem/Motivation

Followup from #1039666: Placing a block in a region via select dropdown moves it to the top of the region, but it will show at the bottom

When there are no blocks in the last region (that would be the Disabled region), you can't move a block to that region using the 'Region' select drop-down.

Target region has blocks:

Target region is empty:

Steps to reproduce

  1. Move all blocks from the last region (Disabled) to other regions.
  2. Move one block back to the last region using the region select drop-down.

Proposed resolution

Fix the block placement in the last region (eg. Disabled) when there are no blocks in that region.

Remaining tasks

  1. Write a patch
  2. Review

User interface changes

When selecting 'Disabled' in a block region, the block is placed in the 'Disabled' region.

API changes

None

CommentFileSizeAuthor
#37 before.png8.91 KBshashank5563
#37 after.png9.44 KBshashank5563
#35 Before The patch.mov1.7 MBRinku Jacob 13
#35 After The patch.mov1.53 MBRinku Jacob 13
#27 2177335-27.patch1.12 KBtherealssj
#21 Step-2 Block moved to target region succesfully.png23.91 KBsamiullah
#21 Step-1 Target Region is empty.png21.83 KBsamiullah
#19 interdiff-2177335-15-19.txt629 bytesdrintios
#19 2177335-19-selecting-disabled.patch1.33 KBdrintios
#15 interdiff-2177335-10804104-15.txt765 bytesdrintios
#15 2177335-15-selecting-disabled.patch1.33 KBdrintios
#12 selecting_disabled-2177335-11.patch1.33 KBbdimaggio
#9 selecting_disabled-2177335-9.patch1.34 KBoo0shiny
#6 2177335-nonempty-region.gif68.76 KBidebr
#6 2177335-empty-region.gif31.69 KBidebr
#3 2177335-3.patch1.23 KBidebr
#1 core-js-block-admin-2177335-1.patch1.23 KBczigor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

czigor’s picture

czigor CreditAttribution: czigor commented
Status: Active » Needs review
FileSize
core-js-block-admin-2177335-1.patch1.23 KB

For me the patch from https://drupal.org/comment/6908112#comment-6908112 works. (Credits go to dozymoe.)

droplet’s picture

droplet CreditAttribution: droplet commented
Issue tags: +JavaScript
idebr’s picture

idebr CreditAttribution: idebr commented
Title: Cannot move block to an empty 'Disabled' with select » Selecting 'Disabled' does not move the block to the disabled region when there are no disabled blocks
Version: 7.x-dev » 8.0.x-dev
Issue summary: View changes
Issue tags: +Needs backport to 7.x
FileSize
2177335-3.patch1.23 KB

This still occurs in Drupal 8 as well as Drupal 7, so it should be fixed in Drupal 8 first and then backported to 7.

Attached patch ports #1 to Drupal 8.

idebr’s picture

idebr CreditAttribution: idebr commented
Issue tags: +Usability
tisteegz’s picture

tisteegz CreditAttribution: tisteegz commented

@idebr I can't seem to replicate this issue in Drupal 8? I had no blocks disabled and then using the region select list I managed to disable it correctly.

idebr’s picture

idebr CreditAttribution: idebr commented
Issue summary: View changes
FileSize
2177335-empty-region.gif31.69 KB
2177335-nonempty-region.gif68.76 KB

@tisteegz Thanks for having a look! Indeed, disabling a block still works. The problem is limited to the display in the UI: when selecting a different region, a block is moved to the bottom of the target region. However, when the target region is 'Disabled' and there are no other disabled blocks this fails. I have updated the issue summary with two screencaps to clarify the problem.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -Needs backport to 7.x +Needs backport to D7
mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented
Status: Needs review » Needs work

Needs re-roll.

oo0shiny’s picture

oo0shiny CreditAttribution: oo0shiny as a volunteer commented
FileSize
selecting_disabled-2177335-9.patch1.34 KB

Rerolled patch with code from idebr.

oo0shiny’s picture

oo0shiny CreditAttribution: oo0shiny as a volunteer commented
Status: Needs work » Needs review
Jeff Cardwell’s picture

Jeff Cardwell CreditAttribution: Jeff Cardwell as a volunteer commented

I was able to reproduce the UI issue. After applying the patch in #9, selecting -none- (when no other blocks are already present in "Disabled") for a block results in the block being moved to the appropriate "Disabled" section in the UI. The patch looks like it works.

bdimaggio’s picture

bdimaggio
he/him
Location Boston, MA
CreditAttribution: bdimaggio commented
Status: Needs review » Reviewed & tested by the community
FileSize
selecting_disabled-2177335-11.patch1.33 KB

This looks and works great in Chrome 47, FF 42, Safari 9.0.3, IE 9, IE 10. This patch just fixes up formatting.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

I've manually tested the patch and it works. I've also run the code against our eslint rules and it passes.

+++ b/core/modules/block/js/block.js
@@ -125,14 +125,22 @@
           table.find('.region-' + select[0].value + '-message').nextUntil('.region-message').eq(-1).before(row);

However, I think this line is now obsolete because of the code added by this patch. The row is placed (in the correct position) by the new code.

drintios’s picture

drintios CreditAttribution: drintios commented
Assigned: Unassigned » drintios
drintios’s picture

drintios CreditAttribution: drintios at Skilld commented
Status: Needs work » Needs review
FileSize
2177335-15-selecting-disabled.patch1.33 KB
interdiff-2177335-10804104-15.txt765 bytes

Removed line suggested by alexpott.

drintios’s picture

drintios CreditAttribution: drintios commented
Assigned: drintios » Unassigned
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Needs work 
+++ b/core/modules/block/js/block.js
@@ -125,14 +125,19 @@
-          // Find the correct region and insert the row as the last in the
-          // region.

please don't remove useful comment

drintios’s picture

drintios CreditAttribution: drintios commented
Assigned: Unassigned » drintios
drintios’s picture

drintios CreditAttribution: drintios at Skilld commented
Assigned: drintios » Unassigned
Status: Needs work » Needs review
FileSize
2177335-19-selecting-disabled.patch1.33 KB
interdiff-2177335-15-19.txt629 bytes

Returned comment

samiullah’s picture

samiullah CreditAttribution: samiullah at Srijan | A Material+ Company commented
Assigned: Unassigned » samiullah
samiullah’s picture

samiullah CreditAttribution: samiullah at Srijan | A Material+ Company commented
Issue summary: View changes
FileSize
Step-1 Target Region is empty.png21.83 KB
Step-2 Block moved to target region succesfully.png23.91 KB

After applying above patch if we move a block to empty region using dropdown, it is working fine.

Step 1

Step 2

samiullah’s picture

samiullah CreditAttribution: samiullah at Srijan | A Material+ Company commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed d841acb and pushed to 8.0.x, 8.1.x, 8.2.x. Thanks! Added myself and @samiullah to the credit for reviewing - me for the code review and @samiullah for adding screenshots.

Unfortunately no one who has worked on this issue will get any credit until the patch is ported and committed to Drupal 7 because of the current backport policy.

  • alexpott committed 866152a on 8.2.x 
    Issue #2177335 by drintios, idebr, czigor, oo0shiny, bdimaggio,...

  • alexpott committed 3a300dc on 8.1.x 
    Issue #2177335 by drintios, idebr, czigor, oo0shiny, bdimaggio,...

  • alexpott committed d841acb on 8.0.x 
    Issue #2177335 by drintios, idebr, czigor, oo0shiny, bdimaggio,...
therealssj’s picture

therealssj CreditAttribution: therealssj commented
FileSize
2177335-27.patch1.12 KB

Here is a patch for D7.

therealssj’s picture

therealssj CreditAttribution: therealssj commented
Status: Patch (to be ported) » Needs review
Triumphent’s picture

Triumphent CreditAttribution: Triumphent commented

Unfortunately, this last patch doesn't work for me (ver 7.x.) I still can't move blocks from the block list (I can however, from inside the block) and I cannot move a block in Dashboard (same thing, I need to configure the block and select the region there. That works.)

  • alexpott committed 866152a on 8.3.x 
    Issue #2177335 by drintios, idebr, czigor, oo0shiny, bdimaggio,...

  • alexpott committed 866152a on 8.3.x 
    Issue #2177335 by drintios, idebr, czigor, oo0shiny, bdimaggio,...

  • alexpott committed 866152a on 8.4.x 
    Issue #2177335 by drintios, idebr, czigor, oo0shiny, bdimaggio,...

  • alexpott committed 866152a on 8.4.x 
    Issue #2177335 by drintios, idebr, czigor, oo0shiny, bdimaggio,...
poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Issue tags: -JavaScript +JavaScript

I have tested the D7 patch #27 and it is working correctly (you need to clear the cache after applying the patch). Now it is possible to move block to the "disabled" region when there are no blocks.

Rinku Jacob 13’s picture

Rinku Jacob 13 CreditAttribution: Rinku Jacob 13 at Srijan | A Material+ Company for Drupal India Association commented
FileSize
After The patch.mov1.53 MB
Before The patch.mov1.7 MB

I think the issue got fixed now, because i have tested the issue in D7 and applied above patch. The patch was successfully applied and issue got solved. Adding Screen record for the reference . Need RTBC +1

poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Pending Drupal 7 commit

Moving carefully to RTBC (because we do not have JS tests in D7) and adding tag for the second review.

shashank5563’s picture

shashank5563 CreditAttribution: shashank5563 at Melity commented
FileSize
after.png9.44 KB
before.png8.91 KB

I have tested the D7 patch #27 and it is working fine. So I am moving it RTBC+1.

  • mcdruid committed 06438b6a on 7.x 
    Issue #2177335 by drintios, idebr, czigor, therealssj, oo0shiny,...
mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid as a volunteer and at Acquia commented
Assigned: samiullah » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs backport to D7, -Pending Drupal 7 commit

Tested this manually and confirmed the fix (once caches are cleared).

I don't think it's worth adding an empty update hook to force a cache clear for this change.

Thank you everyone that contributed!

Status: Fixed » Closed (fixed)

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