Added htmlParser.element.findOne #2719

engineering-this · 2019-01-03T14:53:35Z

What is the purpose of this pull request?

New feature

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?

Added CKEDITOR.htmlParser.element.findOne
Refactored Paste From Word filter function removeSymbolText.

Closes #2698.

Comandeer

findOne method should have the same signature as find one, therefore recursive search should be activated via recursive parameter.

CHANGES.md

Comandeer · 2019-01-04T16:26:36Z

core/htmlparser/element.js

+		 *
+		 * @param {String/Function} criteria Tag name or evaluator function.
+		 * @param {CKEDITOR.htmlParser.node} criteria.child The currently iterated child.
+		 * @returns {CKEDITOR.htmlParser.node} First matched child, `undefined` otherwise.


If undefined is also allowed, then returned type is CKEDITOR.htmlParser.node/undefined.

Additionally IMO it should return null, as every API searching for elements returns it in case of no match.

Comandeer · 2019-01-04T16:41:41Z

core/htmlparser/element.js

+		 */
+		findOne: function( criteria ) {
+			var findMethod = typeof criteria === 'function' ? criteria : findByName,
+				nestedMatch,


Variables without value should be declared at the end.

Comandeer · 2019-01-04T16:47:14Z

core/htmlparser/element.js

+				match = CKEDITOR.tools.array.find( this.children, function( child ) {
+					if ( findMethod( child ) ) {
+						return true;
+					} else if ( child.children && child.findOne ) {


This else if statement is not necessary, as execution will end on return true; statement, if the first condition is true.

Comandeer · 2019-01-04T17:00:30Z

tests/core/htmlparser/element.js

@@ -16,6 +16,14 @@ function write( el ) {
 	return writer.getHtml();
 }

+var testHtml = '<body>' +
+		'<p>Foo' +
+			'<div>Baz' +


This HTML is incorrect: div can't be nested in p tag.

engineering-this · 2019-01-10T10:55:53Z

Changed returned type to null when no match is found.
Extracted findOne tests to separate file, so they are much better readable with html string right above TCs. Also reformatted fixture, so each node is in new line, except for elements that contain only one text.
Corrected changelog, and other small things.

Comandeer

Overall looks good, only some minor issues left.

Additionally I'm thinking about adding unit test with deeper, 2/3 level recursion – just to be sure that it really goes deeper than the first child.

Comandeer · 2019-01-14T15:58:26Z

core/htmlparser/element.js

+
+			return nestedMatch || match || null;
+
+			function findByName( item ) {


This method can be inlined into ret declaration.

We can also rename ret to something more meaningful, like match or something like that.

Comandeer · 2019-01-14T16:01:53Z

plugins/pastefromword/filter/default.js

-					}
+				node = element.findOne( function( node ) {
+					return node.value && node.value.indexOf( symbol ) > -1;
+				}, true ),


Code style issue – there should be one more indentation level.

Adding indentation level would make closing bracket higher indentation than opening, shouldn't they be the same?

It's a very similar case to objects in such lists and they are indented with that extra tab: https://github.com/ckeditor/ckeditor-dev/blob/1f365357c043a74e69ad21f32c829929d8610106/plugins/tableselection/plugin.js#L893-L901

plugins/pastefromword/filter/default.js

Comandeer · 2019-01-14T16:04:10Z

tests/core/htmlparser/elementfindone.js

+( function() {
+	'use strict';
+
+	var testElement = CKEDITOR.htmlParser.fragment.fromHtml(


I wonder if it wouldn't be more readable to move the HTML into separate file, elementfindone.html.

tests/core/htmlparser/elementfindone.js

Comandeer

Looks good, just some couple of style issues.

Comandeer · 2019-01-21T16:02:10Z

plugins/pastefromword/filter/default.js

-					}
+				node = element.findOne( function( node ) {
+					return node.value && node.value.indexOf( symbol ) > -1;
+				}, true ),


It's a very similar case to objects in such lists and they are indented with that extra tab: https://github.com/ckeditor/ckeditor-dev/blob/1f365357c043a74e69ad21f32c829929d8610106/plugins/tableselection/plugin.js#L893-L901

Comandeer · 2019-01-21T16:03:26Z

plugins/pastefromword/filter/default.js


-			element.forEach( function( node ) {
+			if ( node ) {
 				// Since symbol may contains special characters we use `indexOf` (instead of RegExp) which is sufficient (#877).


indexOf was moved up, so the comment also should.

Comandeer · 2019-01-21T16:10:44Z

core/htmlparser/element.js

+		 * @param {String/Function} criteria Tag name or evaluator function.
+		 * @param {CKEDITOR.htmlParser.node} criteria.child The currently iterated child.
+		 * @param {Boolean} [recursive=false] If set to `true` will iterate all descendants, otherwise only direct children.
+		 * @returns {CKEDITOR.htmlParser.node} First matched child, `null` otherwise.


It should be marked as nullable type.

Comandeer · 2019-01-21T16:12:20Z

core/htmlparser/element.js

+		 * @returns {CKEDITOR.htmlParser.node} First matched child, `null` otherwise.
+		 */
+		findOne: function( criteria, recursive ) {
+			var findMethod = typeof criteria === 'function' ? criteria : function( item ) {


By inlining I meant "inline it directly into CKEDITOR.tools.array.find callback":

var isMatching = typeof criteria === 'function' ? criteria( child ) : item.name === criteria;

IMO such version is still readable and dedicated function findMethod is not necessary in this case.

Comandeer

Actually I wanted to requested changes… Sorry for misclick.

Comandeer

LGTM, however http://tests.ckeditor.test:1030/tests/core/htmlparser/elementfindone#tests%2Fcore%2Fhtmlparser%2Felementfindone%20test%20findOne%20by%20function fails. Additionally failure message is not visible.

engineering-this · 2019-02-04T16:19:01Z

Fixed failing test. Fail message was wrongly placed as findOne param instead of assert, as a result findOne treated message as recursive param, and assert didn't have any message to display.

Comandeer

http://tests.ckeditor.test:1030/tests/core/htmlparser/elementfindone#tests%2Fcore%2Fhtmlparser%2Felementfindone%20test%20findOne%20deep%20nested%20child fails in IE8. Everything else LGTM. I've also rebased branch onto latest major and corrected API docs. I've removed criteria.child from them as it was rendered as property of criteria parameter, not a parameter passed to it.

engineering-this · 2019-02-12T10:51:02Z

IE cuts some of text nodes. To make test constant regardless of environment I've moved tested html code back into js file and removed html file.

…ild.

Co-Authored-By: engineering-this <[email protected]>

…n no match is found.

…expected element.

Comandeer

LGTM!

Comandeer self-requested a review January 4, 2019 09:26

Comandeer self-assigned this Jan 4, 2019

CKEditorBot unassigned Comandeer Jan 4, 2019

Comandeer requested changes Jan 4, 2019

View reviewed changes

engineering-this requested a review from Comandeer January 10, 2019 11:13

Comandeer self-assigned this Jan 14, 2019

Comandeer requested changes Jan 14, 2019

View reviewed changes

CKEditorBot unassigned Comandeer Jan 14, 2019

engineering-this self-assigned this Jan 18, 2019

engineering-this requested a review from Comandeer January 21, 2019 11:32

CKEditorBot unassigned engineering-this Jan 21, 2019

Comandeer self-assigned this Jan 21, 2019

Comandeer approved these changes Jan 21, 2019

View reviewed changes

CKEditorBot unassigned Comandeer Jan 21, 2019

Comandeer requested changes Jan 21, 2019

View reviewed changes

engineering-this requested a review from Comandeer January 22, 2019 15:25

Comandeer self-assigned this Jan 29, 2019

Comandeer requested changes Jan 29, 2019

View reviewed changes

CKEditorBot unassigned Comandeer Jan 29, 2019

engineering-this requested a review from Comandeer February 4, 2019 16:19

Comandeer self-assigned this Feb 8, 2019

Comandeer force-pushed the t/2698 branch from 400bce8 to 43909ed Compare February 8, 2019 15:02

Comandeer requested changes Feb 8, 2019

View reviewed changes

CKEditorBot unassigned Comandeer Feb 8, 2019

engineering-this self-assigned this Feb 11, 2019

engineering-this requested a review from Comandeer February 12, 2019 10:52

CKEditorBot unassigned engineering-this Feb 12, 2019

Comandeer assigned Comandeer and unassigned Comandeer Feb 14, 2019

engineering-this and others added 24 commits February 14, 2019 15:53

Tests: added test cases for findOne.

d6214bf

Added htmlParser.element.findOne method.

0341b0e

Docs: Added entry on htmlParser.element.findOne, renamed item into ch…

294e391

…ild.

Refactored removeSymbolText to use htmlParser.element.findOne.

bb728b5

Changelog: added entry.

0cd0e43

Tests: extracted duplicate code to variable.

3a19815

Changelog: Fixed method name.

406cc5f

Co-Authored-By: engineering-this <[email protected]>

htmlParser.element.findOne added param , changed returned type to whe…

3a56254

…n no match is found.

Tests: extracted htmlParser.element.findOne to separate test file.

774fba5

Added param to recursive search symbol in function.

48e8549

Tests: removed empty line.

2e115b6

Removed findByName helper by inlining it.

4072303

Renamed variable ret to isMatching.

9d6e85a

Added removed comments.

09de7eb

Moved fixture to separate file. Added assertion messages pointing on …

9fdc9d4

…expected element.

Increased indent level.

0f0dd90

Commend moved to proper place.

d3d620e

Added null as possible returned type.

1b7d17d

Removed findMethod variable by inlining it into isMatching.

a94d5b3

Tets: fixed failing test due to wrongly places param.

e1190ae

Fix incorrect API docs.

7a07495

Slightly rephrase changelog entry.

d740dcd

Test: removed space.

d67a76a

Tests: removed moved tested html into js file and removed html file.

eb12c41

Comandeer force-pushed the t/2698 branch from ee83347 to eb12c41 Compare February 14, 2019 15:23

Comandeer approved these changes Feb 14, 2019

View reviewed changes

Comandeer merged commit 9bfa5c2 into major Feb 14, 2019

CKEditorBot deleted the t/2698 branch February 14, 2019 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added htmlParser.element.findOne #2719

Added htmlParser.element.findOne #2719

engineering-this commented Jan 3, 2019 •

edited by Comandeer

Loading

Comandeer left a comment

Comandeer Jan 4, 2019

Comandeer Jan 4, 2019

Comandeer Jan 4, 2019

Comandeer Jan 4, 2019

engineering-this commented Jan 10, 2019

Comandeer left a comment

Comandeer Jan 14, 2019

Comandeer Jan 14, 2019

Comandeer Jan 14, 2019

engineering-this Jan 21, 2019

Comandeer Jan 21, 2019

Comandeer Jan 14, 2019

Comandeer left a comment

Comandeer Jan 21, 2019

Comandeer Jan 21, 2019

Comandeer Jan 21, 2019

Comandeer Jan 21, 2019

Comandeer left a comment

Comandeer left a comment

engineering-this commented Feb 4, 2019

Comandeer left a comment

engineering-this commented Feb 12, 2019

Comandeer left a comment


		return nestedMatch \|\| match \|\| null;

		function findByName( item ) {

Added htmlParser.element.findOne #2719

Added htmlParser.element.findOne #2719

Conversation

engineering-this commented Jan 3, 2019 • edited by Comandeer Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

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

engineering-this commented Jan 10, 2019

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

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

Comandeer left a comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

engineering-this commented Feb 4, 2019

Comandeer left a comment

Choose a reason for hiding this comment

engineering-this commented Feb 12, 2019

Comandeer left a comment

Choose a reason for hiding this comment

engineering-this commented Jan 3, 2019 •

edited by Comandeer

Loading