search

primefaces / primefaces

Ultimate Component Suite for JavaServer Faces
http://www.primefaces.org
MIT License
1.79k stars 762 forks source link

Datatable: Reorder with RowExpansion doesn't work for expanded rows #6557

Closed klaffke closed 3 years ago

klaffke commented 3 years ago

Describe the defect I have a p:dataTable with draggableRows="true", a p:rowToggler column and a p:rowExpansion. When I expand one of the columns and then drag it to the last position, only the table row (without the expansion) gets dropped. The expanded row stays at the top. Also, the rowReorder event doesn't get fired.

Works: 1 2

Doesn't work: 3 4

Reproducer I have attached a reproducer: primefaces-test-masterReproducer.zip

Environment:

To Reproduce Steps to reproduce the behavior:

  1. Expand the row with the index 0
  2. Drag this row to the last position

Expected behavior

Example XHTML

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml"
      xmlns:ui="http://java.sun.com/jsf/facelets"
      xmlns:f="http://java.sun.com/jsf/core"
      xmlns:p="http://primefaces.org/ui"
      xmlns:h="http://java.sun.com/jsf/html">

    <h:head>
        <title>PrimeFaces Test</title>
        <h:outputScript name="test.js" />
    </h:head>
    <h:body>

        <h1>#{testView.testString}</h1>
        <h:form id="frmTest">
            <p:growl id="msgs" showDetail="true" skipDetailIfEqualsSummary="true" />

            <p:dataTable var="str" value="#{dtReorderView.strings}" draggableRows="true">
                <p:ajax event="rowReorder" listener="#{dtReorderView.onRowReorder}" update=":frmTest:msgs" />
                <f:facet name="header">
                    Draggable Rows
                </f:facet>

                <p:column style="width:16px">
                    <p:rowToggler />
                </p:column>

                <p:column headerText="Index">
                    <h:outputText value="#{dtReorderView.strings.indexOf(str)}" />
                </p:column>

                <p:column headerText="String">
                    <h:outputText value="#{str}" />
                </p:column>

                <p:column headerText="Hash">
                    <h:outputText value="#{car.hashCode()}" />
                </p:column>

                <p:rowExpansion>
                    <p:panelGrid columns="2" columnClasses="label,value" style="width:300px">

                        <h:outputText value="String:" />
                        <h:outputText value="#{str}" />

                        <h:outputText value="Hashcode" />
                        <h:outputText value="#{str.hashCode()}" />
                    </p:panelGrid>
                </p:rowExpansion>

            </p:dataTable>
        </h:form>

    </h:body>
</html>

Example Bean

