Widget has now correct selection behaviour inside tables #1104

msamsel · 2017-10-30T12:19:08Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

There is no unit test. Selection use quite unfriendly for testing purposes listener mechanism, which use timeouts to execute.
https://github.com/ckeditor/ckeditor-dev/blob/02c35d9ae44656bd75d0efd69028888195dd2a6f/core/selection.js#L272-L299
I wasn't able to properly repeat the problem. I tried to call just selectionChange event, also fire mouseup, mousedown events with target of different table cell. All those things remain selection on widget even tough problem was fixed already.

This PR contains

Unit tests
Manual tests

What changes did you make?

provide method which return information if element is either widget or widget wrapper.
use this method to omit some rules when table selection is checked. As far as I know only tableselection and widget use fake-selection, so this solution should be sufficient for this case

close #1027, close #522

f1ames

There is one case in which selection is not visible. When widget in the first table is selected and then text from the second table gets selected:

The selection is not visible, however it is there, because typing works fine (from what I see it happens only for non-collapsed selection).

Dragging widgets also looks a little strange. The widget is dragged properly and placed in the proper cell (you can compare how it behaves in widgetintable and widgetintablewithtableselection tests), however due to a fact that custom table selection gets activated it looks strange (not sure if it is related to this fix, if not it should be extracted to separate issue):

Maybe you could add manual test covering #1027 scenario (you may put it
in tests/tickets/ directory)?

