ICEfaces
  1. ICEfaces
  2. ICE-10507

ace:dataTable - Non-submitted row edits updated on stop editing call

    Details

      Description

      If multiple rows are set to be editable and changes are made on the cells, if one row is submitted and row edits are cancelled via the server side, the changed value is updated. The same behavior is not seen with a single row edit, nor does it do this if one row edit hasn't been saved.

      The following steps can be used to reproduce the issue with the Showcase demo:

      Steps:
      1. Click edit for any row.
      2. Click edit for another row.
      3. Change a value in both rows
      4. Submit just one row over the little blue checkmark
      5. Now you have one row in edit modus with a changed value. Click on the button "Stop Editing For All"
      6. Editing is stopped but the changed value is submited.

      If you do these steps with one edited row there are no changed valus submited.

        Activity

        Hide
        Arturo Zambrano added a comment -

        Committed fix to 4.0 trunk at revision 44102 and to 3.3 EE maintenance branch at revision 44103.

        The initial fix was simply to avoid processing decodes for ace:cellEditor children when the source of the request isn't the save edits button. However, because of the same reason, domdiff was erasing the inputs entered in other rows in edit mode, when the whole form was being submitted, including requests to save edits of a specific row. So, if two or more rows were in edit mode, and the inputs were edited in some or all of them, after clicking the save edits button for one row, all the other edited inputs were revered to their values in the server, because of a domdiff mechanism that does this. Therefore, a mechanism was added to remove name attributes from input fields inside cell editors, and thus avoid that domdiff see these changes (and revert them). The name attributes are restored when the save edits button is activated for the current row.

        Testing notes: please try different use cases that a typical user may have, like having several rows in edit mode, editing them and only saving some of them; submitting the whole form via submit button or causing a full form or full table update, etc. Values in edit mode rows should only be saved when clicking the save edits button of their corresponding row.

        Show
        Arturo Zambrano added a comment - Committed fix to 4.0 trunk at revision 44102 and to 3.3 EE maintenance branch at revision 44103. The initial fix was simply to avoid processing decodes for ace:cellEditor children when the source of the request isn't the save edits button. However, because of the same reason, domdiff was erasing the inputs entered in other rows in edit mode, when the whole form was being submitted, including requests to save edits of a specific row. So, if two or more rows were in edit mode, and the inputs were edited in some or all of them, after clicking the save edits button for one row, all the other edited inputs were revered to their values in the server, because of a domdiff mechanism that does this. Therefore, a mechanism was added to remove name attributes from input fields inside cell editors, and thus avoid that domdiff see these changes (and revert them). The name attributes are restored when the save edits button is activated for the current row. Testing notes: please try different use cases that a typical user may have, like having several rows in edit mode, editing them and only saving some of them; submitting the whole form via submit button or causing a full form or full table update, etc. Values in edit mode rows should only be saved when clicking the save edits button of their corresponding row.
        Hide
        Liana Munroe added a comment - - edited

        Verified ICEfaces ee-3.3.0 maintenance branch and ICEfaces 4 trunk r44114, Tomcat 7, IE 11, FF 34, Chrome 40 using showcase and QA dataTable test apps with several scenarios used.
        An issue has been raised when using event=editSubmit execute=@all render=@all. These tests were passing just prior to this fix at r 44100. Art is looking into it.

        Show
        Liana Munroe added a comment - - edited Verified ICEfaces ee-3.3.0 maintenance branch and ICEfaces 4 trunk r44114, Tomcat 7, IE 11, FF 34, Chrome 40 using showcase and QA dataTable test apps with several scenarios used. An issue has been raised when using event=editSubmit execute=@all render=@all. These tests were passing just prior to this fix at r 44100. Art is looking into it.
        Hide
        Liana Munroe added a comment -

        Reopening for 2 reasons:
        1.) showcase dataTable > Click Events.
        Demo fails manually. Data is not saved when any cell is updated. This test was passing icefaces 4 trunk r44100 and fails at r44105.

        2.) An issue has been raised when using event=editSubmit execute=@all render=@all. These tests were passing just prior to this fix at r 44100.
        This can be seen with the QA test apps cellEditingTest.jsf and dataTableAjax.jsf.

        Show
        Liana Munroe added a comment - Reopening for 2 reasons: 1.) showcase dataTable > Click Events. Demo fails manually. Data is not saved when any cell is updated. This test was passing icefaces 4 trunk r44100 and fails at r44105. 2.) An issue has been raised when using event=editSubmit execute=@all render=@all. These tests were passing just prior to this fix at r 44100. This can be seen with the QA test apps cellEditingTest.jsf and dataTableAjax.jsf.
        Hide
        Arturo Zambrano added a comment - - edited

        The Click Events demo doesn't work after this fix because the request is not done via an ace:rowEditor button, but via a double-click ajax event. So, the client id of the cell editor is not explicitly specified in the execute parameter in the request, and therefore the decoding phase is skipped for the cell editor.

        The only adequate solution for this seems to be adding a new boolean attribute to the ace:cellEditor component to revert to its old behaviour or executing its contents at any request (including requests to save cell editors in other rows/cells).

        Tentatively, the attribute would be called 'restrictedExecution', and the default value would be true. When true, the cell editor will only execute its contents when a row editor in the same row initiates an editSubmit request. When false, any request that executes the whole form, the whole table, or '@all' will also execute the cell editor contents (as it was before this fix). In the case of this demo, if all cell editors have 'restrictedExecution' set to false, the demo would work as before. In general, when the cell editor is to be used in a different way than the typical use case, page authors would set 'restrictedExecution' to false.

        This attribute would be different from the ace:dataTable's 'alwaysExecuteContents' attribute, because the latter is used to enable/disable execution of contents on table feature requests (e.g. selection, pagination, etc.). In the case of 'restrictedExecution', it enables/disables execution on requests initiated by other controls present on the page that were added by its author.

        Show
        Arturo Zambrano added a comment - - edited The Click Events demo doesn't work after this fix because the request is not done via an ace:rowEditor button, but via a double-click ajax event. So, the client id of the cell editor is not explicitly specified in the execute parameter in the request, and therefore the decoding phase is skipped for the cell editor. The only adequate solution for this seems to be adding a new boolean attribute to the ace:cellEditor component to revert to its old behaviour or executing its contents at any request (including requests to save cell editors in other rows/cells). Tentatively, the attribute would be called 'restrictedExecution', and the default value would be true. When true, the cell editor will only execute its contents when a row editor in the same row initiates an editSubmit request. When false, any request that executes the whole form, the whole table, or '@all' will also execute the cell editor contents (as it was before this fix). In the case of this demo, if all cell editors have 'restrictedExecution' set to false, the demo would work as before. In general, when the cell editor is to be used in a different way than the typical use case, page authors would set 'restrictedExecution' to false. This attribute would be different from the ace:dataTable's 'alwaysExecuteContents' attribute, because the latter is used to enable/disable execution of contents on table feature requests (e.g. selection, pagination, etc.). In the case of 'restrictedExecution', it enables/disables execution on requests initiated by other controls present on the page that were added by its author.
        Hide
        Arturo Zambrano added a comment -

        Another possibility that would work better, perhaps, is to simply modify the current fix (and no new attribute would need to be added), as explained below.

        When the editSubmit event is sent to the server, by clicking on the ace:rowEditor's check button, the client ids of the ace:cellEditor's in the same row are added to the execute parameter in the request. The current fix consists in the ace:cellEditor component examining this execute parameter and only execute if its client id was explicitly included in this parameter. This way, we avoid executing input components inside ace:cellEditor's that are in other rows in editing mode when the editSubmit request is made for a particular row, as desired. The side effect of this is that a row of ace:cellEditor's won't be executed if the editSubmit ajax event contains @all or @form in its execute attribute; also, it won't be executed when submitting the whole form or when using the cell editor in non-typical ways, as in the case of the Click Events demo, because the client ids of the cell editors is not explicitly included in the execute parameter in these cases.

        Therefore, instead of the current fix that makes a cell editor only execute if it finds its client id in the execute parameter, we could change that fix to look somewhere in the request to see if its an editSubmit request and to see for which row it is. If it is an editSubmit request for another row, the cell editor won't execute; in all other cases it will execute. This way, we prevent a row of cell editors from executing when another row triggered the editSubmit request (original issue), and we allow the cell editors to execute by any other means, like when using @all or @form in the execute attribute of the ajax tag, or when submitting the whole form via a submit button or when using the cell editor in a different way, as in the Click Events demo.

        Show
        Arturo Zambrano added a comment - Another possibility that would work better, perhaps, is to simply modify the current fix (and no new attribute would need to be added), as explained below. When the editSubmit event is sent to the server, by clicking on the ace:rowEditor's check button, the client ids of the ace:cellEditor's in the same row are added to the execute parameter in the request. The current fix consists in the ace:cellEditor component examining this execute parameter and only execute if its client id was explicitly included in this parameter. This way, we avoid executing input components inside ace:cellEditor's that are in other rows in editing mode when the editSubmit request is made for a particular row, as desired. The side effect of this is that a row of ace:cellEditor's won't be executed if the editSubmit ajax event contains @all or @form in its execute attribute; also, it won't be executed when submitting the whole form or when using the cell editor in non-typical ways, as in the case of the Click Events demo, because the client ids of the cell editors is not explicitly included in the execute parameter in these cases. Therefore, instead of the current fix that makes a cell editor only execute if it finds its client id in the execute parameter, we could change that fix to look somewhere in the request to see if its an editSubmit request and to see for which row it is. If it is an editSubmit request for another row, the cell editor won't execute; in all other cases it will execute. This way, we prevent a row of cell editors from executing when another row triggered the editSubmit request (original issue), and we allow the cell editors to execute by any other means, like when using @all or @form in the execute attribute of the ajax tag, or when submitting the whole form via a submit button or when using the cell editor in a different way, as in the Click Events demo.
        Hide
        Ken Fyten added a comment -

        I think the latest idea makes sense. By default, clicking triggering an editSubmit event for a single row should only execute that row, but submitting the whole form/table should execute all the contents.

        Show
        Ken Fyten added a comment - I think the latest idea makes sense. By default, clicking triggering an editSubmit event for a single row should only execute that row, but submitting the whole form/table should execute all the contents.
        Hide
        Arturo Zambrano added a comment -

        r44221: modified previous fix to allow cell editor contents to be executed when used in non-typical ways.

        Show
        Arturo Zambrano added a comment - r44221: modified previous fix to allow cell editor contents to be executed when used in non-typical ways.
        Hide
        Arturo Zambrano added a comment -

        r44223: committed new fix to the 3.3 EE maintenance branch.

        Show
        Arturo Zambrano added a comment - r44223: committed new fix to the 3.3 EE maintenance branch.
        Hide
        Jack Van Ooststroom added a comment -

        Sending samples/showcase/showcase/src/main/java/org/icefaces/samples/showcase/ace/documentationResources/CloudPushResources.java
        Transmitting file data .
        Committed revision 44228.

        Show
        Jack Van Ooststroom added a comment - Sending samples/showcase/showcase/src/main/java/org/icefaces/samples/showcase/ace/documentationResources/CloudPushResources.java Transmitting file data . Committed revision 44228.
        Hide
        Jack Van Ooststroom added a comment -

        Fixed a small compilation error by removing the trailing comma withing the resources.

        Show
        Jack Van Ooststroom added a comment - Fixed a small compilation error by removing the trailing comma withing the resources.
        Hide
        Ken Fyten added a comment -

        ace:dataTable -> Click Events demo Regression

        Re-opened as there is now an issue where multiple submit/responses appear to occur in succession when editing and submitting cell values via double-click.

        To reproduce:

        1. Double-click a name cell to edit it and double-click to submit the changes.
        2. Navigate to pg2 and then back to pg. via the pagination
        3. Edit another cell via double-click, notice the flashing between edit and read mode.

        The number of flashes between edit and read mode correlates to the number of paginator pages that have been changed. Weird...

        Show
        Ken Fyten added a comment - ace:dataTable -> Click Events demo Regression Re-opened as there is now an issue where multiple submit/responses appear to occur in succession when editing and submitting cell values via double-click. To reproduce: Double-click a name cell to edit it and double-click to submit the changes. Navigate to pg2 and then back to pg. via the pagination Edit another cell via double-click, notice the flashing between edit and read mode. The number of flashes between edit and read mode correlates to the number of paginator pages that have been changed. Weird...
        Hide
        Arturo Zambrano added a comment -

        That last issue is actually a side effect of ICE-10529. Removing the client id that was added to the span that contains the script, resolves this issue.

        This happens because now the table markup is not updated in full when clicking on a page in the paginator. Because now the script tag is inside a span with its own client id and the table body (<tbody>) has its own client id as well, so, when the user clicks on a page of the paginator, the table body and the script are updated, instead of the whole markup from the root div, and, because the script re-initializes the table, a new event listener for double clicks is added to the root element, so now multiple requests are fired because there are multiple event listeners registered.

        This could be fixed by removing click event listeners before (re)initializing the table. We could also consider solving ICE-10529 without adding those inner client ids, if feasible.

        Show
        Arturo Zambrano added a comment - That last issue is actually a side effect of ICE-10529 . Removing the client id that was added to the span that contains the script, resolves this issue. This happens because now the table markup is not updated in full when clicking on a page in the paginator. Because now the script tag is inside a span with its own client id and the table body (<tbody>) has its own client id as well, so, when the user clicks on a page of the paginator, the table body and the script are updated, instead of the whole markup from the root div, and, because the script re-initializes the table, a new event listener for double clicks is added to the root element, so now multiple requests are fired because there are multiple event listeners registered. This could be fixed by removing click event listeners before (re)initializing the table. We could also consider solving ICE-10529 without adding those inner client ids, if feasible.
        Hide
        Arturo Zambrano added a comment -

        Closing again, since the last issue is caused by ICE-10529. Comments were copied to that JIRA.

        Show
        Arturo Zambrano added a comment - Closing again, since the last issue is caused by ICE-10529 . Comments were copied to that JIRA.

          People

          • Assignee:
            Arturo Zambrano
            Reporter:
            Arran Mccullough
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: