Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.#428744: Make the main page content a real block introduced an alert() that notify the user if a block can't be placed in the region it was dragged to. But right now even if the region is allowed, if the block is not dropped at first in that region the alert is triggered anyway.
The fault is in line 28 of block.js var regionRow = $(dragObject.rowObject.element).prev('tr').get(0);. This line gets the previous row of the one dragged and in line 29 it assumes it has the the class "region-messagege region-[region]message..." which is not the case if the block is not first in the region. After that the condition in line 32 becomes true and the alert is triggered.
The attached patch change line 28 so we look for the previous row (tr) with class "region-message".
On a side note, for me the regexp in line 29 it looks like it could have been done simpler.
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | 477018-28-block.js_.patch | 1.04 KB | tic2000 |
| #25 | block.js_.patch | 913 bytes | gábor hojtsy |
| #23 | block.js_.patch | 964 bytes | chx |
| #7 | regionjs.jpg | 80.68 KB | gábor hojtsy |
| block.js_.patch | 964 bytes | tic2000 |
Comments
Comment #1
tic2000 commentedI mark this critical cause right now you can't order the blocks as you'd like, unless you know before hand what is the order and add the blocks in reverse order.
The behavior now is that you get the alert but the block is always placed first in the selected region (unless really it's not allowed to place in that region)
To replicate, just try to change the order of the blocks in left sidebar on a fresh install (or even an old, but updated one).
Comment #2
tic2000 commentedbetter title
Comment #3
catchI can reproduce the bug, patch works, well found.
Comment #4
tic2000 commentedNot many people are moving blocks yet :)
Comment #5
dries commentedShould we write a test case for this?
Comment #6
catchI don't think simpletest will see the javascript alert will it?
Comment #7
gábor hojtsyGood catch, I did make this wrong in D7, right. It probably simplifies the JS if we look for the .region row instead. The region name is also included there in the classname.
Looking at the code, one more thing which should be looked at here IMHO is that region names might have an underscore (although core regions don't have any). Classnames however use dashes, so we should look at whether these are converted properly for the table row classes and if so, we should convert them here so that the select box values are properly checked.
Comment #8
tic2000 commentedRight now the code looks for the .region-message row. I could change it to look for just .region, but I think it's just the same.
If the region uses an underscore the class will be with an underscore too. The code works if the region is with an underscore or even a dash.
About a test, I don't know to write even a simple test so I don't know if a test to see if the block is dropped where it should and not moved by javascript is even possible.
Comment #9
tic2000 commentedWhat do we need here to get this committed?
Comment #10
tic2000 commentedComment #11
dropcube commentedI can not find why this feature is required and when it was added. I mean, in what situations the block can't be placed in the region it was dragged to ??? Is this defined somewhere?
Comment #12
tic2000 commentedIf you read the OP you will see a link to the issue where that feature was added and why.
The main goal of it is so you can't remove the main content block. Anyway that is not the topic of this issue.
Comment #13
dropcube commented@tic2000: Ok, to review the patch, I needed to know in what situations the block can't be placed in a region, this is the info I could not find, and was kindly asking.
Comment #14
tic2000 commentedAs I already said, but maybe not clear enough, you can't place the main content block in the "none" region (you can't disable it) and the info is in #428744: Make the main page content a real block.
The effect because of a small javascript bug is that you get a "false positive" every time you drag a block to a position that is not first in a region.
And to explain why I marked this back to CNR.
We can't test javascript (now I know a little about how to write one).
I think it's better to use region-[region_name]-message because you have delimiter before and after the region name.
As I pointed in #8 the code works if the region name has an "_" or even a "-" in it.
Comment #16
tic2000 commentedWhatever you say bot :)
Comment #18
chx commentedComment #20
tic2000 commentedComment #22
tic2000 commentedComment #23
chx commentedComment #24
chx commentedPs. please do not credit me i am just resetting testing.
Comment #25
gábor hojtsyAs I've said above, let's simplify this to .region instead of .region-message. Otherwise IMHO it looks good. Let's get this one in, this bug is quite a bad one.
Comment #26
tic2000 commentedWhy do you think using "region" is simplifying?
If you use "region"
var regionName = regionRow.className.replace(/([^ ]+[ ]+)*region-([^ ]+)-message([ ]+[^ ]+)*/, '$2');becomesvar regionName = regionRow.className.replace(/([^ ]+[ ]+)*region-([^ ]+)([ ]+[^ ]+)*/, '$2');because region-{region_var}-message is on another row so in your patch regionName variable will not get the region name but just give all classes like "region region-sidebar_first". Also if you do that you have no start and end "markers" so if some theme adds "region-collapsed" as a class before the "region-$region" class the script will get "collapsed" as being a region name.Instead of simplifying I think the change would just make the script more prone to match errors.
Comment #27
gábor hojtsy@tic2000: oh, you are right! Can you please add some docs then to the patch, so others would not have the inclination to make this "simplification" in the future. Otherwise your patch makes the blocks page work again, so I'm all with it now.
Comment #28
tic2000 commentedHere it is.
Comment #29
gábor hojtsyLooks good. At least catch and I tested the same patch minus docs. The docs look good.
Comment #30
Bojhan commentedTested, was working
Comment #31
webchickMy, that is some insanely obtuse code that could really do with some comments to explain what on earth is going on. :)
However, it wasn't invented here, and the patch does indeed fix the bug. I confirm that there is sadly no way currently to test JS. :(
Committed to HEAD. Thanks!