Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TextInput event is not raised on typing (#closes 1956) #2303

Merged
merged 5 commits into from
May 10, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 81 additions & 25 deletions src/client/automation/playback/type/type-text.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import testCafeCore from '../../deps/testcafe-core';
import nextTick from '../../utils/next-tick';

var browserUtils = hammerhead.utils.browser;
var eventSandbox = hammerhead.sandbox.event;
var eventSimulator = hammerhead.eventSandbox.eventSimulator;
var listeners = hammerhead.eventSandbox.listeners;

Expand Down Expand Up @@ -64,6 +65,26 @@ function _typeTextInElementNode (elementNode, text, offset) {
textSelection.selectByNodesAndOffsets(selectPosition, selectPosition);
}

function _typeTextInChildTextNode (element, selection, text) {
let startNode = selection.startPos.node;

// NOTE: startNode could be moved or deleted on textInput event. Need ensure startNode.
if (!domUtils.isElementContainsNode(element, startNode)) {
selection = _excludeInvisibleSymbolsFromSelection(_getSelectionInElement(element));
startNode = selection.startPos.node;
}

const startOffset = selection.startPos.offset;
const endOffset = selection.endPos.offset;
const nodeValue = startNode.nodeValue;
const selectPosition = { node: startNode, offset: startOffset + text.length };

startNode.nodeValue = nodeValue.substring(0, startOffset) + text +
nodeValue.substring(endOffset, nodeValue.length);

textSelection.selectByNodesAndOffsets(selectPosition, selectPosition);
}

function _excludeInvisibleSymbolsFromSelection (selection) {
var startNode = selection.startPos.node;
var startOffset = selection.startPos.offset;
Expand All @@ -84,26 +105,68 @@ function _excludeInvisibleSymbolsFromSelection (selection) {
return selection;
}

// NOTE: Typing can be prevented in Chrome/Edge but can not be prevented in IE11 or Firefox
// Firefox does not support TextInput event
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually write comments this way

// NOTE: some text
// text without tab indent

// Safari supports the TextInput event but has a bug: e.data is added to the node value.
// So in Safari we need to call preventDefault in the last textInput handler but not prevent the Input event

let forceInputInSafari;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually I don't like nested methods, but in this case I would love to encapsulate logic for Safari in simulateTextInput method. We can move forceInputInSafari, onSafariTextInput, and onSafariPreventTextInput in it.
@AndreyBelym


function simulateTextInput (element, text) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this method almost all textInput logic is written for Safari. May be it would be more clear if we e.g.:

function simulateTextInput (element, text) {
    //logic only for Safari
}

var beforeContentChanged = () => {
        if (browserUtils.isSafari)
            needProcessInput = simulateTextInput(element, text);
        else
            needProcessInput = browserUtils.isFirefox || eventSimulator.textInput(element, text) || browserUtils.isIE11;

        needRaiseInputEvent = needProcessInput && !browserUtils.isIE11;
};	

What do you think @AlexKamaev @AndreyBelym ?

Copy link
Contributor Author

@AlexKamaev AlexKamaev May 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use simulateTextInput not only in beforeContentChanged but also in _typeTextToTextEditable.
If we detach safari logic from eventSimulator.textInput(...) we will need to duplicate safari logic

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok then

if (browserUtils.isSafari) {
listeners.addInternalEventListener(window, ['textInput'], onSafariTextInput);
eventSandbox.on(eventSandbox.EVENT_PREVENTED_EVENT, onSafariPreventTextInput);
}

const isInputEventRequired = browserUtils.isFirefox || eventSimulator.textInput(element, text) || forceInputInSafari;

if (browserUtils.isSafari) {
listeners.removeInternalEventListener(window, ['textInput'], onSafariTextInput);
eventSandbox.off(eventSandbox.EVENT_PREVENTED_EVENT, onSafariPreventTextInput);
}

return isInputEventRequired || browserUtils.isIE11;
}

function onSafariTextInput (e) {
e.preventDefault();

forceInputInSafari = true;
}

function onSafariPreventTextInput (e) {
if (e.type === 'textInput')
forceInputInSafari = false;
}

function _typeTextToContentEditable (element, text) {
var currentSelection = _getSelectionInElement(element);
var startNode = currentSelection.startPos.node;
var endNode = currentSelection.endPos.node;
var currentSelection = _getSelectionInElement(element);
var startNode = currentSelection.startPos.node;
var endNode = currentSelection.endPos.node;
var needProcessInput = true;
var needRaiseInputEvent = true;

// NOTE: some browsers raise the 'input' event after the element
// content is changed, but in others we should do it manually.
var inputEventRaised = false;

var onInput = () => {
inputEventRaised = true;
needRaiseInputEvent = false;
};

// NOTE: IE11 does not raise input event when type to contenteditable

var beforeContentChanged = () => {
needProcessInput = simulateTextInput(element, text);
needRaiseInputEvent = needProcessInput && !browserUtils.isIE11;
};

var afterContentChanged = () => {
nextTick()
.then(() => {
if (!inputEventRaised)
if (needRaiseInputEvent)
eventSimulator.input(element);

listeners.removeInternalEventListener(window, 'input', onInput);
listeners.removeInternalEventListener(window, ['input'], onInput);
});
};

Expand All @@ -126,27 +189,16 @@ function _typeTextToContentEditable (element, text) {
if (!startNode || !domUtils.isContentEditableElement(startNode) || !domUtils.isRenderedNode(startNode))
return;

// NOTE: we can type only to the text nodes; for nodes with the 'element-node' type, we use a special behavior
if (domUtils.isElementNode(startNode)) {
_typeTextInElementNode(startNode, text, currentSelection.startPos.offset);
beforeContentChanged();

afterContentChanged();
return;
if (needProcessInput) {
// NOTE: we can type only to the text nodes; for nodes with the 'element-node' type, we use a special behavior
if (domUtils.isElementNode(startNode))
_typeTextInElementNode(startNode, text);
else
_typeTextInChildTextNode(element, _excludeInvisibleSymbolsFromSelection(currentSelection), text);
}

currentSelection = _excludeInvisibleSymbolsFromSelection(currentSelection);
startNode = currentSelection.startPos.node;

var startOffset = currentSelection.startPos.offset;
var endOffset = currentSelection.endPos.offset;
var nodeValue = startNode.nodeValue;
var selectPosition = { node: startNode, offset: startOffset + text.length };

startNode.nodeValue = nodeValue.substring(0, startOffset) + text +
nodeValue.substring(endOffset, nodeValue.length);

textSelection.selectByNodesAndOffsets(selectPosition, selectPosition);

afterContentChanged();
}

Expand All @@ -156,6 +208,10 @@ function _typeTextToTextEditable (element, text) {
var startSelection = textSelection.getSelectionStart(element);
var endSelection = textSelection.getSelectionEnd(element);
var isInputTypeNumber = domUtils.isInputElement(element) && element.type === 'number';
var needProcessInput = simulateTextInput(element, text);

if (!needProcessInput)
return;

// NOTE: the 'maxlength' attribute doesn't work in all browsers. IE still doesn't support input with the 'number' type
var elementMaxLength = !browserUtils.isIE && isInputTypeNumber ? null : parseInt(element.maxLength, 10);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,9 @@ $(document).ready(function () {
var fixedText = 'Test' + String.fromCharCode(160) + 'me' + String.fromCharCode(160) + 'all!';
var inputEventRaisedCount = 0;

// NOTE IE11 does not raise input event on contenteditable element
var expectedInputEventRaisedCount = !browserUtils.isIE11 ? 12 : 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's preferred to use strait condition where it's possible
var expectedInputEventRaisedCount = browserUtils.isIE11 ? 0 : 12;


$el = $('#2');

function onInput () {
Expand All @@ -454,7 +457,7 @@ $(document).ready(function () {
.then(function () {
checkSelection($el, $el[0].childNodes[2], 4 + text.length, $el[0].childNodes[2], 4 + text.length);
equal($.trim($el[0].childNodes[2].nodeValue), 'with' + fixedText + ' br');
equal(inputEventRaisedCount, 12);
equal(inputEventRaisedCount, expectedInputEventRaisedCount);
$el.unbind('input', onInput);

startNext();
Expand All @@ -465,6 +468,9 @@ $(document).ready(function () {
var text = 'Test';
var inputEventRaisedCount = 0;

// NOTE IE11 does not raise input event on contenteditable element
var expectedInputEventRaisedCount = !browserUtils.isIE11 ? 4 : 0;

$el = $('#8');

function onInput () {
Expand All @@ -479,7 +485,7 @@ $(document).ready(function () {
.run()
.then(function () {
equal($.trim($el[0].textContent), text);
equal(inputEventRaisedCount, 4);
equal(inputEventRaisedCount, expectedInputEventRaisedCount);
$el.unbind('input', onInput);

startNext();
Expand Down
3 changes: 2 additions & 1 deletion test/functional/fixtures/regression/gh-1054/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ describe('[Regression](GH-1054)', function () {
return runTests('testcafe-fixtures/index.test.js', 'Type text in the input');
});

// NOTE: IE11 does not raise Input event on contenteditable element
it('Typing in the content editable element with "replace" option', function () {
return runTests('testcafe-fixtures/index.test.js', 'Type text in the content editable element');
return runTests('testcafe-fixtures/index.test.js', 'Type text in the content editable element', { skip: [ 'ie' ] });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it. I can paste text in contenteditable by ctrl+v shortcut in IE. I think we should execute typeText action with replace option for IE. Do we?

Copy link
Contributor Author

@AlexKamaev AlexKamaev May 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test checks if input event happened. But in IE in contenteditable element input event never raised. So test is incorrect now for ie. We can dublicate this test for IE only without this additional check

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we skip only input event check for IE and leave check for typing itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

});
});
});
105 changes: 105 additions & 0 deletions test/functional/fixtures/regression/gh-1956/pages/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
<html>
<head><title>Edit test</title></head>
<style>
h2 {
margin-top: 20px;
}
div {
border: 1px solid black;
min-height: 10px;
}
</style>
<script type="text/javascript">
function onTextInput (event) {
var needPreventDefault = true;

if (isTargetElement(event.target, 'contentEditableWithModify'))
changeNodeValueOnTextInput(event);

if (isTargetElement(event.target, 'contentEditableWithReplace')) {
replaceEditableElementOnTextInput(event);

needPreventDefault = false;
}

if(needPreventDefault)
event.preventDefault();
}

function changeNodeValueOnTextInput(event) {
document.getElementById('contentEditableWithModify').childNodes[0].childNodes[0].nodeValue += event.data;
}

function replaceEditableElementOnTextInput (event) {
if (window.preventNextElementReplacement)
return;

window.preventNextElementReplacement = true;

var div = document.getElementById('contentEditableWithReplace');
var paragraph = div.childNodes[0];
var textNode = paragraph.childNodes[0];
var newParagraph = document.createElement("P");

newParagraph.innerHTML = 'X';

div.removeChild(paragraph);
div.appendChild(newParagraph);
newParagraph.focus();
}

function onInput (event) {
if (!isTargetElement(event.target, 'contentEditableWithReplace'))
throw new Error('Input event has raised');
}

function isTargetElement(el, id) {
while (el) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can store required elements (such as const contentEditableWithModify = document.getElementById('contentEditableWithModify')) and use DOM element contains method

if (el.id === id)
return true;

el = el.parentNode;
}

return null;
}

document.addEventListener('textInput', onTextInput, true);
document.addEventListener('textinput', onTextInput, true);
document.addEventListener('input', onInput, true);
</script>
<body>
<h2>Prevent Input event on TextInput when type to input element</h2>
<pre>
Chrome/Edge. Typing is prevented and Input event is not raised
IE11/Firefox. Typing is not prevented. Input event is raised
</pre>
<input id="simpleInput" />
<h2>Prevent Input event and typing on simple ContentEditable div</h2>
<pre>
Chrome/Edge. Typing is prevented. Input event is not raised
IE11/Firefox. Typing is not prevented.
Input event is raised in firefox but is not raised in IE11 - it's a IE11 bug
</pre>
<div contenteditable="true" id="simpleContentEditable"></div>
<h2>Prevent Input event on TextInput event when type to element node</h2>
<pre>
Not for IE11 because preventDefault will not prevent typing
Not for Firefox because Firefox does not support TextInput event
</pre>
<div contenteditable="true" id="contentEditableWithElementNode"><p><br/></p></div>
<h2>Modify text node of ContentEditable div on TextInput event and prevent Input event</h2>
<pre>
Not for IE11 because is's not possible to prevent typing in IE11
Not for Firefox because Firefox does not support TextInput event
</pre>
<div contenteditable="true" id="contentEditableWithModify"><p>A</p></div>
<h2>Type to ContentEditable div when selected node was replaced on TextInput event</h2>
<pre>
Not for IE11 because this test emulates behavior from https://github.com/DevExpress/testcafe/issues/1956.
This behavior is different in IE11
Not for Firefox because Firefox does not support TextInput event
</pre>
<div contenteditable="true" id="contentEditableWithReplace"><p>B</p></div>
</body>
</html>
63 changes: 63 additions & 0 deletions test/functional/fixtures/regression/gh-1956/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
var expect = require('chai').expect;

var browsersWithLimitations = [ 'ie', 'firefox', 'firefox-osx' ];

describe('Should support TextInput event[Regression](GH-1956)', function () {
it('Prevent Input event on TextInput when type to input element', function () {
return runTests('testcafe-fixtures/index.js',
'Prevent Input event on TextInput when type to input element',
{ skip: browsersWithLimitations });
});

it('Prevent Input event on TextInput when type to input element IE11/Firefox', function () {
return runTests('testcafe-fixtures/index.js',
'Prevent Input event on TextInput when type to input element IE11/Firefox',
{ only: [ 'ie', 'firefox', 'firefox-osx' ], shouldFail: true })
.catch(function (errs) {
var errors = [ errs['ie'], errs['firefox'] ].filter(err => err);

errors.forEach(err => {
expect(err[0]).contains('Input event has raised');
});
});
});

it('Prevent Input event on TextInput when type to ContentEditable div', function () {
return runTests('testcafe-fixtures/index.js',
'Prevent Input event on TextInput when type to ContentEditable div',
{ skip: browsersWithLimitations });
});

it('Prevent Input event on TextInput when type to ContentEditable div IE11', function () {
return runTests('testcafe-fixtures/index.js',
'Prevent Input event on TextInput when type to ContentEditable div IE11/Firefox',
{ only: [ 'ie' ] });
});

it('Prevent Input event on TextInput when type to ContentEditable div Firefox', function () {
return runTests('testcafe-fixtures/index.js',
'Prevent Input event on TextInput when type to ContentEditable div IE11/Firefox',
{ only: [ 'firefox', 'firefox-osx' ], shouldFail: true })
.catch(function (errs) {
expect(errs[0]).contains('Input event has raised');
});
});

it('Modify text node of ContentEditable div on TextInput event and prevent Input event', function () {
return runTests('testcafe-fixtures/index.js',
'Modify text node of ContentEditable div on TextInput event and prevent Input event',
{ skip: browsersWithLimitations });
});

it('Type to ContentEditable div when selected node was replaced on TextInput event', function () {
return runTests('testcafe-fixtures/index.js',
'Type to ContentEditable div when selected node was replaced on TextInput event',
{ skip: browsersWithLimitations });
});

it('Prevent Input event on TextInput when type to element node', function () {
return runTests('testcafe-fixtures/index.js',
'Prevent Input event on TextInput when type to element node',
{ skip: browsersWithLimitations });
});
});
Loading