Skip to content

Commit

Permalink
DomConverter should use a separate DOM document instance in the data …
Browse files Browse the repository at this point in the history
…pipeline.
  • Loading branch information
niegowski committed Jul 25, 2022
1 parent b2662a1 commit a0244b0
Show file tree
Hide file tree
Showing 20 changed files with 213 additions and 138 deletions.
2 changes: 1 addition & 1 deletion packages/ckeditor5-clipboard/tests/clipboardobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe( 'ClipboardObserver', () => {
// Create view and DOM structures.
el = writer.createContainerElement( 'p' );
writer.insert( writer.createPositionAt( root, 0 ), el );
view.domConverter.viewToDom( root, document, { withChildren: true, bind: true } );
view.domConverter.viewToDom( root, { withChildren: true, bind: true } );

doc.selection._setTo( el, 0 );
range = writer.createRange( writer.createPositionAt( root, 1 ) );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @module engine/dataprocessor/htmldataprocessor
*/

/* globals document, DOMParser */
/* globals DOMParser */

import BasicHtmlWriter from './basichtmlwriter';
import DomConverter from '../view/domconverter';
Expand Down Expand Up @@ -56,7 +56,7 @@ export default class HtmlDataProcessor {
*/
toData( viewFragment ) {
// Convert view DocumentFragment to DOM DocumentFragment.
const domFragment = this.domConverter.viewToDom( viewFragment, document );
const domFragment = this.domConverter.viewToDom( viewFragment );

// Convert DOM DocumentFragment to HTML output.
return this.htmlWriter.getHtml( domFragment );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @module engine/dataprocessor/xmldataprocessor
*/

/* globals DOMParser, document */
/* globals DOMParser */

import BasicHtmlWriter from './basichtmlwriter';
import DomConverter from '../view/domconverter';
Expand Down Expand Up @@ -71,7 +71,7 @@ export default class XmlDataProcessor {
*/
toData( viewFragment ) {
// Convert view DocumentFragment to DOM DocumentFragment.
const domFragment = this.domConverter.viewToDom( viewFragment, document );
const domFragment = this.domConverter.viewToDom( viewFragment );

// Convert DOM DocumentFragment to XML output.
// There is no need to use dedicated for XML serializing method because BasicHtmlWriter works well in this case.
Expand Down
59 changes: 32 additions & 27 deletions packages/ckeditor5-engine/src/view/domconverter.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @module engine/view/domconverter
*/

/* globals document, Node, NodeFilter, DOMParser, Text */
/* globals Node, NodeFilter, DOMParser, Text */

import ViewText from './text';
import ViewElement from './element';
Expand All @@ -30,9 +30,9 @@ import getAncestors from '@ckeditor/ckeditor5-utils/src/dom/getancestors';
import isText from '@ckeditor/ckeditor5-utils/src/dom/istext';
import isComment from '@ckeditor/ckeditor5-utils/src/dom/iscomment';

const BR_FILLER_REF = BR_FILLER( document ); // eslint-disable-line new-cap
const NBSP_FILLER_REF = NBSP_FILLER( document ); // eslint-disable-line new-cap
const MARKED_NBSP_FILLER_REF = MARKED_NBSP_FILLER( document ); // eslint-disable-line new-cap
const BR_FILLER_REF = BR_FILLER( global.document ); // eslint-disable-line new-cap
const NBSP_FILLER_REF = NBSP_FILLER( global.document ); // eslint-disable-line new-cap
const MARKED_NBSP_FILLER_REF = MARKED_NBSP_FILLER( global.document ); // eslint-disable-line new-cap
const UNSAFE_ATTRIBUTE_NAME_PREFIX = 'data-ck-unsafe-attribute-';
const UNSAFE_ELEMENT_REPLACEMENT_ATTRIBUTE = 'data-ck-unsafe-element';

Expand Down Expand Up @@ -135,6 +135,14 @@ export default class DomConverter {
*/
this.unsafeElements = [ 'script', 'style' ];

/**
* The DOM Document used to create DOM nodes.
*
* @type {Document}
* @private
*/
this._domDocument = this.renderingMode === 'editing' ? global.document : global.document.implementation.createHTMLDocument( '' );

/**
* The DOM-to-view mapping.
*
Expand Down Expand Up @@ -352,17 +360,16 @@ export default class DomConverter {
*
* @param {module:engine/view/node~Node|module:engine/view/documentfragment~DocumentFragment} viewNode
* View node or document fragment to transform.
* @param {Document} domDocument Document which will be used to create DOM nodes.
* @param {Object} [options] Conversion options.
* @param {Boolean} [options.bind=false] Determines whether new elements will be bound.
* @param {Boolean} [options.withChildren=true] If `true`, node's and document fragment's children will be converted too.
* @returns {Node|DocumentFragment} Converted node or DocumentFragment.
*/
viewToDom( viewNode, domDocument, options = {} ) {
viewToDom( viewNode, options = {} ) {
if ( viewNode.is( '$text' ) ) {
const textData = this._processDataFromViewText( viewNode );

return domDocument.createTextNode( textData );
return this._domDocument.createTextNode( textData );
} else {
if ( this.mapViewToDom( viewNode ) ) {
return this.mapViewToDom( viewNode );
Expand All @@ -372,17 +379,17 @@ export default class DomConverter {

if ( viewNode.is( 'documentFragment' ) ) {
// Create DOM document fragment.
domElement = domDocument.createDocumentFragment();
domElement = this._domDocument.createDocumentFragment();

if ( options.bind ) {
this.bindDocumentFragments( domElement, viewNode );
}
} else if ( viewNode.is( 'uiElement' ) ) {
if ( viewNode.name === '$comment' ) {
domElement = domDocument.createComment( viewNode.getCustomProperty( '$rawContent' ) );
domElement = this._domDocument.createComment( viewNode.getCustomProperty( '$rawContent' ) );
} else {
// UIElement has its own render() method (see #799).
domElement = viewNode.render( domDocument, this );
domElement = viewNode.render( this._domDocument, this );
}

if ( options.bind ) {
Expand All @@ -397,9 +404,9 @@ export default class DomConverter {

domElement = this._createReplacementDomElement( viewNode.name );
} else if ( viewNode.hasAttribute( 'xmlns' ) ) {
domElement = domDocument.createElementNS( viewNode.getAttribute( 'xmlns' ), viewNode.name );
domElement = this._domDocument.createElementNS( viewNode.getAttribute( 'xmlns' ), viewNode.name );
} else {
domElement = domDocument.createElement( viewNode.name );
domElement = this._domDocument.createElement( viewNode.name );
}

// RawElement take care of their children in RawElement#render() method which can be customized
Expand All @@ -419,7 +426,7 @@ export default class DomConverter {
}

if ( options.withChildren !== false ) {
for ( const child of this.viewChildrenToDom( viewNode, domDocument, options ) ) {
for ( const child of this.viewChildrenToDom( viewNode, options ) ) {
domElement.appendChild( child );
}
}
Expand Down Expand Up @@ -488,23 +495,22 @@ export default class DomConverter {
* Additionally, this method adds block {@link module:engine/view/filler filler} to the list of children, if needed.
*
* @param {module:engine/view/element~Element|module:engine/view/documentfragment~DocumentFragment} viewElement Parent view element.
* @param {Document} domDocument Document which will be used to create DOM nodes.
* @param {Object} options See {@link module:engine/view/domconverter~DomConverter#viewToDom} options parameter.
* @returns {Iterable.<Node>} DOM nodes.
*/
* viewChildrenToDom( viewElement, domDocument, options = {} ) {
* viewChildrenToDom( viewElement, options = {} ) {
const fillerPositionOffset = viewElement.getFillerOffset && viewElement.getFillerOffset();
let offset = 0;

for ( const childView of viewElement.getChildren() ) {
if ( fillerPositionOffset === offset ) {
yield this._getBlockFiller( domDocument );
yield this._getBlockFiller();
}

const transparentRendering = childView.is( 'element' ) && childView.getCustomProperty( 'dataPipeline:transparentRendering' );

if ( transparentRendering && this.renderingMode == 'data' ) {
yield* this.viewChildrenToDom( childView, domDocument, options );
yield* this.viewChildrenToDom( childView, options );
} else {
if ( transparentRendering ) {
/**
Expand All @@ -515,14 +521,14 @@ export default class DomConverter {
logWarning( 'domconverter-transparent-rendering-unsupported-in-editing-pipeline', { viewElement: childView } );
}

yield this.viewToDom( childView, domDocument, options );
yield this.viewToDom( childView, options );
}

offset++;
}

if ( fillerPositionOffset === offset ) {
yield this._getBlockFiller( domDocument );
yield this._getBlockFiller();
}
}

Expand All @@ -537,7 +543,7 @@ export default class DomConverter {
const domStart = this.viewPositionToDom( viewRange.start );
const domEnd = this.viewPositionToDom( viewRange.end );

const domRange = document.createRange();
const domRange = this._domDocument.createRange();
domRange.setStart( domStart.parent, domStart.offset );
domRange.setEnd( domEnd.parent, domEnd.offset );

Expand Down Expand Up @@ -1094,7 +1100,7 @@ export default class DomConverter {

// Since it takes multiple lines of code to check whether a "DOM Position" is before/after another "DOM Position",
// we will use the fact that range will collapse if it's end is before it's start.
const range = document.createRange();
const range = this._domDocument.createRange();

range.setStart( selection.anchorNode, selection.anchorOffset );
range.setEnd( selection.focusNode, selection.focusOffset );
Expand Down Expand Up @@ -1169,17 +1175,16 @@ export default class DomConverter {
* Returns the block {@link module:engine/view/filler filler} node based on the current {@link #blockFillerMode} setting.
*
* @private
* @params {Document} domDocument
* @returns {Node} filler
*/
_getBlockFiller( domDocument ) {
_getBlockFiller() {
switch ( this.blockFillerMode ) {
case 'nbsp':
return NBSP_FILLER( domDocument ); // eslint-disable-line new-cap
return NBSP_FILLER( this._domDocument ); // eslint-disable-line new-cap
case 'markedNbsp':
return MARKED_NBSP_FILLER( domDocument ); // eslint-disable-line new-cap
return MARKED_NBSP_FILLER( this._domDocument ); // eslint-disable-line new-cap
case 'br':
return BR_FILLER( domDocument ); // eslint-disable-line new-cap
return BR_FILLER( this._domDocument ); // eslint-disable-line new-cap
}
}

Expand Down Expand Up @@ -1580,7 +1585,7 @@ export default class DomConverter {
* @returns {Element}
*/
_createReplacementDomElement( elementName, originalDomElement = null ) {
const newDomElement = document.createElement( 'span' );
const newDomElement = this._domDocument.createElement( 'span' );

// Mark the span replacing a script as hidden.
newDomElement.setAttribute( UNSAFE_ELEMENT_REPLACEMENT_ATTRIBUTE, elementName );
Expand Down
6 changes: 3 additions & 3 deletions packages/ckeditor5-engine/src/view/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ export default class Renderer {
this.domConverter.mapViewToDom( viewElement ).childNodes
);
const expectedDomChildren = Array.from(
this.domConverter.viewChildrenToDom( viewElement, domElement.ownerDocument, { withChildren: false } )
this.domConverter.viewChildrenToDom( viewElement, { withChildren: false } )
);
const diff = this._diffNodeLists( actualDomChildren, expectedDomChildren );
const actions = this._findReplaceActions( diff, actualDomChildren, expectedDomChildren );
Expand Down Expand Up @@ -518,7 +518,7 @@ export default class Renderer {
*/
_updateText( viewText, options ) {
const domText = this.domConverter.findCorrespondingDomText( viewText );
const newDomText = this.domConverter.viewToDom( viewText, domText.ownerDocument );
const newDomText = this.domConverter.viewToDom( viewText );

const actualText = domText.data;
let expectedText = newDomText.data;
Expand Down Expand Up @@ -597,7 +597,7 @@ export default class Renderer {
const inlineFillerPosition = options.inlineFillerPosition;
const actualDomChildren = this.domConverter.mapViewToDom( viewElement ).childNodes;
const expectedDomChildren = Array.from(
this.domConverter.viewChildrenToDom( viewElement, domElement.ownerDocument, { bind: true } )
this.domConverter.viewChildrenToDom( viewElement, { bind: true } )
);

// Inline filler element has to be created as it is present in the DOM, but not in the view. It is required
Expand Down
14 changes: 14 additions & 0 deletions packages/ckeditor5-engine/tests/dataprocessor/basichtmlwriter.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ describe( 'BasicHtmlWriter', () => {

const data = basicHtmlWriter.getHtml( fragment );
expect( data ).to.equal( text );

// Verify if node was not adopted to main document.
expect( textNode.ownerDocument ).not.equal( document );
} );

it( 'should return correct HTML from fragment with paragraph', () => {
Expand All @@ -33,6 +36,10 @@ describe( 'BasicHtmlWriter', () => {

const data = basicHtmlWriter.getHtml( fragment );
expect( data ).to.equal( '<p>foo bar</p>' );

// Verify if node was not adopted to main document.
expect( paragraph.ownerDocument ).not.equal( document );
expect( paragraph.firstChild.ownerDocument ).not.equal( document );
} );

it( 'should return correct HTML from fragment with multiple child nodes', () => {
Expand All @@ -51,5 +58,12 @@ describe( 'BasicHtmlWriter', () => {
const data = basicHtmlWriter.getHtml( fragment );

expect( data ).to.equal( 'foo bar<p>foo</p><div>bar</div>' );

// Verify if node was not adopted to main document.
expect( text.ownerDocument ).not.equal( document );
expect( paragraph.ownerDocument ).not.equal( document );
expect( paragraph.firstChild.ownerDocument ).not.equal( document );
expect( div.ownerDocument ).not.equal( document );
expect( div.firstChild.ownerDocument ).not.equal( document );
} );
} );
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,7 @@ describe( 'DomConverter', () => {
const viewElement = writer.createContainerElement( 'p', {}, { renderUnsafeAttributes: [ 'onclick' ] } );
viewElement.getFillerOffset = () => null;

const domElement = converter.viewToDom( viewElement, document );
const domElement = converter.viewToDom( viewElement );

converter.setDomElementAttribute( domElement, 'onclick', 'bar', viewElement );

Expand Down
Loading

0 comments on commit a0244b0

Please sign in to comment.