?
avatar joomdonation
joomdonation
16 May 2021

Steps to reproduce the issue

While reviewing PR #33909, we found potential issue with items re-ordering using drag and drop in Joomla 4. Main issue are:

  • On page 2, page 3...., ordering of items are re-started from 1, so it causes wrong ordering saved if we change ordering of items from page 2, page 3.....
  • Also, from @Ruud68

And to further thicken the plot: in J3 ordering is using negative numbers, so e.g. when you set field one below field 2 then field one gets ordering of 1 and field 2 gets ordering of -1.
It should also be tested how ordering of fields will handle these when upgrading to 4.0 and then reordering n 4. Will it correctly handle the negative numbers etc. as the logic of ordering has apparently changed.

avatar joomdonation joomdonation - open - 16 May 2021
avatar joomla-cms-bot joomla-cms-bot - change - 16 May 2021
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - labeled - 16 May 2021
avatar Fedik
Fedik - comment - 16 May 2021

hm, it seems PHP part for saveOrder is not changed,
on quick look it the same in J3 and J4 for AdminController::saveOrderAjax(), AdminController::saveorder(), AdminModel::saveorder()

avatar joomdonation
joomdonation - comment - 16 May 2021

Yes, I think only client part was changed, so should not be hard to fix :). As mentioned, page 2, page 3 restart item ordering from 1 and it causes the issue. I guess instead of re-start with 1, it should be started with min ordering of the items on that page and it should be fine.

avatar richard67 richard67 - change - 16 May 2021
Labels Added: ?
avatar richard67 richard67 - labeled - 16 May 2021
avatar richard67
richard67 - comment - 16 May 2021

Set release blocker label as discussed with other maintainers.

avatar Fedik
Fedik - comment - 16 May 2021

okay, so it is JS, in J3 the order is recalculated before submit in

if (count > 1) {
//recalculate order number
if (ui.originalPosition.top > ui.position.top) //if item moved up
{
if (ui.item.position().top != ui.originalPosition.top){
$('[name="order[]"]', ui.item).attr('value', parseInt($('[type=text]', ui.item.next()).attr('value')));
}
$(range).each(function () {
var _top = $(this).position().top;
if ( ui.item.get(0) !== $(this).get(0)){
if (_top > ui.item.position().top && _top < ui.originalPosition.top + ui.item.outerHeight()) {
if (sortDir == 'asc') {
var newValue = parseInt($('[name="order[]"]', $(this)).attr('value')) + 1;
} else {
var newValue = parseInt($('[name="order[]"]', $(this)).attr('value')) - 1;
}
$('[name="order[]"]', $(this)).attr('value', newValue);
}
}
});
} else if (ui.originalPosition.top < ui.position.top) {
if (ui.item.position().top != ui.originalPosition.top){
$('[name="order[]"]', ui.item).attr('value', parseInt($('[name="order[]"]', ui.item.prev()).attr('value')));
}
$(range).each(function () {
var _top = $(this).position().top;
if ( ui.item.get(0) !== $(this).get(0)){
if (_top < ui.item.position().top && _top >= ui.originalPosition.top) {
if (sortDir == 'asc') {
var newValue = parseInt($('[name="order[]"]', $(this)).attr('value')) - 1;
} else {
var newValue = parseInt($('[name="order[]"]', $(this)).attr('value')) + 1;
}
$('[name="order[]"]', $(this)).attr('value', newValue);
}
}
});
}
}

in J4 it always start fro 1

avatar joomdonation
joomdonation - comment - 16 May 2021

That's right. And keep in might that in J3, we do not change ordering of all items like we are doing in J4. Change ordering of all items could cause performance issue during saving process (note that we also have row in same group, and ordering value of row from different groups should be be changed)

avatar Fedik
Fedik - comment - 16 May 2021

It changes the order for the active page, if I right understood.
But need to check how it doing the calculation, it not that easy to get what there is written ?

avatar joomdonation
joomdonation - comment - 16 May 2021

Yes, but as I understand, it does not change ordering value of every items on the page. Only certain items (especially in case there are items from different groups - for example fields from different fields group)

avatar joomdonation
joomdonation - comment - 16 May 2021

But need to check how it doing the calculation, it not that easy to get what there is written

Yes, not easy to understand. I will said that it is JS code and we will ask you @Fedik and @dgrammatiko for this difficult task, lol

avatar dgrammatiko
dgrammatiko - comment - 16 May 2021

I think it's a miracle that this thing even works for some cases...

I will revisit the code once I'm feeling a bit better (kinda sick the last few days, so the brain is not there 100%) unless @Fedik is faster. The idea is simple you have an array of all the rows before the d&d and another after and you diff the changes

avatar Ruud68
Ruud68 - comment - 17 May 2021