Also when writing test scenario, remember to put dots after each step (if it's a sentence):

1. Click into widgets in table.
1. Try to select some text **in table**.

f1ames · 2017-11-03T12:11:43Z

plugins/widget/plugin.js

+	 *
+	 * @since 4.8.0
+	 * @static
+	 * @param {CKEDItOR.dom.element} node


As CKEDITOR.dom.element inherits from CKEDITOR.dom.node and this method accepts both, shouldn't this param be {CKEDITOR.dom.node} node ?

Also there is a typo:

CKEDItOR

f1ames · 2017-11-03T12:18:07Z

plugins/widget/plugin.js

+	 * @param {CKEDItOR.dom.element} node
+	 * @returns {Boolean}
+	 */
+	Widget.isDomWidget = function( node ) {


Took me some time to understand why those methods are named like that (with the Dom part). It checks if given node is a widget element for CKEDITOR.dom.* representation. The other ones are with Parser (e.g. isParserWidgetElement) related to CKEDITOR.htmlParser.* namespace, makes sense 👍

f1ames · 2017-11-03T12:57:35Z

core/selection.js

@@ -96,6 +98,11 @@
 			return table.equals( fakeTable ) || fakeTable.contains( table );
 		}

+		// When widget is selected, then definately is not a table.


Issue number could be added here.

f1ames · 2017-11-03T12:57:54Z

core/selection.js

@@ -19,7 +19,9 @@
 		if ( ranges.length === 0 ) {
 			return false;
 		}
-
+		if ( CKEDITOR.plugins.widget && CKEDITOR.plugins.widget.isDomWidget( ranges[ 0 ].getEnclosedNode() ) ) {


Issue number could be added here (with some short comment maybe?).

f1ames · 2017-11-03T13:16:03Z

plugins/tableselection/plugin.js

@@ -269,15 +270,19 @@
 		if ( !evt.data.getTarget().getName ) {
 			return;
 		}
+		// Prevent of applying table selection when widget is selected.
+		// Mouseup remain possibility to finish table selection when user release mouse button above widget in table.
+		if ( evt.name !== 'mouseup' && CKEDITOR.plugins.widget && CKEDITOR.plugins.widget.isDomWidget( evt.data.getTarget() ) ) {


This line introduced one unexpected (I think), behaviour:

Before:

After:

so when dragging mouse over widgets, cells are not selected with fake selection as they would normally be.

Extracted to #1125

f1ames · 2017-11-03T13:20:32Z

tests/plugins/widget/integration/table/manual/widgetintable.html

+		}
+	} );
+
+	CKEDITOR.plugins.add( 'blockWidget', {


Maybe you could extract those widget definitions to some helper as they are duplicated in both manual tests?

f1ames · 2017-11-03T13:25:45Z

tests/plugins/widget/integration/table/manual/widgetintable.md

+
+----
+
+1. Click into widgets in table


Click on the widget in table

f1ames · 2017-11-03T13:27:43Z

tests/plugins/widget/integration/table/manual/widgetintable.md

+1. Click into widgets in table
+1. Try to select some text **in table**
+1. Perform some operation on selected text ( change, append new text, apply some style, etc. )
+1. Check behaviour inside inner and outer table depend of testing widget


This step is a little unclear. I assume it means to test for both text in nested and outer table. Maybe it can be skipped and added to the instructions below test scenario?

f1ames · 2017-11-03T13:29:10Z

tests/plugins/widget/integration/table/manual/widgetintable.md

+
+_Perform above steps for all widgets and both editors._
+
+**Expected:** Widget is deselect when focus is changed to text in table. You are able to perfom operatons on selected text.


Widget is deselect when focus is changed to text in table and text selection is visible. You are...

f1ames · 2017-11-03T13:32:50Z

tests/plugins/widget/integration/table/manual/widgetintable.md

+1. Try to select some text **in table**
+1. Perform some operation on selected text ( change, append new text, apply some style, etc. )
+1. Check behaviour inside inner and outer table depend of testing widget
+1. Try to add new widget in table ( buttons without icon in toolbr )


This looks a little out of place here. We can treat it as a last step of the scenario, but then it misses expected result. You can:

Extract it as a second test scenario (well, pretty short one).

Rearrange scenario in a way that the user/tester should add a widget on the beginning and then proceed with the rest of the steps (so added widget will also be a part of further testing).

f1ames · 2017-11-03T13:56:07Z

I see the case with widget drag&drop is reported in #710 (Case 1), the question is if the cause is the same so it can also be solved by this fix?

msamsel · 2017-11-06T14:35:59Z

First case with missing selection. Problem it's not related to this case and might be reproduced without widget. More details here: #1122

Dragging problems (Activation table selection, and problem with dropping to proper cell) are covered by #710 (case 1 and 2)

Problem with not having tableselection when going with cursor above the widgets is reported here: #1125

f1ames

Nice work with extracting all the issues 👍

Just some small corrections in tests needed.

f1ames · 2017-11-08T10:37:15Z

tests/plugins/widget/integration/table/manual/widgetintablewithtableselection.md

@@ -1,17 +1,17 @@
 @bender-tags: 1027, 4.8.0, widget
 @bender-ui: collapsed
-@bender-ckeditor-plugins: undo,clipboard,basicstyles,toolbar,wysiwygarea,widget,table,resize,tableselection
+@bender-include: ../_helpers/testwidgets.js
+@bender-ckeditor-plugins: undo,clipboard,basicstyles,toolbar,wysiwygarea,widget,table,resize


Missing tableresize plugin here.

I assume you have tableselection on your mind ;)
I've copy paste the description earlier and haven't notice that is missing now :(

f1ames · 2017-11-08T10:59:46Z

tests/tickets/1027/1.html

@@ -0,0 +1,27 @@
+<textarea id="editor">
+	<p>Lena's path: "../../_assets/lena.jpg"</p>


I suggest moving this path to test description (.md file) when you open the dialog you are unable to select and copy the path and I believe this is the main case here ;)

You may put it inside input so it will be easy to copy in IEs.

f1ames · 2017-11-08T12:05:06Z

tests/plugins/widget/integration/table/manual/widgetintablewithtableselection.md

@@ -0,0 +1,17 @@
+@bender-tags: 1027, 4.8.0, widget


This test could be ignored for IE8-10 as tableselection is not supported there so it will be exactly the same as .../manual/widgetintable test.

f1ames · 2017-11-08T12:07:37Z

tests/tickets/1027/1.md

@@ -0,0 +1,15 @@
+@bender-tags: 1027, 4.8.0, widget


This test could be ignored for IE8-10 as tableselection is not supported there.

mlewand · 2017-11-08T12:15:31Z

Gents, just one thing - I'd like to have @Comandeer a second look at it before it gets merged just for extra safety. So @Comandeer please give it a look once it gets a final R+ (I'll mark you already).

