From db44e5d85a695e7016709247734964c989140a28 Mon Sep 17 00:00:00 2001 From: Ed Hager Date: Mon, 21 Apr 2014 09:29:25 -0700 Subject: [PATCH 1/3] Fixed the editor unit tests that were causing errors (but not test failures) because the test was not waiting for the promise from Editor#edit to resolve before deleting the grid. --- test/intern/plugins/editor.js | 202 +++++++++++++++++++++++++--------- 1 file changed, 148 insertions(+), 54 deletions(-) diff --git a/test/intern/plugins/editor.js b/test/intern/plugins/editor.js index 4b8a0a616..d8e478d24 100644 --- a/test/intern/plugins/editor.js +++ b/test/intern/plugins/editor.js @@ -2,15 +2,18 @@ define([ "intern!tdd", "intern/chai!assert", "dojo/_base/declare", + "dojo/Deferred", "dojo/on", + "dojo/promise/all", "dojo/query", + "dojo/when", "dijit/registry", "dijit/form/TextBox", "dgrid/Grid", "dgrid/OnDemandGrid", "dgrid/editor", "dgrid/test/data/base" -], function (test, assert, declare, on, query, registry, TextBox, Grid, OnDemandGrid, editor) { +], function (test, assert, declare, Deferred, on, all, query, when, registry, TextBox, Grid, OnDemandGrid, editor) { var grid; // testOrderedData: global from dgrid/test/data/base.js @@ -93,6 +96,8 @@ define([ assert.isUndefined(results[3], "canEdit should not have been called yet for editOn editor (item 3)"); + // Note: The "Data 2" column's canEdit method always returns false so none of the following + // grid.edit calls will return a promise and not editor with receive focus. grid.edit(grid.cell(1, "data2")); assert.isUndefined(results[1], "canEdit should not have been called yet for editOn editor (item 1)"); @@ -118,16 +123,17 @@ define([ "canEdit should have been called for editOn editor (item 3)"); }); - - test.test("canEdit: suppress on false", function () { + test.test("canEdit always on editor: suppress on false", function(){ var rowIndex, + rowCount, cell, - matchedNodes; + matchedNodes, + dfd = this.async(); function canEdit(data) { return data.order % 2; } - + grid = new OnDemandGrid({ columns: { order: "step", @@ -148,13 +154,12 @@ define([ document.body.appendChild(grid.domNode); grid.startup(); grid.renderArray(testOrderedData); + rowCount = testOrderedData.length; - for (rowIndex = 0; rowIndex < testOrderedData.length; rowIndex++) { - // Test always-on editors + function testRow(rowIndex){ cell = grid.cell(rowIndex, "name"); - grid.edit(cell); + when(grid.edit(cell)).then(dfd.rejectOnError(function(){ matchedNodes = query("input", cell.element); - if (canEdit(cell.row.data)) { assert.strictEqual(1, matchedNodes.length, "Cell with canEdit=>true should have an editor element"); @@ -163,23 +168,76 @@ define([ assert.strictEqual(0, matchedNodes.length, "Cell with canEdit=>false should not have an editor element"); } + rowIndex++; + if(rowIndex < rowCount){ + testRow(rowIndex); + }else{ + dfd.resolve(); + } + })); + } + testRow(0); - // Test non-always-on editors - cell = grid.cell(rowIndex, "description"); - grid.edit(cell); - matchedNodes = query("input", cell.element); + return dfd; + }); - if (canEdit(cell.row.data)) { - assert.strictEqual(1, matchedNodes.length, - "Cell with canEdit=>true should have an editor element"); - } - else { - assert.strictEqual(0, matchedNodes.length, - "Cell with canEdit=>false should not have an editor element"); + test.test("canEdit edit-on click editor: suppress on false", function(){ + var rowIndex, + rowCount, + cell, + matchedNodes, + dfd = this.async(); + + function canEdit(data) { + return data.order % 2; + } + + grid = new OnDemandGrid({ + columns: { + order: "step", + name: editor({ + label: "Name", + editor: "text", + canEdit: canEdit + }), + description: editor({ + label: "Description", + editor: "text", + editOn: "click", + canEdit: canEdit + }) } + }); + + document.body.appendChild(grid.domNode); + grid.startup(); + grid.renderArray(testOrderedData); + rowCount = testOrderedData.length; + + function testRow(rowIndex){ + cell = grid.cell(rowIndex, "description"); + when(grid.edit(cell)).then(dfd.rejectOnError(function(){ + matchedNodes = query("input", cell.element); + if (canEdit(cell.row.data)) { + assert.strictEqual(1, matchedNodes.length, + "Cell with canEdit=>true should have an editor element"); + } + else { + assert.strictEqual(0, matchedNodes.length, + "Cell with canEdit=>false should not have an editor element"); + } + rowIndex++; + if(rowIndex < rowCount){ + testRow(rowIndex); + }else{ + dfd.resolve(); + } + })); } - }); + testRow(0); + return dfd; + }); test.test("destroy editor widgets: native", function () { var matchedNodes; @@ -208,7 +266,8 @@ define([ matchedNodes = query("input"); assert.strictEqual(testOrderedData.length, matchedNodes.length, - "There should be " + testOrderedData.length + " input elements for the grid's editors"); + "There should be " + testOrderedData.length + " input elements for the grid's editors and there were " + + matchedNodes.length); grid.destroy(); @@ -250,25 +309,12 @@ define([ "After grid is destroyed there should be 0 widgets on the page"); }); - - // Goal: test that when "grid.edit(cell)" is called the cell gets an editor with focus - // - // Observed behavior: - // In a cell without an always-on editor, if you call "grid.edit(cell)" - // repeatedly, the previously edited cell loses its content (not just its editor). - // document.activeElement.blur() between calls of "grid.edit" does not - // seem to work in this automated test, though it is of no consequence - // since this test is simply testing the editor's presence, not its after-effects. - // - // grid.edit: - // In a cell with an always-on editor, the editor's "focus" event is fired and - // "document.activeElement" is set to the editor. - // In a cell with a click-to-activate editor, no "focus" event is fired and - // "document.activeElement" is the body. - test.test("editor focus", function () { + test.test("editor focus with always on editor", function () { var rowIndex, + rowCount, cell, - cellEditor; + cellEditor, + dfd = this.async(); grid = new OnDemandGrid({ columns: { @@ -287,37 +333,85 @@ define([ document.body.appendChild(grid.domNode); grid.startup(); grid.renderArray(testOrderedData); + rowCount = testOrderedData.length; - for (rowIndex = 0; rowIndex < testOrderedData.length; rowIndex++) { - // Calling 'grid.edit()' on different cells consecutively results in the last-edited - // cell losing its content. It seems the blur process is important, so try to trigger that: - document.activeElement.blur(); - + function testRow(rowIndex){ // Test calling 'grid.edit()' in an always-on cell cell = grid.cell(rowIndex, "name"); - grid.edit(cell); - + grid.edit(cell).then(dfd.rejectOnError(function(node){ cellEditor = query("input", cell.element)[0]; + assert.strictEqual(cellEditor, node, + "edit method's promise should return the active editor"); assert.strictEqual(cellEditor, document.activeElement, "Editing a cell should make the cell's editor active"); + rowIndex++; + if(rowIndex < rowCount){ + testRow(rowIndex); + }else{ + dfd.resolve(); + } + })); + } + testRow(0); - document.activeElement.blur(); + return dfd; + }); + + test.test("editor focus and show event with edit-on click editor", function () { + var rowIndex, + rowCount, + cell, + cellEditor, + dfd = this.async(); + + grid = new OnDemandGrid({ + columns: { + order: "step", + name: editor({ + label: "Name", + editor: "text" + }), + description: editor({ + label: "Description", + editor: "text", + editOn: "click" + }) + } + }); + document.body.appendChild(grid.domNode); + grid.startup(); + grid.renderArray(testOrderedData); + rowCount = testOrderedData.length; + function testRow(rowIndex){ + var dfdEvent = new Deferred(); + // Test calling 'grid.edit()' in an always-on cell cell = grid.cell(rowIndex, "description"); // Respond to the "dgrid-editor-show" event to ensure the // correct cell has an editor. This event actually fires // synchronously, so we don't need to use this.async. - on.once(grid.domNode, "dgrid-editor-show", function (event) { - // document.activeElement is the body for some reason. - // So at least check to ensure that the cell we called edit on - // is the same as the cell passed to the "dgrid-editor-show" event. + on.once(grid.domNode, "dgrid-editor-show", dfd.rejectOnError(function(event){ assert.strictEqual(cell.element, event.cell.element, "The activated cell should be being edited" ); - }); - - grid.edit(cell); + dfdEvent.resolve(); + })); + // Don't move on to the next row until, the editor has received focus and the show event has fired. + all([grid.edit(cell), dfdEvent]).then(dfd.rejectOnError(function(){ + cellEditor = query("input", cell.element)[0]; + assert.strictEqual(cellEditor, document.activeElement, + "Editing a cell should make the cell's editor active"); + rowIndex++; + if(rowIndex < rowCount){ + testRow(rowIndex); + }else{ + dfd.resolve(); + } + })); } + testRow(0); + + return dfd; }); }); }); From ab4077cf913f0c8a108280fb8677607293b34473 Mon Sep 17 00:00:00 2001 From: Ed Hager Date: Wed, 23 Apr 2014 13:38:06 -0700 Subject: [PATCH 2/3] I goofed before and added code that was not compatible with Dojo 1.7. I fixed that. In doing so, I found two problems. 1) The tests active an editOn editor and then later try to move it. During the move (done via a DOM appendChild call), the blur event handler was firing and removing the editor from the DOM. That combo was causing an error in the browser. 2) The editor plug-in module defined variables in which it tracked active editor state. Because they were defined at the module level, that state was carrying over between tests. --- editor.js | 49 ++++++++------- test/intern/plugins/editor.js | 112 +++++++++------------------------- 2 files changed, 57 insertions(+), 104 deletions(-) diff --git a/editor.js b/editor.js index 6578627de..e6604f818 100644 --- a/editor.js +++ b/editor.js @@ -12,10 +12,6 @@ define([ "dojo/_base/sniff" ], function(kernel, lang, arrayUtil, Deferred, on, aspect, has, query, Grid, put){ -// Variables to track info for cell currently being edited -// (active* variables are for editOn editors only) -var activeCell, activeValue, activeOptions, focusedCell; - function updateInputValue(input, value){ // common code for updating value of a standard input input.value = value; @@ -111,15 +107,16 @@ function setPropertyFromEditor(grid, cmp, triggerEvent) { column = cell.column, value, id, - editedRow; + editedRow, + activeCell = grid._activeCell; if(!cmp.isValid || cmp.isValid()){ value = setProperty(grid, cell, - activeCell ? activeValue : cmp._dgridLastValue, + activeCell ? grid._activeValue : cmp._dgridLastValue, dataFromEditor(column, cmp), triggerEvent); if(activeCell){ // for editors with editOn defined - activeValue = value; + grid._activeValue = value; }else{ // for always-on editors, update _dgridLastValue immediately cmp._dgridLastValue = value; } @@ -238,7 +235,7 @@ function createSharedEditor(column, originalRenderCell){ keyHandle; function blur(){ - var element = activeCell; + var element = grid._activeCell; focusNode.blur(); if(typeof grid.focus === "function"){ @@ -276,13 +273,13 @@ function createSharedEditor(column, originalRenderCell){ // Clear out the rest of the cell's contents, then re-render with new value. while(i--){ put(parentNode.firstChild, "!"); } Grid.appendIfNode(parentNode, column.renderCell( - column.grid.row(parentNode).data, activeValue, parentNode, - activeOptions ? lang.delegate(options, activeOptions) : options)); + column.grid.row(parentNode).data, grid._activeValue, parentNode, + grid._activeOptions ? lang.delegate(options, grid._activeOptions) : options)); // Reset state now that editor is deactivated; // reset focusedCell as well since some browsers will not trigger the // focusout event handler in this case - activeCell = activeValue = activeOptions = focusedCell = null; + grid._focusedCell = grid._activeCell = grid._activeValue = grid._activeOptions = null; } function dismissOnKey(evt){ @@ -292,7 +289,7 @@ function createSharedEditor(column, originalRenderCell){ if(key == 27){ // escape: revert + dismiss reset(); - activeValue = cmp._dgridLastValue; + grid._activeValue = cmp._dgridLastValue; blur(); }else if(key == 13 && column.dismissOnEnter !== false){ // enter: dismiss // FIXME: Opera is "reverting" even in this case @@ -338,8 +335,8 @@ function showEditor(cmp, column, cellElement, value){ cmp._dgridLastValue = value; // if this is an editor with editOn, also update activeValue // (activeOptions will have been updated previously) - if(activeCell){ - activeValue = value; + if(grid._activeCell){ + grid._activeValue = value; // emit an event immediately prior to placing a shared editor on.emit(cellElement, "dgrid-editor-show", { grid: grid, @@ -363,7 +360,7 @@ function edit(cell) { // input/widget when the cell editor is focused. // If the cell is not editable, returns null. - var row, column, cellElement, dirty, field, value, cmp, dfd; + var row, column, cellElement, dirty, field, value, cmp, dfd, activeCell; if(!cell.column){ cell = this.cell(cell); } if(!cell || !cell.element){ return null; } @@ -371,6 +368,7 @@ function edit(cell) { column = cell.column; field = column.field; cellElement = cell.element.contents || cell.element; + activeCell = column.grid._activeCell; if((cmp = column.editorInstance)){ // shared editor (editOn used) if(activeCell != cellElement){ @@ -383,6 +381,13 @@ function edit(cell) { if(!column.canEdit || column.canEdit(cell.row.data, value)){ activeCell = cellElement; + // In some browsers, moving a DOM node causes a blur event to fire which is not + // the best time for the blur handler to fire. Force the issue by blurring the + // editor now. + if(column.editorInstance){ + column.editorInstance.blur(); + } + showEditor(column.editorInstance, column, cellElement, value); // focus / blur-handler-resume logic is surrounded in a setTimeout @@ -431,15 +436,16 @@ return function(column, editor, editOn){ grid.edit = edit; listeners.push(on(grid.domNode, '.dgrid-input:focusin', function () { - focusedCell = grid.cell(this); + grid._focusedCell = grid.cell(this); })); focusoutHandle = grid._editorFocusoutHandle = on.pausable(grid.domNode, '.dgrid-input:focusout', function () { - focusedCell = null; + grid._focusedCell = null; }); listeners.push(focusoutHandle); listeners.push(aspect.before(grid, 'removeRow', function (row) { + var focusedCell = grid._focusedCell; row = grid.row(row); if (focusedCell && focusedCell.row.id === row.id) { // Pause the focusout handler until after this row has had @@ -455,7 +461,8 @@ return function(column, editor, editOn){ } })); listeners.push(aspect.after(grid, 'insertRow', function (rowElement) { - var row = grid.row(rowElement); + var row = grid.row(rowElement), + focusedCell = grid._focusedCell; if (focusedCell && focusedCell.row.id === row.id) { grid.edit(grid.cell(row, focusedCell.column.id)); } @@ -518,12 +525,12 @@ return function(column, editor, editOn){ // TODO: Consider using event delegation // (Would require using dgrid's focus events for activating on focus, // which we already advocate in README for optimal use) - + var grid = column.grid; if(!options || !options.alreadyHooked){ // in IE<8, cell is the child of the td due to the extra padding node on(cell.tagName == "TD" ? cell : cell.parentNode, editOn, function(){ - activeOptions = options; - column.grid.edit(this); + grid._activeOptions = options; + grid.edit(this); }); } diff --git a/test/intern/plugins/editor.js b/test/intern/plugins/editor.js index d8e478d24..1c67bfd93 100644 --- a/test/intern/plugins/editor.js +++ b/test/intern/plugins/editor.js @@ -2,18 +2,15 @@ define([ "intern!tdd", "intern/chai!assert", "dojo/_base/declare", - "dojo/Deferred", "dojo/on", - "dojo/promise/all", "dojo/query", - "dojo/when", "dijit/registry", "dijit/form/TextBox", "dgrid/Grid", "dgrid/OnDemandGrid", "dgrid/editor", "dgrid/test/data/base" -], function (test, assert, declare, Deferred, on, all, query, when, registry, TextBox, Grid, OnDemandGrid, editor) { +], function (test, assert, declare, on, query, registry, TextBox, Grid, OnDemandGrid, editor) { var grid; // testOrderedData: global from dgrid/test/data/base.js @@ -123,12 +120,10 @@ define([ "canEdit should have been called for editOn editor (item 3)"); }); - test.test("canEdit always on editor: suppress on false", function(){ - var rowIndex, - rowCount, + function testCanEdit(dfd, columnId){ + var rowCount, cell, - matchedNodes, - dfd = this.async(); + matchedNodes; function canEdit(data) { return data.order % 2; @@ -157,86 +152,41 @@ define([ rowCount = testOrderedData.length; function testRow(rowIndex){ - cell = grid.cell(rowIndex, "name"); - when(grid.edit(cell)).then(dfd.rejectOnError(function(){ - matchedNodes = query("input", cell.element); - if (canEdit(cell.row.data)) { - assert.strictEqual(1, matchedNodes.length, - "Cell with canEdit=>true should have an editor element"); - } - else { - assert.strictEqual(0, matchedNodes.length, - "Cell with canEdit=>false should not have an editor element"); - } + var editPromise; + + function checkAndMoveOn(expectedMatchCount, message){ + matchedNodes = query("input", cell.element); + assert.strictEqual(expectedMatchCount, matchedNodes.length, message); + rowIndex++; if(rowIndex < rowCount){ testRow(rowIndex); }else{ dfd.resolve(); } - })); + } + + cell = grid.cell(rowIndex, columnId); + editPromise = grid.edit(cell); + if(editPromise){ + editPromise.then(dfd.rejectOnError(function(){ + checkAndMoveOn(1, "Cell with canEdit=>true should have an editor element"); + })); + }else{ + checkAndMoveOn(0, "Cell with canEdit=>false should not have an editor element"); + } } testRow(0); return dfd; + } + + test.test("canEdit always on editor: suppress on false", function(){ + return testCanEdit(this.async(), "name"); }); test.test("canEdit edit-on click editor: suppress on false", function(){ - var rowIndex, - rowCount, - cell, - matchedNodes, - dfd = this.async(); - - function canEdit(data) { - return data.order % 2; - } - - grid = new OnDemandGrid({ - columns: { - order: "step", - name: editor({ - label: "Name", - editor: "text", - canEdit: canEdit - }), - description: editor({ - label: "Description", - editor: "text", - editOn: "click", - canEdit: canEdit - }) - } - }); - - document.body.appendChild(grid.domNode); - grid.startup(); - grid.renderArray(testOrderedData); - rowCount = testOrderedData.length; - - function testRow(rowIndex){ - cell = grid.cell(rowIndex, "description"); - when(grid.edit(cell)).then(dfd.rejectOnError(function(){ - matchedNodes = query("input", cell.element); - if (canEdit(cell.row.data)) { - assert.strictEqual(1, matchedNodes.length, - "Cell with canEdit=>true should have an editor element"); - } - else { - assert.strictEqual(0, matchedNodes.length, - "Cell with canEdit=>false should not have an editor element"); - } - rowIndex++; - if(rowIndex < rowCount){ - testRow(rowIndex); - }else{ - dfd.resolve(); - } - })); - } - testRow(0); - - return dfd; + return testCanEdit(this.async(), "description"); }); test.test("destroy editor widgets: native", function () { @@ -310,8 +260,7 @@ define([ }); test.test("editor focus with always on editor", function () { - var rowIndex, - rowCount, + var rowCount, cell, cellEditor, dfd = this.async(); @@ -358,8 +307,7 @@ define([ }); test.test("editor focus and show event with edit-on click editor", function () { - var rowIndex, - rowCount, + var rowCount, cell, cellEditor, dfd = this.async(); @@ -384,7 +332,6 @@ define([ rowCount = testOrderedData.length; function testRow(rowIndex){ - var dfdEvent = new Deferred(); // Test calling 'grid.edit()' in an always-on cell cell = grid.cell(rowIndex, "description"); // Respond to the "dgrid-editor-show" event to ensure the @@ -394,10 +341,9 @@ define([ assert.strictEqual(cell.element, event.cell.element, "The activated cell should be being edited" ); - dfdEvent.resolve(); })); // Don't move on to the next row until, the editor has received focus and the show event has fired. - all([grid.edit(cell), dfdEvent]).then(dfd.rejectOnError(function(){ + grid.edit(cell).then(dfd.rejectOnError(function(){ cellEditor = query("input", cell.element)[0]; assert.strictEqual(cellEditor, document.activeElement, "Editing a cell should make the cell's editor active"); From d54d05dbe8f8e97c71b65e7957fb325a4e6a1368 Mon Sep 17 00:00:00 2001 From: Ed Hager Date: Wed, 23 Apr 2014 14:02:52 -0700 Subject: [PATCH 3/3] Blur the shared editor only if it is visible. --- editor.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/editor.js b/editor.js index e6604f818..acdaace43 100644 --- a/editor.js +++ b/editor.js @@ -360,7 +360,7 @@ function edit(cell) { // input/widget when the cell editor is focused. // If the cell is not editable, returns null. - var row, column, cellElement, dirty, field, value, cmp, dfd, activeCell; + var row, column, cellElement, dirty, field, value, cmp, dfd, activeCell, node; if(!cell.column){ cell = this.cell(cell); } if(!cell || !cell.element){ return null; } @@ -384,8 +384,10 @@ function edit(cell) { // In some browsers, moving a DOM node causes a blur event to fire which is not // the best time for the blur handler to fire. Force the issue by blurring the // editor now. - if(column.editorInstance){ - column.editorInstance.blur(); + node = cmp.domNode || cmp; + if(node.offsetWidth){ + // The editor is visible. Blur it. + node.blur(); } showEditor(column.editorInstance, column, cellElement, value);