Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

t/201: Bulletproofed isDomNode helper when used in iframes. Removed isWindow logic. #208

Merged
merged 2 commits into from
Dec 11, 2017

Conversation

oleq
Copy link
Member

@oleq oleq commented Nov 24, 2017

Suggested merge commit message (convention)

Fix: Bulletproofed isDomNode helper when used in iframes. Removed isWindow logic from the helper. Closes ckeditor/ckeditor5#4985.

return !!( obj && isNative( obj.addEventListener ) );
if ( obj ) {
if ( obj.defaultView ) {
return obj instanceof obj.defaultView.Document;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is document a node?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I actually wanted to know whether this code is needed at all and whether this code is semantically correct. But now I see that both things are true.

@Reinmar
Copy link
Member

Reinmar commented Dec 11, 2017

The existing isWindow() made me think about:

image

That may be some direction to consider if the one proposed by you in this PR will turn to be incorrect one day. Something like this should do the job:

isDomNode( obj ) {
    return Object.prototype.toString.apply( obj ).startsWith( '[object HTML' );
}

@Reinmar Reinmar merged commit 84ccda2 into master Dec 11, 2017
@Reinmar Reinmar deleted the t/201 branch December 11, 2017 16:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isDomNode() is confusing
2 participants