Block position not saved when below the first block of another region

I've managed to reproduce this issue and will start work on fixing it.

block.js contained an if statement that checked if the block was directly below a region label before updating the region/weight elements. I've removed this if statement and the issue is resolved. See attached patch.

@joelpittet, thanks for the review. I will take a look at it on Thursday.

Attached patch fixes the ordering problem. I have however noticed that the block is now no longer placed at the bottom if you manually select the region using the select box. This seems to be occur because using the select box triggers a drop event on the tabledrag element (HUH?? WTF??!!) for no apparent reason. It does however seem to place the block in the second to last position consequently.

Any ideas on how to find out wether or not the row was actually dropped or updated by the select box?

Attached patch fixes the following

1. The original issue
2. The incorrect trigger of tableDrag.onDrop . (Also occurs in D7)
3. The block weights not being updated correctly when selecting a region using the select element. (Also occurs in D7)

Updated the patch to only adjust block weights in the (drop-)target region to minimise disruption in (features)config when changing block positioning.

Ignore #9. I forgot to rebase my local branch.

Fixed eslint error.

I'd like to keep the .eq() and avoid using .last() here.

Replaced the .last() with .eq().

Thanks Ankit Agrawal,

I tested your changes and everything is still working the way it should. However, since i did most of the work on this patch, someone else should review it thoroughly.

+++ b/core/modules/block/js/block.js
@@ -88,16 +88,18 @@
+        // Update the block weights

@@ -106,11 +108,12 @@
+          // Update the block weights

Nit: Comments should end with periods. It can be fixed on commit.

Simplified some jQuery stuff, made the variable and method name follow our naming standards, added JSDoc for the new function (replace the duplicated inline comments from comment above).

Was RTBC for me so if legolasbo can test it to make sure my refactor didn't break stuff, feel free to RTBC afterwards.

I only reviewed the code, still have to do manual testing.

+++ b/core/modules/block/js/block.js
@@ -112,23 +111,29 @@
+            return weight++;

Weight should be incremented before being used as a value, not after. That way we'll always have an unused weight setting before and after the blocks, which makes manually setting the weights easier for users who prefer to do it that way.

The current behaviour never sets the weight to be the maximum/minimum allowed value. Which is something the patch in #16 changes. By incrementing first we keep that part of the original behaviour in tact.

I manually changed return weight++; to return ++weight; in the patch, since I don't have developer tools on this machine.

Other than that no changes.

Oh i see! Didn't get what it was for. Attention to detaiks++ :)

Would it be possible to do a +1 on the Math.round line with a comment explaining why it's there? This would be our first legitimate use of this form of ++. And i'd rather keep weight++ so that people don't have to wander about why it's like that.

But that's really minor.

@nod_,

That would result in a "let's take the result of this Math.round operation, make it a negative number and increase it by 1". Seems really confusing and hard to document. I'd be more inclined to just document the ++weight and explain why we increment it first.

// Increment the weight before assigning it to prevent using the absolute
// minimum available weight. This way we always have an unused upper
// and lower bound, which makes manually setting the weights easier for
// users who prefer to do it that way.

That would pretty much be the same comment. It's just for consistency.

@nod_,

If you think so I'm fine with either way. Unfortunately I'm currently not on a pc which is set up for development. Could you adjust the patch as you see fit? Then @joelpittet can RTBC it.

On my phone too. Can't update

Yikes, that's pretty bad. Nice find. /me pines for JS testing :\

Committed and pushed to 8.0.x. Thanks!

Moving back to D7 for backport.

I have tested this on D7 and it seems like that the issue is not present here. The code for D7 is a bit different and blocks are correctly saved when moved to another region (as illustrated in issue summary gif). So I am closing this.