as a reminder for testing / debugging: there are two scenarios: first sort (where all fields get an order set because all fields order = 0) and second order (where all fields already have an order and only changed fields get an order set)

When all fields have an order set, then ordering on a sub page should work correct, the issue here is that when no ordering is set and thus the ordering on the other pages is set to 0.

This could be solved by NOT setting the default sort order to 0 but to e.g. the primary key (server side)

In theory that is :)

avatar joomdonation
joomdonation - comment - 17 May 2021

Actually, not that simple if you look at the script which calculate the order value of each record :) which @Fedik mentioned earlier #33917 (comment)

In theory:

  1. We should minimize the number of records which need to update ordering value.
  2. Only the record of the dragging element and the records between it's original position and it's dragend position should have the ordering value updated
  3. If the record belong to group (controlled by data-draggable-group attribute), then only the records from same group (and meet the requirement point 2 above) will need to be updated.

As I understand, it is complicated like that for performance issue (think about change ordering of items from a table with 100K records as you mentioned). The process not only update ordering value of the records but also recalculate ordering value for other related records (for example, from same category) https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/MVC/Model/AdminModel.php#L1548-L1625

avatar dgrammatiko
dgrammatiko - comment - 17 May 2021

Only the record of the dragging element and the records between it's original position and it's dragend position should have the ordering value update

Ok, so this is the difference, in J3 the reordering is basically swapping the dragged element with the one in the drop position but with the current script the dragged element is inserted before the drop position (ie real reordering, not swapping)
If we want to keep the swapping behaviour it should be straightforward:

// in the `drag` event get the original array of rows and the position of the dragged element
let rows = [];
let draggedElementIndex = 5;

// A simple swap function
function swap(input, index_A, index_B) {
    let temp = input[index_A];

    input[index_A] = input[index_B];
    input[index_B] = temp;
}

// On drop get the drop element position
let droppedElementIndex = 5;


// Do the swap 
swap(rows, droppedElementIndex, draggedElementIndex);

//Apply the changes to the DOM

// Do the ajax...
avatar joomdonation
joomdonation - comment - 17 May 2021

Guess I don't understand JS code or it is not that simple :(. Let's say we are on the page with 20 records ordered in ASC order and you want to move a record from position 2 to position 5 (so basically, the record is moved down). Here is how I understand the ordering value should be changed:

  • The record from first position (any records before dragged element) will have it's ordering value un-changed
  • The record from position 2 (the dragged element) will keep ordering value of the record in position 5.
  • The record from positions 3 and 4, and 5 (between old and new position of the dragged element) will have ordering value decreased by 1 (move up one value)
  • There records from position 6 to 20 will have it ordering value un-changed.

If the item is moved up, the logic would be different. That's how I see it works in J3. We have to care about the case items are ordered in DESC order, too. That's why I said we should not change ordering value of all records on the current page.

avatar dgrammatiko
dgrammatiko - comment - 17 May 2021

The rearrange code in J3 is

this.rearrangeOrderingValues = function (sortableGroupId, ui) {
var range;
if (sortableGroupId) {
root.sortableRange = $('tr[' + ops.orderingGroup + '=' + sortableGroupId + ']');
} else {
root.sortableRange = $('.' + ops.sortableClassName);
}
range = root.sortableRange;
var count = range.length;
var i = 0;
if (count > 1) {
//recalculate order number
if (ui.originalPosition.top > ui.position.top) //if item moved up
{
if (ui.item.position().top != ui.originalPosition.top){
$('[name="order[]"]', ui.item).attr('value', parseInt($('[type=text]', ui.item.next()).attr('value')));
}
$(range).each(function () {
var _top = $(this).position().top;
if ( ui.item.get(0) !== $(this).get(0)){
if (_top > ui.item.position().top && _top < ui.originalPosition.top + ui.item.outerHeight()) {
if (sortDir == 'asc') {
var newValue = parseInt($('[name="order[]"]', $(this)).attr('value')) + 1;
} else {
var newValue = parseInt($('[name="order[]"]', $(this)).attr('value')) - 1;
}
$('[name="order[]"]', $(this)).attr('value', newValue);
}
}
});
} else if (ui.originalPosition.top < ui.position.top) {
if (ui.item.position().top != ui.originalPosition.top){
$('[name="order[]"]', ui.item).attr('value', parseInt($('[name="order[]"]', ui.item.prev()).attr('value')));
}
$(range).each(function () {
var _top = $(this).position().top;
if ( ui.item.get(0) !== $(this).get(0)){
if (_top < ui.item.position().top && _top >= ui.originalPosition.top) {
if (sortDir == 'asc') {
var newValue = parseInt($('[name="order[]"]', $(this)).attr('value')) - 1;
} else {
var newValue = parseInt($('[name="order[]"]', $(this)).attr('value')) + 1;
}
$('[name="order[]"]', $(this)).attr('value', newValue);
}
}
});
}
}
}

