Skip to content

Commit

Permalink
Fix editor tests and focus/blur issues
Browse files Browse the repository at this point in the history 
* Fix 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.
* Move closured variables to the instance to resolve issues with state carrying
  over between tests
* Blur any active editOn editor before activating another one to avoid errors
  • Loading branch information
@edhager
edhager authored and Kenneth G. Franqueiro committed May 16, 2014
1 parent a442724 commit 97f5832
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 83 deletions.
51 changes: 30 additions & 21 deletions editor.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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"){
Expand Down Expand Up @@ -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){
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -363,14 +360,15 @@ 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, node;

if(!cell.column){ cell = this.cell(cell); }
if(!cell || !cell.element){ return null; }

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){
Expand All @@ -381,6 +379,15 @@ function edit(cell) {
column.get ? column.get(row.data) : row.data[field];
// check to see if the cell can be edited
if(!column.canEdit || column.canEdit(cell.row.data, value)){
// 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.
node = cmp.domNode || cmp;
if(node.offsetWidth){
// The editor is visible. Blur it.
node.blur();
}

activeCell = cellElement;

showEditor(column.editorInstance, column, cellElement, value);
Expand Down Expand Up @@ -431,15 +438,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
Expand All @@ -455,7 +463,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));
}
Expand Down Expand Up @@ -518,12 +527,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);
});
}

Expand Down
164 changes: 102 additions & 62 deletions test/intern/plugins/editor.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,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)");
Expand All @@ -118,16 +120,15 @@ define([
"canEdit should have been called for editOn editor (item 3)");
});


test.test("canEdit: suppress on false", function () {
var rowIndex,
function testCanEdit(dfd, columnId){
var rowCount,
cell,
matchedNodes;

function canEdit(data) {
return data.order % 2;
}

grid = new OnDemandGrid({
columns: {
order: "step",
Expand All @@ -148,38 +149,45 @@ 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
cell = grid.cell(rowIndex, "name");
grid.edit(cell);
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");
}
function testRow(rowIndex){
var editPromise;

// Test non-always-on editors
cell = grid.cell(rowIndex, "description");
grid.edit(cell);
matchedNodes = query("input", cell.element);
function checkAndMoveOn(expectedMatchCount, message){
matchedNodes = query("input", cell.element);
assert.strictEqual(expectedMatchCount, matchedNodes.length, message);

if (canEdit(cell.row.data)) {
assert.strictEqual(1, matchedNodes.length,
"Cell with canEdit=>true should have an editor element");
rowIndex++;
if(rowIndex < rowCount){
testRow(rowIndex);
}else{
dfd.resolve();
}
}
else {
assert.strictEqual(0, matchedNodes.length,
"Cell with canEdit=>false should not have an editor element");

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(){
return testCanEdit(this.async(), "description");
});

test.test("destroy editor widgets: native", function () {
var matchedNodes;
Expand Down Expand Up @@ -208,7 +216,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();

Expand Down Expand Up @@ -250,25 +259,11 @@ 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 () {
var rowIndex,
test.test("editor focus with always on editor", function () {
var rowCount,
cell,
cellEditor;
cellEditor,
dfd = this.async();

grid = new OnDemandGrid({
columns: {
Expand All @@ -287,37 +282,82 @@ 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 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){
// 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);
}));
// Don't move on to the next row until, the editor has received focus and the show event has fired.
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");
rowIndex++;
if(rowIndex < rowCount){
testRow(rowIndex);
}else{
dfd.resolve();
}
}));
}
testRow(0);

return dfd;
});
});
});

0 comments on commit 97f5832

Please sign in to comment.