Skip to content

Commit

Permalink
Fix several bugs found after code review (#4662)
Browse files Browse the repository at this point in the history
  • Loading branch information
@budnix
budnix committed May 29, 2018
1 parent e0953a9 commit 932486b
Show file tree
Hide file tree
Showing 11 changed files with 528 additions and 246 deletions.
6 changes: 6 additions & 0 deletions lib/autoResize/autoResize.js
View file
Expand Up @@ -145,6 +145,9 @@ function autoResize() {
observe(el, 'drop', delayedResize);
observe(el, 'keydown', delayedResize);
observe(el, 'focus', resize);
observe(el, 'compositionstart', delayedResize);
observe(el, 'compositionupdate', delayedResize);
observe(el, 'compositionend', delayedResize);
}

resize();
Expand All @@ -165,6 +168,9 @@ function autoResize() {
unObserve(el, 'drop', delayedResize);
unObserve(el, 'keydown', delayedResize);
unObserve(el, 'focus', resize);
unObserve(el, 'compositionstart', delayedResize);
unObserve(el, 'compositionupdate', delayedResize);
unObserve(el, 'compositionend', delayedResize);
},
resize: resize
};
Expand Down
14 changes: 8 additions & 6 deletions src/core.js
View file
Expand Up @@ -1220,10 +1220,11 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
*
* @memberof Core#
* @function destroyEditor
* @param {Boolean} [revertOriginal] If != `true`, edited data is saved. Otherwise the previous value is restored.
* @param {Boolean} [revertOriginal=false] If `true`, the previous value will be restored. Otherwise, the edited value will be saved.
* @param {Boolean} [prepareEditorIfNeeded=true] If `true` the editor under the selected cell will be prepared to open.
*/
this.destroyEditor = function(revertOriginal) {
instance._refreshBorders(revertOriginal);
this.destroyEditor = function(revertOriginal = false, prepareEditorIfNeeded = true) {
instance._refreshBorders(revertOriginal, prepareEditorIfNeeded);
};

/**
Expand Down Expand Up @@ -3544,13 +3545,14 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
* Refresh selection borders. This is temporary method relic after selection rewrite.
*
* @private
* @param {Boolean} revertOriginal
* @param {Boolean} [revertOriginal=false] If `true`, the previous value will be restored. Otherwise, the edited value will be saved.
* @param {Boolean} [prepareEditorIfNeeded=true] If `true` the editor under the selected cell will be prepared to open.
*/
this._refreshBorders = function(revertOriginal) {
this._refreshBorders = function(revertOriginal = false, prepareEditorIfNeeded = true) {
editorManager.destroyEditor(revertOriginal);
instance.view.render();

if (selection.isSelected()) {
if (prepareEditorIfNeeded && selection.isSelected()) {
editorManager.prepareEditor();
}
};
Expand Down
2 changes: 1 addition & 1 deletion src/editorManager.js
View file
Expand Up @@ -253,7 +253,7 @@ function EditorManager(instance, priv, selection) {

// Open editor when text composition is started (IME editor)
eventManager.addEventListener(document.documentElement, 'compositionstart', (event) => {
if (!destroyed && activeEditor && !activeEditor.isOpened()) {
if (!destroyed && activeEditor && !activeEditor.isOpened() && instance.isListening()) {
_this.openEditor('', event);
}
});
Expand Down
39 changes: 22 additions & 17 deletions src/editors/textEditor.js
View file
Expand Up @@ -41,39 +41,39 @@ TextEditor.prototype.init = function() {
};

TextEditor.prototype.prepare = function(row, col, prop, td, originalValue, cellProperties) {
const previousState = this.state;

BaseEditor.prototype.prepare.apply(this, arguments);

if (!cellProperties.readOnly) {
this.refreshDimensions(true);

if (cellProperties.allowInvalid) {
const {
allowInvalid,
fragmentSelection,
} = cellProperties;

if (allowInvalid) {
this.TEXTAREA.value = ''; // Remove an empty space from texarea (added by copyPaste plugin to make copy/paste functionality work with IME)
}

if (isMSBrowser()) {
// Move textarea element out off the viewport due to the cursor overlapping bug on IE.
if (previousState !== EditorState.FINISHED) {
this.hideEditableElement();
}
// @TODO: The fragmentSelection functionality is conflicted with IME. To make fragmentSelection working below is a condition which disables
// IME when fragmentSelection is enabled
if (!cellProperties.fragmentSelection) {

// @TODO: The fragmentSelection functionality is conflicted with IME. For this feature refocus has to
// be disabled (to make IME working).
const restoreFocus = !fragmentSelection;

if (restoreFocus) {
this.instance._registerImmediate(() => this.focus());
}
}
};

TextEditor.prototype.hideEditableElement = function() {
// IE and Edge have the bug where the caret of the editable elements (eg. input, texarea) is always visible
// despite the element is overlapped by another element. To hide element we need to move element out of the viewport.
if (isMSBrowser()) {
this.textareaParentStyle.top = '-9999px';
this.textareaParentStyle.left = '-9999px';
} else {
// For other browsers hide element under Handsontable itself.
this.textareaParentStyle.top = '0px';
this.textareaParentStyle.left = '0px';
}

this.textareaParentStyle.top = '-9999px';
this.textareaParentStyle.left = '-9999px';
this.textareaParentStyle.zIndex = '-1';
};

Expand All @@ -90,6 +90,10 @@ TextEditor.prototype.setValue = function(newValue) {
};

TextEditor.prototype.beginEditing = function(newInitialValue, event) {
if (this.state !== EditorState.VIRGIN) {
return;
}

this.TEXTAREA.value = ''; // Remove an empty space from texarea (added by copyPaste plugin to make copy/paste functionality work with IME).
BaseEditor.prototype.beginEditing.apply(this, arguments);
};
Expand Down Expand Up @@ -215,6 +219,7 @@ TextEditor.prototype.focus = function() {

TextEditor.prototype.createElements = function() {
this.TEXTAREA = document.createElement('TEXTAREA');
this.TEXTAREA.tabIndex = -1;

addClass(this.TEXTAREA, 'handsontableInput');

Expand Down
2 changes: 1 addition & 1 deletion src/tableView.js
View file
Expand Up @@ -140,7 +140,7 @@ function TableView(instance) {
if (outsideClickDeselects) {
instance.deselectCell();
} else {
instance.destroyEditor();
instance.destroyEditor(false, false);
}
});

Expand Down
200 changes: 0 additions & 200 deletions test/e2e/Core_selection.spec.js
View file
Expand Up @@ -77,206 +77,6 @@ describe('Core_selection', () => {
expect(getSelected()).toBeUndefined();
});

it('should not deselect the currently selected cell after clicking on a scrollbar', () => {
var hot = handsontable({
outsideClickDeselects: false,
minRows: 20,
minCols: 2,
width: 400,
height: 100
});
selectCell(0, 0);

var holderBoundingBox = hot.view.wt.wtTable.holder.getBoundingClientRect(),
verticalScrollbarCoords = {
x: holderBoundingBox.left + holderBoundingBox.width - 3,
y: holderBoundingBox.top + (holderBoundingBox.height / 2)
},
horizontalScrollbarCoords = {
x: holderBoundingBox.left + (holderBoundingBox.width / 2),
y: holderBoundingBox.top + holderBoundingBox.height - 3
};

$(hot.view.wt.wtTable.holder).simulate('mousedown', {
clientX: verticalScrollbarCoords.x,
clientY: verticalScrollbarCoords.y
});

expect(getSelected()).toEqual([[0, 0, 0, 0]]);

$(hot.view.wt.wtTable.holder).simulate('mousedown', {
clientX: horizontalScrollbarCoords.x,
clientY: horizontalScrollbarCoords.y
});

expect(getSelected()).toEqual([[0, 0, 0, 0]]);
});

it('should not deselect currently selected cell', () => {
handsontable({
outsideClickDeselects: false
});
selectCell(0, 0);

$('html').simulate('mousedown');

expect(getSelected()).toEqual([[0, 0, 0, 0]]);
});

it('should allow to focus on external input and hold current selection informations', () => {
var textarea = $('<input id="test_textarea" type="text">').prependTo($('body'));

handsontable({
outsideClickDeselects: false
});
selectCell(0, 0);

textarea.simulate('mousedown');
textarea.focus();

expect(document.activeElement.id).toEqual('test_textarea');
expect(getSelected()).toEqual([[0, 0, 0, 0]]);
textarea.remove();
});

it('should allow to type in external input while holding current selection information', () => {
var textarea = $('<textarea id="test_textarea"></textarea>').prependTo($('body'));
var keyPressed;
handsontable({
outsideClickDeselects: false
});
selectCell(0, 0);

textarea.focus();
textarea.simulate('mousedown');
textarea.simulate('mouseup');

textarea.on('keydown', (event) => {
keyPressed = event.keyCode;
});

var LETTER_A_KEY = 97;

$(document.activeElement).simulate('keydown', {
keyCode: LETTER_A_KEY
});

// textarea should receive the event and be an active element
expect(keyPressed).toEqual(LETTER_A_KEY);
expect(document.activeElement).toBe(document.getElementById('test_textarea'));

// should preserve selection, close editor and save changes
expect(getSelected()).toEqual([[0, 0, 0, 0]]);
expect(getDataAtCell(0, 0)).toBeNull();

textarea.remove();
});

it('should allow to type in external input after opening cell editor', () => {
var textarea = $('<textarea id="test_textarea"></textarea>').prependTo($('body'));
var keyPressed;
handsontable({
outsideClickDeselects: false
});
selectCell(0, 0);
keyDown('enter');
document.activeElement.value = 'Foo';

textarea.focus();
textarea.simulate('mousedown');
textarea.simulate('mouseup');

textarea.on('keydown', (event) => {
keyPressed = event.keyCode;
});

var LETTER_A_KEY = 97;

$(document.activeElement).simulate('keydown', {
keyCode: LETTER_A_KEY
});

// textarea should receive the event and be an active element
expect(keyPressed).toEqual(LETTER_A_KEY);
expect(document.activeElement).toBe(document.getElementById('test_textarea'));

// should preserve selection, close editor and save changes
expect(getSelected()).toEqual([[0, 0, 0, 0]]);
expect(getDataAtCell(0, 0)).toEqual('Foo');

textarea.remove();
});

it('should deselect on outside click if outsideClickDeselects is a function that returns true', () => {
var textarea = $('<textarea id="test_textarea"></textarea>').prependTo($('body'));
var keyPressed;
handsontable({
outsideClickDeselects: () => true,
});
selectCell(0, 0);
keyDown('enter');
document.activeElement.value = 'Foo';

textarea.focus();
textarea.simulate('mousedown');
textarea.simulate('mouseup');

textarea.on('keydown', (event) => {
keyPressed = event.keyCode;
});

var LETTER_A_KEY = 97;

$(document.activeElement).simulate('keydown', {
keyCode: LETTER_A_KEY
});

// textarea should receive the event and be an active element
expect(keyPressed).toEqual(LETTER_A_KEY);
expect(document.activeElement).toBe(document.getElementById('test_textarea'));

// should NOT preserve selection
expect(getSelected()).toBeUndefined();
expect(getDataAtCell(0, 0)).toEqual('Foo');

textarea.remove();
});

it('should not deselect on outside click if outsideClickDeselects is a function that returns false', () => {
var textarea = $('<textarea id="test_textarea"></textarea>').prependTo($('body'));
var keyPressed;
handsontable({
outsideClickDeselects: () => false,
});
selectCell(0, 0);
keyDown('enter');
document.activeElement.value = 'Foo';

textarea.focus();
textarea.simulate('mousedown');
textarea.simulate('mouseup');

textarea.on('keydown', (event) => {
keyPressed = event.keyCode;
});

var LETTER_A_KEY = 97;

$(document.activeElement).simulate('keydown', {
keyCode: LETTER_A_KEY
});

// textarea should receive the event and be an active element
expect(keyPressed).toEqual(LETTER_A_KEY);
expect(document.activeElement).toBe(document.getElementById('test_textarea'));

// should preserve selection, close editor and save changes
expect(getSelected()).toEqual([[0, 0, 0, 0]]);
expect(getDataAtCell(0, 0)).toEqual('Foo');

textarea.remove();
});

it('should fix start range if provided is out of bounds (to the left)', () => {
handsontable({
startRows: 5,
Expand Down

0 comments on commit 932486b

Please sign in to comment.