We could just copy this over (I don't remember why I skipped that and rewrote the logic)

avatar Ruud68
Ruud68 - comment - 17 May 2021
* There records from position 6 to 20 will have it ordering value un-changed.

True only on second+ ordering, on initial order these all have vaue 0 and will break the order

avatar Fedik
Fedik - comment - 17 May 2021

That's why I said we should not change ordering value of all records on the current page.

you have to.
example on first page:
1
2
3
4
5

You move 1 to the end:
2
3
4
5
1

So all previous items need to be shift:
2 = 1
3 = 2
4 = 3
5 = 4
1 = 5 actual index

In the resulting DIFF an every item will be changed.

Then on second page:
6
7
8
9
10

You move 9 up:
6
9
7
8
10

You again recalculate all:
6 = 6
9 = 7
7 = 8
8 = 9
10 = 10

Here in the diff will be changed only 3 element.

That basically what @dgrammatiko wrote.

Of course we have to skip non related groups, but the idea is the same.

avatar joomdonation
joomdonation - comment - 17 May 2021

you have to

@Fedik in your example for second page, we only need to change ordering of 3 items, not all items. So what I said is correct:

That's why I said we should not change ordering value of all records on the current page

avatar Fedik
Fedik - comment - 17 May 2021

The thing getting more fun when you have a filter enabled.
So example your first page become:

1
3
4
7
10

And you move 1 item to the end:

3
4
7
10
1

You recalculate all:

3 = 1
4 = 2
7 = 3
10 = 4
1 = 5

All element changes.
You disable filter and you have totaly different ordering with doubled elements:
1
1
2
2
3
3
4
4
....

~But joomla 3 also ignores this case I guess ~
Hm not really, joomla 3 seems just doing +1/-1 depend from direction.

avatar Fedik
Fedik - comment - 17 May 2021

That's why I said we should not change ordering value of all records on the current page

More correct would be: we change ordering only for "changed elements". Because amount of changed elements may be equal to whole page ;)

avatar joomdonation
joomdonation - comment - 17 May 2021

Somehow, we would have to find the items between the original position of the element and it's dropped position and apply the ordering change (+1, -1 depends on move up, move down and how items are sorted in ASC or DESC order)

avatar Ruud68
Ruud68 - comment - 17 May 2021

The thing getting more fun when you have a filter enabled.

That is why (imo) the initial ordering should be set to the primary key AND the new order should not be 'recalculated' (read: new values) client side, but should be based on the existing order ids. That way, sub pages and filtering have no negative impact

what is shown (subpage or filter) is pushed as an array to the server, new order is done on INDEX (of that array) and not on values. That way the values will not get doubled / 'corrupted'

avatar Fedik
Fedik - comment - 17 May 2021

the initial ordering should be set to the primary key

It will not help much, you still get doubled items, after reorder of filtered data.
Filtered items where Order is set from primary key:
11
14
15
32

You move 11 to end:
14
15
32
11

Recalculate:
14
15
32
11 = 33

In the end you have 2 (or more) items at position 33.
Additionally it may be harder to do calculation

avatar dgrammatiko
dgrammatiko - comment - 17 May 2021

Somehow, we would have to find the items between the original position of the element and it's dropped position and apply the ordering change (+1, -1 depends on move up, move down and how items are sorted in ASC or DESC order)

The data exists there, you have the original array of the elements on drag start and the final one on drag end. So basically it's all about array diffing and then a forEach to apply the changes

avatar Fedik
Fedik - comment - 17 May 2021

subpage and filter is a different things, you always will get doubled row for "filtered", unless you will not apply more complex algorithm, to resort whole data in database.

But that is not big problem, I guess. More important that sorting works correctly "per page".

avatar Ruud68
Ruud68 - comment - 17 May 2021

subpage and filter is a different things, you always will get doubled row for "filtered", unless you will not apply more complex algorithm, to resort whole data in database.

But that is not big problem, I guess. More important that sorting works correctly "per page".

No, you will only get double rows if you assign NEW values to sort order, that is why I propose to use index as pointers to existing values as source for order instead of new values.

avatar dgrammatiko
dgrammatiko - comment - 17 May 2021

I'm thinking that we might have to find index of drag position (should be easy) and then index of the dropped position

Actually Draggula is very helpful here as each event ('drag', 'drop') provide us the element, eg:

      .on('drag', (draggedElement) => {
// Get the index of draggedElement from the rows array
      })
      .on('drop', (dropElement) => {
     // Get the index of dropElement from the (new? Idk if the event is fired before or after the DOM update) rows array
// Diff the arrays
// Do the ajax
     })
avatar Fedik
Fedik - comment - 17 May 2021

I don't get how array diff would help in this case.

Check my example for Second Page with 3 changed elements.
before [6, 7, 8, 9, 10]
after [6, 9, 7, 8, 10]
diff [9, 7, 8] which will be re-indexed to [7,8,9] and send

avatar dgrammatiko
dgrammatiko - comment - 17 May 2021

@Fedik I think he means instead of using 0 indexed array to have an array of the actual id numbers

What we have

// First element
<input type="text" name="order[]" size="5" value="" class="width-20 text-area-order hidden" data-order="1">

Proposed:

// First element's id = 100
<input type="text" name="order[]" size="5" value="" class="width-20 text-area-order hidden" data-order="100">
avatar Ruud68
Ruud68 - comment - 17 May 2021

correct :) that way you will never have to create a new value with the risk of double etc.
Only issue then left is the ordering value of 0 which in J3 is worked around by having negative ordering values (e.g. -1).
As said that could be 'fixed' by initially setting the order value not to 0 but to the primary key value. Not sure what that would mean for upgrading from 3.x though

avatar dgrammatiko
dgrammatiko - comment - 17 May 2021

Not sure what that would mean for upgrading from 3.x though

This attribute is created client-side (js) so it shouldn't break anything, but getting the ID number might be tricky?

<input type="text" name="order[]" size="5" value="<?php echo $item->ordering; ?>" class="width-20 text-area-order hidden">

Edit:
if there is a check input then it's easy, just get the value from there: <input class="form-check-input" autocomplete="off" type="checkbox" id="cb0" name="cid[]" value="5" onclick="Joomla.isChecked(this.checked);">

avatar Ruud68
Ruud68 - comment - 17 May 2021

that way you will never have to create a new value with the risk of double etc.

Check my example above #33917 (comment)
Where element 11 moved at the end, and changed to 33,
however on next page may already exist element with order 33 (which set from ID).
So you get 2 element at position 33.

That not should happen: order value is set on item creation, so when item with ID is created there cannot be another item that has order value of 33, unless you upgrade from 3.x where order values could be anything...
because it is based on the (primary) key, even deleting e.g. 33 will never use 33 again

avatar Ruud68
Ruud68 - comment - 17 May 2021

Not sure what that would mean for upgrading from 3.x though

This attribute is created client-side (js) so it shouldn't break anything, but getting the ID number might be tricky?

<input type="text" name="order[]" size="5" value="<?php echo $item->ordering; ?>" class="width-20 text-area-order hidden">

Edit:
if there is a check input then it's easy, just get the value from there: <input class="form-check-input" autocomplete="off" type="checkbox" id="cb0" name="cid[]" value="5" onclick="Joomla.isChecked(this.checked);">

or just echo it in server side?

avatar joomdonation
joomdonation - comment - 17 May 2021

I think for now, the priority should be making client side works in the same way with Joomla 3. Please note that after the ordering is saved, the server side also have logic to re-calculate ordering of the related items , see https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/MVC/Model/AdminModel.php#L1615-L1619

So the issue you are discussing about might be invalid. Or even it is valid, it should be discussed in separate issue. For now, just please find a way to make it works in the same way with Joomla 3.

avatar dgrammatiko
dgrammatiko - comment - 17 May 2021

or just echo it in server side?

Yes, that's easier but also a B/C break. We can have some logic there like, get data-id if exists if not check for the checkbox input in the same row and disable d&d if it doesn't;t exist. So no B/C break

avatar Fedik
Fedik - comment - 17 May 2021

so when item with ID is created there cannot be another item that has order value of 33

okay, and how then ordering should work?
you see that after moving/sorting the element around this number will changes (many times back and forth), and I already show how you will get a double.

@dgrammatiko ignore id, they will be picked from row cid[] field, we just have to set correct order[]

avatar Fedik
Fedik - comment - 17 May 2021

I would say +1/-1.

Setting ID as default may help avoid negative numbers, but this changes behavior, where each new Item will be at end of the list. While current behavior is when each new Item is at top of the list. Or I am wrong?

In the end a negative number still a number ;)

avatar joomdonation joomdonation - change - 18 May 2021
Status New Closed
Closed_Date 0000-00-00 00:00:00 2021-05-18 09:27:49
Closed_By joomdonation
Labels Added: ?
Removed: ?
avatar joomdonation joomdonation - close - 18 May 2021
avatar joomdonation
joomdonation - comment - 18 May 2021

PR #33937 should address this concern. So I'm closing this issue. Will re-open if that PR is not accepted.

avatar richard67 richard67 - change - 20 May 2021
Labels Removed: ?
avatar richard67 richard67 - unlabeled - 20 May 2021

Add a Comment

Login with GitHub to post a comment