Changing list type wont throw error for empty list elements #2440

msamsel · 2018-09-26T14:07:14Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

Fix throwing errors in console, when markup with empty ul list was changed to different list type.

Close #2411

Related also to #2438

mlewand

Checks in your patch are bit too loose, you can still get the error which is a subject of this issue by following a manual test but putting following markup as input:

<ul>
	<li>
		<div>test<ul>
				<!-- I'm a sneaky little comment -->
			</ul>
		</div>
	</li>
	<li>test</li>
</ul>

Good job with covering existing functionality (that has not been covered with unit tests yet).

mlewand · 2018-09-28T06:37:04Z

tests/plugins/list/manual/listconversionerrors.html

@@ -0,0 +1,30 @@
+<h2>Markup 1:</h2>
+<input type="text" value="&lt;ul&gt;&lt;li&gt;&lt;div&gt;test&lt;ul&gt;&lt;/ul&gt;&lt;/div&gt;&lt;/li&gt;&lt;li&gt;test&lt;/li&gt;&lt;/ul&gt;">


it would be helpful to mark these inputs as readonly.

mlewand · 2018-09-28T06:42:01Z

tests/plugins/list/manual/listconversionerrors.md

+
+## Expected:
+There is no error in console.
+However content might look strange and this is not a bug, situation is reportd as list plugin improvement.


reportd

-> reported

Actually it would be nice to link mentioned issue.

might look strange and this is not a bug

This actually is a buggy content, but it is to be fixed in mentioned issue 😕

mlewand · 2018-09-28T06:54:13Z

tests/plugins/list/blockconversion.html

+	</ul>
+
+	<ul>
+		<li><a href="https://powerusers.microsoft.com/plugins/common/feature/oauth2sso/sso_login_redirect?referer=https%3A%2F%2Fpowerusers.microsoft.com%2Ft5%2FPowerApps-Community%2Fct-p%2FPowerApps1">Register</a></li>


Please, use dummy links.

mlewand · 2018-09-28T07:05:40Z

plugins/list/plugin.js

