Consider comments when comparing nodes. #7403
Conversation
| } ); | ||
|
|
||
| // https://github.com/ckeditor/ckeditor5/issues/5734 | ||
| it( 'should replace a comment with the added element', () => { |
There was a problem hiding this comment.
TBH I don't understand this description even though I read the test and I know what the issue is about 😛
There was a problem hiding this comment.
Do you have any alternative in mind?
Is "should remove the comment and add a child element" any better?
There was a problem hiding this comment.
Nothing particular in my mind. The alternative sounds better, though.
| @@ -930,6 +930,7 @@ function addInlineFiller( domDocument, domParentOrArray, offset ) { | |||
| function areSimilar( node1, node2 ) { | |||
| return isNode( node1 ) && isNode( node2 ) && | |||
| !isText( node1 ) && !isText( node2 ) && | |||
| node1.nodeType !== 8 && node2.nodeType !== 8 && | |||
There was a problem hiding this comment.
Node.COMMENT_NODE via https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType.
There was a problem hiding this comment.
Also: DomCoverter already implements isComment(). I think it should be extracted to ckeditor5-utils/src/dom (like isNode(), isText() and many others), tested, and then used both in DomConverter and Renderer.
There was a problem hiding this comment.
Do we really need to add a dependency and bunch of tests for quite straightforward, native, and atomic comparison: node.nodeType !== Node.COMMENT_NODE? To me, it seems like overhead.
There was a problem hiding this comment.
I figured since there are 2 places that would benefit from the new thing, then why not? We have is*() helpers for most of the native DOM object types anyway. There are even tests for it already.
We could use Object.prototype.toString.apply( obj ) == '[object Comment]'; to make sure it is document-agnostic (like in isText() or isRange()).
There was a problem hiding this comment.
Or... to make it faster (I assume toString is sluggish), we could call isNode() first and then check the #nodeType.
There was a problem hiding this comment.
...or not :D It looks like isNode() is buggy. A comment does not have a #defaultView, so isNode() will always return false even though Comment inherits from Node
There was a problem hiding this comment.
Maybe let's have a follow-up for this whole story then 😛
Suggested merge commit message (convention)
Fix (engine): Editor no longer crashes when initialized on HTML with comment. Closes #5734.
Additional information
I was not sure about the location of the tests.