Should type in contenteditable div even if it has invisible child with contenteditable=false (#2205) #2222

AlexKamaev · 2018-03-16T07:35:36Z

…

testcafe-build-bot · 2018-03-16T08:01:11Z

❌ Tests for the commit 8f15bfe have failed. See details:

testcafe-build-bot · 2018-03-16T10:03:46Z

❌ Tests for the commit 5ec78da have failed. See details.

testcafe-build-bot · 2018-03-16T11:44:54Z

✅ Tests for the commit 4cbb952 have passed. See details:

testcafe-build-bot · 2018-03-16T16:13:09Z

✅ Tests for the commit 2f19d9d have passed. See details:

testcafe-build-bot · 2018-03-16T16:30:14Z

✅ Tests for the commit 2f19d9d have passed. See details:

testcafe-build-bot · 2018-03-16T17:01:12Z

✅ Tests for the commit 2f19d9d have passed. See details:

AlexKamaev · 2018-03-20T06:52:00Z

Here we use isDisplayNone instead of IsElementHidden, because the situation is possible when textNode is inside a div with height = 0 and width = 0. In this case text is still visible, so I think textNode should not be ignored

AlexanderMoskovkin

need to check visibility:hidden too

testcafe-build-bot · 2018-03-23T16:00:30Z

❌ Tests for the commit 5636020 have failed. See details:

testcafe-build-bot · 2018-03-27T12:12:25Z

❌ Tests for the commit 5fe7023 have failed. See details:

…d with contenteditable=false(closes DevExpress#2205)

testcafe-build-bot · 2018-04-02T14:20:29Z

❌ Tests for the commit 00fc15b have failed. See details:

testcafe-build-bot · 2018-04-02T15:18:15Z

❌ Tests for the commit 00fc15b have failed. See details:

testcafe-build-bot · 2018-04-02T20:34:33Z

✅ Tests for the commit aef9b68 have passed. See details:

AlexKamaev · 2018-04-03T05:32:21Z

FPR

AlexanderMoskovkin · 2018-04-05T15:26:42Z

/cc @helen-dikareva

helen-dikareva

nice work)

helen-dikareva · 2018-04-09T12:48:29Z

src/client/core/utils/content-editable.js

-function hasChildren (node) {
-    return node.childNodes && domUtils.getChildNodesLength(node.childNodes);
+function getVisibleChildren (node) {
+    return arrayUtils.filter(node.childNodes, child => domUtils.isTextNode(child) || domUtils.isElementNode(child) && styleUtils.isElementVisible(child));


should we use isVisibleTextNode for textNodes?

No, by using getVisibleChilndren I need to ignore elements which has display:none or visibility:hidden

helen-dikareva · 2018-04-09T12:50:25Z

src/client/core/utils/content-editable.js

+}
+
+function hasVisibleChildren (node) {
+    return getVisibleChildren(node).length;


faster will be use arrayUtils.some(node.childNodes,.... To do it we need to extract predicate function from get Visible Children.

helen-dikareva · 2018-04-09T14:33:05Z

src/client/core/utils/content-editable.js

+    const selectableChildren    = arrayUtils.filter(visibleChildren, child => isNodeSelectable(child));
+    const breakLineElements     = arrayUtils.filter(visibleChildren, child => domUtils.getTagName(child) === 'br');
+
+    return selectableChildren.length ? includeDescendants : isContentEditableRoot || breakLineElements.length;


We ussually simplify this type of ternary expression :isContentEditableRoot || breakLineElements.length

helen-dikareva · 2018-04-09T15:19:09Z

src/client/core/utils/style.js


-    return el && get(el, 'visibility') === 'hidden';
+var isVisibilityHiddenTextNode = function (textNode) {


we already have had styleUtils.isNotVisibleNode please check methods. Can we merge them?

For my purposes I use isElementVisible from hammerhead.js which fits to me. As for other methods from style.js, I've just noticed that isVisibilityHiddenTextNode does not check all ancestors, it looks strange. So I've added isVisibilityHiddenNode method which checks only visibility:hidden property and then I used this method in isVisibilityHiddenTextNode.
As for styleUtils.isNotVisibleNode it also looks really strange:

export function isNotVisibleNode (node) { return !domUtils.isRenderedNode(node) || isHiddenNode(node) || isVisibilityHiddenTextNode(node); }

It checks visibility:hidden only if node is text.
I think we need to review all these methods and try to refactor them. But I would prefer to creafe the separate issue.

It seems like isNotVisibleNode method has incorrect name, because it used only for text nodes

if (domUtils.isTextNode(el)) return !styleUtils.isNotVisibleNode(el);

But the name doesn't contains info about that.

I think we need to review all these methods and try to refactor them. But I would prefer to creafe the separate issue.

I agree with this, but please rename isNotVisibleNode -> isNotVisibleTextNode now. Don't you mind?

testcafe-build-bot · 2018-04-11T05:43:57Z

❌ Tests for the commit d474b5d have failed. See details:

testcafe-build-bot · 2018-04-11T07:14:03Z

✅ Tests for the commit d474b5d have passed. See details:

AlexKamaev · 2018-04-11T07:20:25Z

please review changes and comments

helen-dikareva · 2018-04-11T08:22:31Z

src/client/core/utils/content-editable.js

+    const visibleChildren       = getVisibleChildren(node);
+    const isContentEditableRoot = !domUtils.isContentEditableElement(node.parentNode);
+    const selectableChildren    = arrayUtils.filter(visibleChildren, child => isNodeSelectable(child));
+    const breakLineElements     = arrayUtils.filter(visibleChildren, child => domUtils.getTagName(child) === 'br');


We don't have to find all br elements, so we can use arrayUtils.some here

helen-dikareva · 2018-04-11T08:34:36Z

src/client/core/utils/style.js


-    return el && get(el, 'visibility') === 'hidden';
+var isVisibilityHiddenTextNode = function (textNode) {


It seems like isNotVisibleNode method has incorrect name, because it used only for text nodes

if (domUtils.isTextNode(el)) return !styleUtils.isNotVisibleNode(el);

But the name doesn't contains info about that.

helen-dikareva · 2018-04-11T08:37:05Z

src/client/core/utils/style.js


-    return el && get(el, 'visibility') === 'hidden';
+var isVisibilityHiddenTextNode = function (textNode) {


I think we need to review all these methods and try to refactor them. But I would prefer to creafe the separate issue.

I agree with this, but please rename isNotVisibleNode -> isNotVisibleTextNode now. Don't you mind?

helen-dikareva · 2018-04-11T08:54:45Z

test/functional/fixtures/regression/gh-2205/testcafe-fixtures/index.js

+    await t
+        .click(editor)
+        .typeText(editor, 'Hello')
+        .wait(1000)


Why we need to wait? Can we increase selectorTimeout here?

testcafe-build-bot · 2018-04-11T14:00:12Z

✅ Tests for the commit 36d29f7 have passed. See details:

helen-dikareva · 2018-04-12T08:38:03Z

src/client/core/utils/content-editable.js

+    const selectableChildren    = arrayUtils.filter(visibleChildren, child => isNodeSelectable(child));
+    const hasBreakLineElements  = arrayUtils.some(visibleChildren, child => domUtils.getTagName(child) === 'br');
+
+    if (selectableChildren.length)


Let's add includeDescendants arg to hasSelectableChildren method and use it here

testcafe-build-bot · 2018-04-12T12:35:23Z

✅ Tests for the commit 2524cdc have passed. See details:

…h contenteditable=false (DevExpress#2205) (DevExpress#2222) * [WIP]Should type in contenteditable div even if it has invisible child with contenteditable=false(closes DevExpress#2205) * add one more test case * fix tests on safari osx * fix changes after request * fixes after review * refactoring

AlexKamaev force-pushed the gh-2205 branch 2 times, most recently from 9e4c057 to 5ec78da Compare March 16, 2018 09:27

AlexKamaev force-pushed the gh-2205 branch 2 times, most recently from 027208c to 4cbb952 Compare March 16, 2018 11:16

AlexKamaev closed this Mar 16, 2018

AlexKamaev reopened this Mar 16, 2018

AlexKamaev force-pushed the gh-2205 branch 2 times, most recently from d1885a9 to 2f19d9d Compare March 16, 2018 11:56

AlexKamaev closed this Mar 16, 2018

AlexKamaev reopened this Mar 16, 2018

AlexKamaev closed this Mar 16, 2018

AlexKamaev reopened this Mar 16, 2018

DevExpress deleted a comment from testcafe-build-bot Mar 16, 2018

AlexKamaev changed the title ~~[WIP]Should type in contenteditable div even if it has invisible child with contenteditable=false (#2205)~~ Should type in contenteditable div even if it has invisible child with contenteditable=false (#2205) Mar 16, 2018

AlexKamaev requested review from AndreyBelym and AlexanderMoskovkin March 16, 2018 22:25

AlexanderMoskovkin suggested changes Mar 23, 2018

View reviewed changes

AlexKamaev force-pushed the gh-2205 branch from 2f19d9d to 5636020 Compare March 23, 2018 15:10

AlexKamaev force-pushed the gh-2205 branch from 5636020 to 5fe7023 Compare March 27, 2018 11:16

[WIP]Should type in contenteditable div even if it has invisible chil…

3ea208a

…d with contenteditable=false(closes DevExpress#2205)

AlexKamaev force-pushed the gh-2205 branch from 00fc15b to 03914a7 Compare April 2, 2018 16:42

fix tests on safari osx

aef9b68

AlexKamaev force-pushed the gh-2205 branch from 03914a7 to aef9b68 Compare April 2, 2018 17:45

AlexanderMoskovkin requested review from helen-dikareva and removed request for AndreyBelym April 5, 2018 15:26

helen-dikareva requested changes Apr 9, 2018

View reviewed changes

fix changes after request

d474b5d

AlexKamaev closed this Apr 10, 2018

AlexKamaev reopened this Apr 10, 2018

AlexKamaev closed this Apr 11, 2018

AlexKamaev reopened this Apr 11, 2018

helen-dikareva reviewed Apr 11, 2018

View reviewed changes

fixes after review

36d29f7

helen-dikareva reviewed Apr 12, 2018

View reviewed changes

helen-dikareva approved these changes Apr 12, 2018

View reviewed changes

AlexanderMoskovkin approved these changes Apr 12, 2018

View reviewed changes

refactoring

2524cdc

AlexKamaev force-pushed the gh-2205 branch from e4b538a to 2524cdc Compare April 12, 2018 11:48

AlexKamaev merged commit 0f83b12 into DevExpress:master Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Should type in contenteditable div even if it has invisible child with contenteditable=false (#2205) #2222

Should type in contenteditable div even if it has invisible child with contenteditable=false (#2205) #2222

AlexKamaev commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

AlexKamaev commented Mar 20, 2018

AlexanderMoskovkin left a comment

testcafe-build-bot commented Mar 23, 2018

testcafe-build-bot commented Mar 27, 2018

testcafe-build-bot commented Apr 2, 2018

testcafe-build-bot commented Apr 2, 2018

testcafe-build-bot commented Apr 2, 2018

AlexKamaev commented Apr 3, 2018

AlexanderMoskovkin commented Apr 5, 2018

helen-dikareva left a comment

helen-dikareva Apr 9, 2018

AlexKamaev Apr 10, 2018

helen-dikareva Apr 9, 2018

helen-dikareva Apr 9, 2018

helen-dikareva Apr 9, 2018

AlexKamaev Apr 10, 2018

helen-dikareva Apr 11, 2018

helen-dikareva Apr 11, 2018

testcafe-build-bot commented Apr 11, 2018

testcafe-build-bot commented Apr 11, 2018

AlexKamaev commented Apr 11, 2018

helen-dikareva Apr 11, 2018

helen-dikareva Apr 11, 2018

helen-dikareva Apr 11, 2018

helen-dikareva Apr 11, 2018

testcafe-build-bot commented Apr 11, 2018

helen-dikareva Apr 12, 2018

testcafe-build-bot commented Apr 12, 2018


		return el && get(el, 'visibility') === 'hidden';
		var isVisibilityHiddenTextNode = function (textNode) {

Should type in contenteditable div even if it has invisible child with contenteditable=false (#2205) #2222

Should type in contenteditable div even if it has invisible child with contenteditable=false (#2205) #2222

Conversation

AlexKamaev commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

testcafe-build-bot commented Mar 16, 2018

AlexKamaev commented Mar 20, 2018

AlexanderMoskovkin left a comment

Choose a reason for hiding this comment

testcafe-build-bot commented Mar 23, 2018

testcafe-build-bot commented Mar 27, 2018

testcafe-build-bot commented Apr 2, 2018

testcafe-build-bot commented Apr 2, 2018

testcafe-build-bot commented Apr 2, 2018

AlexKamaev commented Apr 3, 2018

AlexanderMoskovkin commented Apr 5, 2018

helen-dikareva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented Apr 11, 2018

testcafe-build-bot commented Apr 11, 2018

AlexKamaev commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented Apr 11, 2018

Choose a reason for hiding this comment

testcafe-build-bot commented Apr 12, 2018