+			function isEmptyList( groupObj ) {
+				// If list is without any li item, then ignore such element from transformation, becasue it throws errors in console.
+				// Hack for situation described in #2411, #2438.
+				return groupObj.root.is( 'ul', 'ol' ) && groupObj.root.getChildCount() === 0;


Reuse listNodeNames to identify list tag names.

Actually in my opinion we should use DTD here (CKEDITOR.dtd.$list) but for some reason it was skipped. Maybe because it should handle only lists, and not description list.

mlewand · 2018-09-28T08:39:19Z

tests/plugins/list/blockconversion.js

+
+var tests = {
+	'test conversion of embedded unordered list and paragraphs to numbered list': function( editor, bot ) { // Strange test result for: 0, 8
+		var testesLength = 9;


Subsequent var declarations/initialization should be contained within a single var statement.

This issue appears later on, please correct each occurence.

mlewand · 2018-09-28T08:46:11Z

tests/plugins/list/blockconversion.html

@@ -0,0 +1,468 @@
+<!--
+
+Strange output markup for tests:


Avoid information duplication. You have already provided this info in blockconversion.js.

blockconversion.js is where the logic of tests are so it is the best place to put this information. Here it's not needed, as it is only a resource file for the logic.

mlewand · 2018-09-28T08:47:05Z

tests/plugins/list/blockconversion.js

+};
+
+var tests = {
+	'test conversion of embedded unordered list and paragraphs to numbered list': function( editor, bot ) { // Strange test result for: 0, 8


Strange test result for: 0, 8 comment will look much better in a line below. Here it exceeds 120 chars which, if convenient, we like to avoid.

…ent.

mlewand · 2019-01-03T10:29:49Z

I have rebased the branch (resolved conflict in CHANGES.md). Also I've adjusted manual test description.

mlewand

childCountWithoutComments method is a blocker for this fix

mlewand · 2019-01-03T10:50:15Z

core/tools.js

+		 * @param {CKEDITOR.dom.element} node Element whos children will be counted.
+		 * @returns {Number} Amount of node's children without counting comments.
+		 */
+		childCountWithoutComments: function( node ) {


We can't add a method like that, it doesn't belong here.

Logically it belongs to CKEDITOR.dom.element type - this is where it should be added.

It's too specific to your particular use case. Having a method like that would be excuse for getChildCountWithoutNodes, getChildCountWithoutElements etc. Instead getChildCount method should be modified to accept options param, that allows to specify options.exclude or options.include. So you'd call it like elem.getChildCount( { exclude: [ CKEDITOR.NODE_COMMENT ] } );.

Name is not aligned with existing getChildCount method.

Finally there's a minor naming issue, node can not have children so the arg should be called element just like you described in @param's description.

I should have pointed this out in previous review.

… an element.", "Update tag in docs description." This reverts commits b199c71 and fc908f1.

mlewand · 2019-01-03T13:31:09Z

I have fixed the issue that I raised during the review by simply inlining the function. Ideally I'd like CKEDITOR.dom.element methods to get include / exclude options, but that would mean that this fix would need to wait for the major release.

mlewand self-requested a review September 28, 2018 06:33

mlewand self-assigned this Sep 28, 2018

CKEditorBot unassigned mlewand Sep 28, 2018

mlewand suggested changes Sep 28, 2018

View reviewed changes

msamsel self-assigned this Oct 1, 2018

msamsel force-pushed the t/2411 branch 2 times, most recently from 84284d2 to 59d2ff2 Compare December 11, 2018 08:53

msamsel removed their assignment Dec 11, 2018

msamsel requested a review from mlewand December 11, 2018 08:58

mlewand self-assigned this Jan 3, 2019

Mateusz Samsel and others added 16 commits January 3, 2019 11:17

Unit test covered current situation of supporting list conversions.

eae2aeb

Add unit test for wrong handling empty lsits.

07126b7

Improve unit test to work on differnet browsers.

20a5bcc

Add manual test for bug with embedded lists without items.

026210a

Fix for not throwing error during handling empty lists.

34e5e56

Rename variable to be more consisten with other names.

85298f8

Add changelog entry.

2d72da8

Improve comment description to be more accurate.

109b05c

Fix descriptions and syntax in tests. Improve list detection in bug fix.

e1f50ce

Add new tools to count amount of children omtting comments of an elem…

b199c71

…ent.

Change way how empty ul and ol elemnets are detected.

cfe8ff9

Prevent of using comments as list items during list processing.

7c71ec5

Fix test markup.

07269f3

Fix version tag in test.

b283ead

Update tag in docs description.

fc908f1

Tests: adjusted list conversion manual test description.

0f7c46b

mlewand force-pushed the t/2411 branch from 0819e93 to 0f7c46b Compare January 3, 2019 10:29

Fixed typos adjusted issue reference.

e79eb88

mlewand suggested changes Jan 3, 2019

View reviewed changes

CKEditorBot unassigned mlewand Jan 3, 2019

mlewand added 2 commits January 3, 2019 13:48

Inlined childCountWithoutComments method proposal.

46c0491

Revert "Add new tools to count amount of children omtting comments of…

2e00d1a

… an element.", "Update tag in docs description." This reverts commits b199c71 and fc908f1.

mlewand approved these changes Jan 3, 2019

View reviewed changes

mlewand merged commit 56c10f2 into master Jan 3, 2019

CKEditorBot deleted the t/2411 branch January 3, 2019 13:42

mlewand mentioned this pull request Jan 3, 2019

Pasting empty nested list and converting it to different list type throws error #2411

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing list type wont throw error for empty list elements #2440

Changing list type wont throw error for empty list elements #2440

msamsel commented Sep 26, 2018

mlewand left a comment

mlewand Sep 28, 2018

mlewand Sep 28, 2018

mlewand Sep 28, 2018

mlewand Sep 28, 2018

mlewand Sep 28, 2018

mlewand Sep 28, 2018

mlewand Sep 28, 2018

mlewand Sep 28, 2018

mlewand Sep 28, 2018

mlewand commented Jan 3, 2019

mlewand left a comment

mlewand Jan 3, 2019

mlewand commented Jan 3, 2019

		@@ -0,0 +1,30 @@
		<h2>Markup 1:</h2>
		<input type="text" value="<ul><li><div>test<ul></ul></div></li><li>test</li></ul>">

Changing list type wont throw error for empty list elements #2440

Changing list type wont throw error for empty list elements #2440

Conversation

msamsel commented Sep 26, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

mlewand 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

mlewand commented Jan 3, 2019

mlewand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand commented Jan 3, 2019