ColumnReorder is broken in dgrid 0.3.7

lixopmstp commented 11 years ago

When I try to swap columns the grid headers disappear. Observed in Firefox 20 (Firebug shows a "too much recursion" error) and MSIE 9 with dojo 1.8.1. Works fine in dgrid 0.3.6. Note I'm using the following extensions and plugins:

    "dgrid/Grid", 
    "dgrid/selector", 
    "dgrid/Keyboard", 
    "dgrid/Selection", 
    "dgrid/extensions/Pagination",
    "dgrid/extensions/ColumnReorder",
    "dgrid/extensions/ColumnResizer", 
    "dgrid/extensions/ColumnHider",

lixopmstp commented 11 years ago

Chrome 26.0.1410.43 m (Windows) also has trouble with the recursion. It's developer tools show a bit more info:

Uncaught RangeError: Maximum call stack size exceeded declare.row column.disabled grid.allowSelect column.disabled grid.allowSelect column.disabled ... (repeats manytimes)

lixopmstp commented 11 years ago

This may not matter, but some columns have formatters and some renderCells, a couple of them also have renderCellHeaders.

kfranqueiro commented 11 years ago

Looking at that stack trace, it looks like this issue might have nothing to do with extensions, and everything to do with how you are using Selection and selector. Can you provide code indicating how your columns are set up, and if you have specified a custom allowSelect method implementation?

lixopmstp commented 11 years ago

Hi,

I cannot easily show how the columns are created (they’re dynamically created in a JSP based on a user query and preferences). The grid is initially created with the following selection options:

    selectionMode: "none",
    deselectOnRefresh: false,
    allowSelectAll: true,
    cellNavigation: true,
    allowTextSelection: true,

I also have an on("dgrid-select, dgrid-deselect") vent handler that simply enables/disables a few buttons and links (not on the grid)

Hope this helps, Pedro

From: Kenneth G. Franqueiro [mailto:notifications@github.com] Sent: Wednesday, April 10, 2013 4:12 PM To: SitePen/dgrid Cc: Pedro Proenca Subject: Re: [dgrid] ColumnReorder is broken in dgrid 0.3.7 (#525)

Looking at that stack trace, it looks like this issue might have nothing to do with extensions, and everything to do with how you are using Selection and selector. Can you provide code indicating how your columns are set up, and if you have specified a custom allowSelect method implementation?

— Reply to this email directly or view it on GitHubhttps://github.com/SitePen/dgrid/issues/525#issuecomment-16198852.

APGarchev commented 11 years ago

Hi, I get the same problem and traced the code. Here is what I found.

The problem is caused by a endless recursion between column.disabled and grid.allowSelect methos.

The selector method defines column.renderHeaderCell. When the selector column type is radio or grid.allowSelectAll is true it calls a method setupSelectionEvents defined in selector.js, else the renderInput calls the setupSelectionEvents method. When the selector column hasn't defined disabled property and it's not a method, the setupSelectionEvents attaches to the selector column a default disabled method which calls the grid.allowSelect method (by default it returns always true), but else the grid.allowSelect is updated to call the original grid.allowSelect and the origianl column.default method and the column.disabled is updated to call the updated grid.allowSelect method.

The reorder action caused call of the grid._setColumns (or _setColumnSets/_setSubRows) method, all existing columns are destroyed (grid._destroyColumns) and as a result the flag (grid._hasSelectorInputListener) that prevents the execution of setupSelectionEvents is set to false. The column.renderHeaderCell is called again and that caused again update of the grid.allowSelect and the column.disabled.

Then the renderInput method is executed and it executes the column.disabled method, in result a recursion between the column.disabled method and the grid.allowSelect happens.

<!DOCTYPE html>
<html>
<head>
  <meta charset="utf-8">
  <link rel="stylesheet" href="dojo/resources/dojo.css" />
  <link rel="stylesheet" href="dijit/themes/claro/claro.css" />
  <style>
    .grid {
      width: 700px;
      margin: 10px;
    }
  </style>
  <title>Tutorial: Hello dgrid!</title>
  <script>
    var dojoConfig = { async: true, parseOnLoad: true };
  </script>
</head>
<body class="claro">
  <script src="dojo/dojo.js"></script>
  <script>
  require([
      "dojo/_base/declare", "dojo/request", "dojo/store/Memory",
      "dgrid/Grid", "dgrid/ColumnSet", "dgrid/selector", "dgrid/Selection",
      "dgrid/extensions/Pagination", "dgrid/extensions/ColumnReorder"
  ], function (declare, request, Memory, Grid, ColumnSet, selector, Selection, 
      Pagination, ColumnReorder) {
    request("hof-batting.json", { handleAs: "json" }).then(function (response) {
      var store = new Memory({ "idProperty": "id", data: response });
      var CustomGrid = declare([Grid, Selection, ColumnReorder, Pagination]);
      var grid = new CustomGrid({
        store: store,
        columns: [
          selector({ field: "id", "selectorType": "checkbox", "reorderable": false,
              disabled: function() { return false; }}),
          { field: "first", label: "First Name" },
          { field: "last", label: "Last Name" },
          { field: "age", label: "Age" }
        ],
        allowSelectAll: true
      }, "grid");
      grid.startup();
    });
  });
  </script>
  <div id="grid"></div>
</body>
</html>

ghost commented 11 years ago

IIRC, I posted a fix for this as a pull request. When Ken gets a. Chance to review it, it will likely be merged into master.

Sent from my iPhone

On Apr 26, 2013, at 9:55 AM, APGarchev notifications@github.com wrote:

Hi, I get the same problem and traced the code. Here is what I found.

The problem is caused by a endless recursion between column.disabled and grid.allowSelect methos.

The selector method defines column.renderHeaderCell. When the selector column type is radio or grid.allowSelectAll is true it calls a method setupSelectionEvents defined in selector.js, else the renderInput calls the setupSelectionEvents method. When the selector column hasn't defined disabled property and it's not a method, the setupSelectionEvents attaches to the selector column a default disabled method which calls the grid.allowSelect method (by default it returns always true), but else the grid.allowSelect is updated to call the original grid.allowSelect and the origianl column.default method and the column.disabled is updated to call the updated grid.allowSelect method.

The reorder action caused call of the grid._setColumns (or _setColumnSets/_setSubRows) method, all existing columns are destroyed (grid._destroyColumns) and as a result the flag (grid._hasSelectorInputListener) that prevents the execution of setupSelectionEvents is set to false. The column.renderHeaderCell is called again and that caused again update of the grid.allowSelect and the column.disabled.

Then the renderInput method is executed and it executes the column.disabled method, in result a recursion between the column.disabled method and the grid.allowSelect happens.

<!DOCTYPE html>
Tutorial: Hello dgrid!

— Reply to this email directly or view it on GitHub.

lixopmstp commented 11 years ago

I had a change to try it out with a local patched 0.3.7 version and it seems fine.

Thanks

kfranqueiro commented 9 years ago

Sorry this issue ended up left open. Yes, I believe this was fixed in 0.3.8. Furthermore, in 0.4, there is no longer a disabled property for selector columns since it is redundant of Selection#allowSelect.

SitePen / dgrid

ColumnReorder is broken in dgrid 0.3.7 #525