/*
 * Copyright 2009-2014 PrimeTek.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package org.primefaces.test;

import org.primefaces.event.ReorderEvent;

import javax.annotation.PostConstruct;
import javax.enterprise.context.RequestScoped;
import javax.faces.application.FacesMessage;
import javax.faces.context.FacesContext;
import javax.inject.Named;
import java.io.Serializable;
import java.util.ArrayList;
import java.util.List;

@Named("dtReorderView")
@RequestScoped
public class ReorderView implements Serializable {

    private List<String> strings;

    @PostConstruct
    public void init() {
        strings = new ArrayList<>();
        strings.add("Primefaces rocks");
        strings.add("Primefaces really rocks");
        strings.add("!!!");
    }

    public List<String> getStrings() {
        return strings;
    }

    public void onRowReorder(ReorderEvent event) {
        FacesMessage msg = new FacesMessage(FacesMessage.SEVERITY_INFO, "Row Moved", "From: " + event.getFromIndex() + ", To:" + event.getToIndex());
        FacesContext.getCurrentInstance().addMessage(null, msg);
    }
}
melloware commented 3 years ago

There are definitely multiple issues here. But one of the issues is because this introduces new rows when you have it expanded the indexes are off. That can be fixed with this in makeRowsDraggable..

            update: function(event, ui) {
                var fromIndex = ui.item.data('ri'),
                itemIndex = ui.item.index();

                // #6557 must not count expanded rows
                if ($this.cfg.rowExpandMode) {
                   var expandedRows = $this.tbody.children('.ui-expanded-row-content').length;
                   itemIndex = itemIndex - expandedRows;
                }

                var toIndex = $this.paginator ? $this.paginator.getFirst() + itemIndex: itemIndex;

That fixes the issue of which row was dragged properly and makes the AJAX fire but that doesn't fix the real problem of closing the original row's expander and having the dragged row show as un-expanded.

melloware commented 3 years ago

As with all things in the Datatable some features were not designed or intended to go together. This may be one of them.

klaffke commented 3 years ago

Hi melloware,

thanks for the really fast reply!

Will the fix for the IndexOutOfBoundsException be part of the 9.0 release?

Regarding the other problem, I noticed that the row expansions are actually table rows (tr), separate from their origin row. Could the event that fires when you drag a row, be modified, so that both the row and its expansion get selected for dragging?

Regards and again, many thanks!

melloware commented 3 years ago

Yep that is the hard part. I am still working on some kind of way to make it work.

klaffke commented 3 years ago

Awesome, thanks!

melloware commented 3 years ago

@klaffke can you try the following:

  1. Add this JS to your application JS file that loads after Primefaces.

    if (PrimeFaces.widget.DataTable) {
PrimeFaces.widget.DataTable.prototype.makeRowsDraggable = function() {
    var $this = this,
        draggableHandle = this.cfg.rowDragSelector || 'td,span:not(.ui-c)';

    this.tbody.sortable({
        placeholder: 'ui-datatable-rowordering ui-state-active',
        cursor: 'move',
        handle: draggableHandle,
        appendTo: document.body,
        start: function(event, ui) {
            ui.helper.css('z-index', PrimeFaces.nextZindex());
        },
        helper: function(event, ui) {
            var cells = ui.children(),
                helper = $('<div class="ui-datatable ui-widget"><table><tbody class="ui-datatable-data"></tbody></table></div>'),
                helperRow = ui.clone(),
                helperCells = helperRow.children();

            for (var i = 0; i < helperCells.length; i++) {
                var helperCell = helperCells.eq(i);
                helperCell.width(cells.eq(i).width());
                // #5584 reflow must remove column title span
                helperCell.children().remove('.ui-column-title');
            }

            helperRow.appendTo(helper.find('tbody'));

            return helper;
        },
        update: function(event, ui) {
            var fromIndex = ui.item.data('ri'),
                itemIndex = ui.item.index();

            // #5296 must not count header group rows
            var groupHeaders = $this.tbody.children('.ui-rowgroup-header').length;
            itemIndex = itemIndex - groupHeaders;

            // #6557 must not count expanded rows
            if ($this.cfg.rowExpandMode) {
                var expandedRows = $this.tbody.children('.ui-expanded-row-content').length;
                itemIndex = itemIndex - expandedRows;
            }

            var toIndex = $this.paginator ? $this.paginator.getFirst() + itemIndex : itemIndex;

            $this.syncRowParity();

            var options = {
                source: $this.id,
                process: $this.id,
                params: [{
                        name: $this.id + '_rowreorder',
                        value: true
                    },
                    {
                        name: $this.id + '_fromIndex',
                        value: fromIndex
                    },
                    {
                        name: $this.id + '_toIndex',
                        value: toIndex
                    },
                    {
                        name: this.id + '_skipChildren',
                        value: true
                    }
                ]
            }

            if ($this.hasBehavior('rowReorder')) {
                $this.callBehavior('rowReorder', options);
            } else {
                PrimeFaces.ajax.Request.handle(options);
            }
        },
        change: function(event, ui) {
            if ($this.cfg.scrollable) {
                PrimeFaces.scrollInView($this.scrollBody, ui.placeholder);
            }
        }
    });
};
}

  2. In your AJAX "rowReorder" event add update="@this" to redraw the datatable after it reorders. This is similar to this defect https://github.com/primefaces/primefaces/issues/5326

tandraschko commented 3 years ago

same here, just a workaround IMO the JS should cover both issues

melloware commented 3 years ago

Well I fixed the JS stacktrace error. But this problem is hard becuase the row expanders create their own <tr/> rows and those are not brought along in the draggable. So the row expanders are detached from their parent row.

klaffke commented 3 years ago

Thanks, melloware! I can test your changes on monday at the earlist, but I will let you know how it worked for our application.

klaffke commented 3 years ago

Hi Melloware,

I've tested your fixes (the javascript code as well as update="@this") and it works! I've attached an updated reproducer, which incorporates your fixes, for testing purposes.

6557Reproducer.zip

klaffke commented 3 years ago

@melloware there is still a way to trigger an IndexOutOfBoundsException when moving elements in the table from bottom to top:

  1. Expand all rows
  2. Drag the last entry to the first position

java.lang.IndexOutOfBoundsException: fromIndex = -3

I think this could also affect #5296

melloware commented 3 years ago

Awesome let me investigate and thanks for testing so far!

klaffke commented 3 years ago

You're welcome! I've also tested the fix on our application with the same result.

melloware commented 3 years ago

@klaffke Can you try this add this 1 extra line to to do Math.max.

var toIndex = $this.paginator ? $this.paginator.getFirst() + itemIndex: itemIndex;
toIndex = Math.max(toIndex, 0);
klaffke commented 3 years ago

@melloware this will prevent the exception, but does not work if you try dragging the row at index 2 to position 1. It will be dragged to position 0, because the toIndex is still negative.

2020-11-30 15_03_05-PrimeFaces Test

melloware commented 3 years ago

ahh good point. let me keep poking.

melloware commented 3 years ago

OK I updated your reproducer can you try and break it again. I moved everything into test.js. I also added 2 more rows for testing. pf-6557-fixed.zip

klaffke commented 3 years ago

@melloware I've tested it thoroughly and found a few cases where it did not work as expected. The culprit was that ui-rowgroup-header and ui-expanded-row-content rows were counted for the target index, even when they are out of the dragging scope (= after both the dragged source row and the target row).

Below is my fix, up to the console log statement:

            update: function(event, ui) {
                var fromIndex = ui.item.data('ri'),
            fromNode = ui.item[0];
                    itemIndex = ui.item.index(),
                    toIndex = $this.paginator ? $this.paginator.getFirst() + itemIndex : itemIndex;
                    isDirectionUp = fromIndex >= toIndex;

                                // #5296 must not count header group rows
                                // #6557 must not count expanded rows
                if (isDirectionUp){
                    var i;
                    var tempNode = fromNode;
                    for (i = 0; i <= toIndex; i++, tempNode = tempNode.nextSibling) {
                      if (tempNode.classList.contains('ui-rowgroup-header') || tempNode.classList.contains('ui-expanded-row-content')){
                        toIndex--;
                      }
                    }
                } else{
                    var i;
                    var tempNode;
                    for (tempNode = fromNode; ;tempNode = tempNode.previousSibling) {
                      if (tempNode.classList.contains('ui-rowgroup-header') || tempNode.classList.contains('ui-expanded-row-content')){
                        toIndex--;
                      }
                      if (tempNode.previousSibling == null){
                          break;
                      }
                    }
                }
                console.log("FromIndex = " + fromIndex + " ToIndex = " + toIndex);
melloware commented 3 years ago

Nice work let me see if I can Jquery this a little bit.

melloware commented 3 years ago

OK can you try this version I made it a little more jQuery friendly.

update: function(event, ui) {
                var fromIndex = ui.item.data('ri'),
                fromNode = ui.item;
                itemIndex = ui.item.index(),
                toIndex = $this.paginator ? $this.paginator.getFirst() + itemIndex : itemIndex;
                isDirectionUp = fromIndex >= toIndex;

                // #5296 must not count header group rows
                // #6557 must not count expanded rows
                if (isDirectionUp) {
                    for (i = 0; i <= toIndex; i++) {
                        fromNode = fromNode.next('tr');
                        if (fromNode.hasClass('ui-rowgroup-header') || fromNode.hasClass('ui-expanded-row-content')){
                            toIndex--;
                        }
                    }
                } else {
                    fromNode.prevAll('tr').each(function() {
                        var node = $(this);
                        if (node.hasClass('ui-rowgroup-header') || node.hasClass('ui-expanded-row-content')){
                            toIndex--;
                        }
                    });
                }
                toIndex = Math.max(toIndex, 0);
                console.log("FromIndex = " + fromIndex + " ToIndex = " + toIndex);
klaffke commented 3 years ago

@melloware

thanks for optimizing the code (the prevAll method is very handy!). I was not able to trigger an exception using this version, neither with the reproducer nor in our application.