f1ames

LGTM 👍

f1ames · 2017-11-21T10:42:30Z

You may now take a look @Comandeer.

Comandeer

Overall seems very solid and well, I have only some stylistic objections.

Comandeer · 2017-11-22T13:01:08Z

core/selection.js

@@ -96,6 +99,11 @@
 			return table.equals( fakeTable ) || fakeTable.contains( table );
 		}

+		// When widget is selected, then definately is not a table (#1027).


Typo: "definitely".

Comandeer · 2017-11-22T13:01:47Z

plugins/tableselection/plugin.js

@@ -208,7 +208,8 @@
 			// The selection is inside one cell, so we should allow native selection,
 			// but only in case if no other cell between mousedown and mouseup
 			// was selected.
-			if ( !fakeSelection.dirty && cells.length === 1 ) {
+			// We don't want to clear selection if widget is event target (#1027).
+			if ( !fakeSelection.dirty && cells.length === 1 && !( CKEDITOR.plugins.widget && CKEDITOR.plugins.widget.isDomWidget( evt.data.getTarget() ) ) ) {


Maybe we could extract the second part to some function as it's repeated several times in the code?

It's repeated both here and in core/selection.js.

Comandeer · 2017-11-22T13:12:04Z

tests/tickets/1027/1.html

@@ -0,0 +1,34 @@
+<label>Lena's path: <input style="border:2px solid black" type="text" value="../../_assets/lena.jpg" readonly></label>


I'l move this test to plugins/tableselection/integrations directory

Comandeer · 2017-11-22T13:23:49Z

tests/plugins/widget/integration/table/manual/widgetintable.html

+	CKEDITOR.replace( 'divarea', {
+		removePlugins: 'tableselection',
+		extraPlugins: 'divarea,inlineWidget,blockWidget'
+	} )


Missing semicolon.

Comandeer · 2017-11-22T13:24:01Z

tests/plugins/widget/integration/table/manual/widgetintablewithtableselection.html

+
+	CKEDITOR.replace( 'divarea', {
+		extraPlugins: 'divarea,inlineWidget,blockWidget'
+	} )


Missing semicolon.

Comandeer · 2017-11-22T13:25:40Z

core/selection.js

@@ -226,6 +234,7 @@
 			realSel = this.getSelection( 1 );
 			// If real (not locked/stored) selection was moved from hidden container
 			// or is not a table one, then the fake-selection must be invalidated.
+


Seems like unnecessary blank line.

Comandeer

I found one more issue in IE8:

Open http://tests.ckeditor.test:1030/tests/plugins/widget/integration/table/manual/widgetintable
In the first row of the first table in the first editor place cursors after the widget (you'll probably have to focus the widget and press right arrow).
Try to add new widget.

Expected
New widget is inserted after the existing one.

Actual
Existing widget gets focus or is replaced by the new widget.

Comandeer · 2017-11-23T10:07:42Z

core/selection.js

@@ -11,6 +11,10 @@
 		fillingCharSequenceRegExp = new RegExp( fillingCharSequence + '( )?', 'g' ),
 		isSelectingTable;

+	function _isWidget( element ) {


Actually there isn't a need to prefix such function with _ as it's not a method. isWidget would be enough.

Comandeer · 2017-11-23T10:07:49Z

plugins/tableselection/plugin.js

@@ -16,6 +16,10 @@
 		insertRow,
 		insertColumn;

+	function _isWidget( element ) {


Actually there isn't a need to prefix such function with _ as it's not a method. isWidget would be enough.

msamsel · 2017-11-24T15:59:23Z

I've extracted IE8 issue to separate task #1226. It existed before my change.

Comandeer

LGTM! @f1ames, please, do the honors ;)

…vements. Also extracting widget definition in test.

…nts.

… fixes, reorganise tests path.

Yanual · 2017-12-28T06:38:14Z

I just downloaded the tableselection plugin on version 4.8 and got an error: "plugin.js: 20 Uncaught TypeError: CKEDITOR.plugins.widget.isDomWidget is not a function", I look at the functions available in the plugin widget and i try the isDomWidgetElement function and the error does not happen again. I would like to know if my modification is correct and why I have had this problem (am I the only one?) or if there is another sollution ?

mlewand · 2017-12-28T09:22:47Z

@Yanual please report this case as a separate issue, according to our issue template (with step-by-step reproduction instruction).

msamsel force-pushed the t/1027 branch 2 times, most recently from 4aab72e to 17c72f2 Compare October 30, 2017 13:13

f1ames self-requested a review November 3, 2017 11:33

f1ames suggested changes Nov 3, 2017

View reviewed changes

This was referenced Nov 6, 2017

Dragging widgets in tables behave wrongly #1124

Closed

Cursor drag over widgets does not activate tableselection #1125

Open

msamsel force-pushed the t/1027 branch 2 times, most recently from 00a2cf2 to fdb4ffe Compare November 6, 2017 14:57

msamsel requested a review from f1ames November 6, 2017 15:04

f1ames suggested changes Nov 8, 2017

View reviewed changes

mlewand requested a review from Comandeer November 8, 2017 12:15

msamsel force-pushed the t/1027 branch from b09752c to 67ac4b8 Compare November 8, 2017 15:47

mlewand requested a review from f1ames November 21, 2017 08:38

f1ames approved these changes Nov 21, 2017

View reviewed changes

Comandeer requested changes Nov 22, 2017

View reviewed changes

msamsel force-pushed the t/1027 branch from 67ac4b8 to cb49462 Compare November 22, 2017 15:25

msamsel requested a review from Comandeer November 22, 2017 15:32

Comandeer requested changes Nov 23, 2017

View reviewed changes

msamsel force-pushed the t/1027 branch from b66c8ce to 1861750 Compare November 24, 2017 15:58

msamsel requested a review from Comandeer November 24, 2017 16:01

Comandeer approved these changes Nov 26, 2017

View reviewed changes

Mateusz Samsel added 4 commits November 27, 2017 13:44

Adding manual test for widget without tableselection plugin.

34fec1e

Selection fix for widgets inside tables.

35058ef

Unit test for 'isDomWidget'.

9048e89

Review fixes: mostly comment update and little test description impro…

727922e

…vements. Also extracting widget definition in test.

Mateusz Samsel and others added 5 commits November 27, 2017 13:45

Review fixes: new test case for table+image2.

488cb27

Rewording.

43aa42e

Review fixes. Adding ignore condition to tests. Little test improveme…

cb994ce

…nts.

Review fixes: extract widget checking to separate function, some typo…

8c34460

… fixes, reorganise tests path.

Fix function names.

42c6479

f1ames force-pushed the t/1027 branch from 1861750 to 42c6479 Compare November 27, 2017 12:46

Changelog entry. [skip ci]

7d792da

f1ames merged commit be26f77 into major Nov 27, 2017

f1ames deleted the t/1027 branch November 27, 2017 12:57


		_Perform above steps for all widgets and both editors._

		Expected: Widget is deselect when focus is changed to text in table. You are able to perfom operatons on selected text.

		@@ -0,0 +1,27 @@
		<textarea id="editor">
		<p>Lena's path: "../../_assets/lena.jpg"</p>

		@@ -0,0 +1,34 @@
		<label>Lena's path: <input style="border:2px solid black" type="text" value="../../_assets/lena.jpg" readonly></label>

Widget has now correct selection behaviour inside tables #1104

Widget has now correct selection behaviour inside tables #1104

Conversation

msamsel commented Oct 30, 2017

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

f1ames 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames commented Nov 3, 2017 • edited Loading

msamsel commented Nov 6, 2017

f1ames 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

mlewand commented Nov 8, 2017

f1ames left a comment

Choose a reason for hiding this comment

f1ames commented Nov 21, 2017

Comandeer 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

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Nov 24, 2017

Comandeer left a comment

Choose a reason for hiding this comment

Yanual commented Dec 28, 2017 • edited Loading

mlewand commented Dec 28, 2017

f1ames commented Nov 3, 2017 •

edited

Loading

Yanual commented Dec 28, 2017 •

edited

Loading