TextInput event is not raised on typing (#closes 1956) #2303
Conversation
|
❌ Tests for the commit b55ba99 have failed. See details: |
|
❌ Tests for the commit 2bc8b23 have failed. See details: |
|
❌ Tests for the commit 7308cd8 have failed. See details: |
|
❌ Tests for the commit 17b3c67 have failed. See details: |
|
❌ Tests for the commit fe41fee have failed. See details: |
|
❌ Tests for the commit 98b99a6 have failed. See details: |
|
❌ Tests for the commit 36d50a4 have failed. See details: |
AlexKamaev
force-pushed
the
gh-1956
branch
2 times, most recently
from
9fb0806
to
e35d579
Compare
|
❌ Tests for the commit e35d579 have failed. See details: |
|
❌ Tests for the commit f65dfc8 have failed. See details: |
|
❌ Tests for the commit 17814af have failed. See details: |
|
❌ Tests for the commit cfae5da have failed. See details: |
|
@testcafe-build-bot \retest |
|
❌ Tests for the commit cfae5da have failed. See details: |
|
✅ Tests for the commit d9ca271 have passed. See details: |
|
❌ Tests for the commit a246d72 have failed. See details: |
|
@testcafe-build-bot \retest |
|
❌ Tests for the commit a246d72 have failed. See details: |
|
@testcafe-build-bot \retest |
|
❌ Tests for the commit a246d72 have failed. See details: |
|
✅ Tests for the commit a246d72 have passed. See details: |
|
✅ Tests for the commit 4136a94 have passed. See details: |
|
❌ Tests for the commit 0dff55c have failed. See details: |
2 similar comments
|
❌ Tests for the commit 0dff55c have failed. See details: |
|
❌ Tests for the commit 0dff55c have failed. See details: |
|
❌ Tests for the commit abd0361 have failed. See details: |
|
@testcafe-build-bot \retest |
|
❌ Tests for the commit abd0361 have failed. See details: |
3 similar comments
|
❌ Tests for the commit abd0361 have failed. See details: |
|
❌ Tests for the commit abd0361 have failed. See details: |
|
❌ Tests for the commit abd0361 have failed. See details: |
|
❌ Tests for the commit b094470 have failed. See details: |
|
✅ Tests for the commit b094470 have passed. See details: |
| @@ -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 | |||
There was a problem hiding this comment.
we usually write comments this way
// NOTE: some text
// text without tab indent|
|
||
| let forceInputInSafari; | ||
|
|
||
| function simulateTextInput (element, text) { |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
| // 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; |
There was a problem hiding this comment.
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
| @@ -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; | |||
There was a problem hiding this comment.
It's preferred to use strait condition where it's possible
var expectedInputEventRaisedCount = browserUtils.isIE11 ? 0 : 12;
| 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' ] }); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Can we skip only input event check for IE and leave check for typing itself?
| } | ||
|
|
||
| function isTargetElement(el, id) { | ||
| while (el) { |
There was a problem hiding this comment.
we can store required elements (such as const contentEditableWithModify = document.getElementById('contentEditableWithModify')) and use DOM element contains method
|
✅ Tests for the commit 1827980 have passed. See details: |
|
❌ Tests for the commit 7186e62 have failed. See details: |
|
@testcafe-build-bot \retest |
|
✅ Tests for the commit 7186e62 have passed. See details: |
* [WIP] process TextInput event on typing (closes DevExpress#1956) * fix textInput dispatch in safari * fix textInput dispatch in safari * fix review remarks * fix user agent module
No description provided.