Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle <style> tag #11235

Merged
merged 8 commits into from
Feb 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 30 additions & 9 deletions packages/ckeditor5-engine/src/view/domconverter.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ 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 UNSAFE_ATTRIBUTE_NAME_PREFIX = 'data-ck-unsafe-attribute-';
const UNSAFE_ELEMENT_REPLACEMENT_ATTRIBUTE = 'data-ck-unsafe-element';
const UNSAFE_ELEMENTS = [ 'script', 'style' ];

/**
* `DomConverter` is a set of tools to do transformations between DOM nodes and view nodes. It also handles
Expand Down Expand Up @@ -323,7 +324,7 @@ export default class DomConverter {

// There are certain nodes, that should be renamed to <span> in editing pipeline.
if ( this._shouldRenameElement( elementName ) ) {
logWarning( 'domconverter-unsafe-element-detected', { unsafeElement: currentNode } );
_logUnsafeElement( elementName );

currentNode.replaceWith( this._createReplacementDomElement( elementName, currentNode ) );
}
Expand Down Expand Up @@ -384,7 +385,7 @@ export default class DomConverter {
} else {
// Create DOM element.
if ( this._shouldRenameElement( viewNode.name ) ) {
logWarning( 'domconverter-unsafe-element-detected', { unsafeElement: viewNode } );
_logUnsafeElement( viewNode.name );

domElement = this._createReplacementDomElement( viewNode.name );
} else if ( viewNode.hasAttribute( 'xmlns' ) ) {
Expand Down Expand Up @@ -1546,7 +1547,9 @@ export default class DomConverter {
* @returns {Boolean}
*/
_shouldRenameElement( elementName ) {
return this.renderingMode == 'editing' && elementName.toLowerCase() == 'script';
const name = elementName.toLowerCase();

return this.renderingMode === 'editing' && UNSAFE_ELEMENTS.includes( name );
}

/**
Expand Down Expand Up @@ -1626,6 +1629,20 @@ function hasBlockParent( domNode, blockElements ) {
return parent && parent.tagName && blockElements.includes( parent.tagName.toLowerCase() );
}

// Log to console the information about element that was replaced.
// Check UNSAFE_ELEMENTS for all recognized unsafe elements.
//
// @param {String} elementName The name of the view element
function _logUnsafeElement( elementName ) {
if ( elementName === 'script' ) {
logWarning( 'domconverter-unsafe-script-element-detected' );
}

if ( elementName === 'style' ) {
logWarning( 'domconverter-unsafe-style-element-detected' );
}
}

/**
* Enum representing the type of the block filler.
*
Expand All @@ -1640,13 +1657,17 @@ function hasBlockParent( domNode, blockElements ) {
*/

/**
* The {@link module:engine/view/domconverter~DomConverter} detected a `<script>` element that may disrupt the
* {@glink framework/guides/architecture/editing-engine#editing-pipeline editing pipeline} of the editor. To avoid this,
* the `<script>` element was renamed to `<span data-ck-unsafe-element="script"></span>`.
* While rendering the editor content, the {@link module:engine/view/domconverter~DomConverter} detected a `<script>` element that may
* disrupt the editing experience. To avoid this, the `<script>` element was replaced with `<span data-ck-unsafe-element="script"></span>`.
*
* @error domconverter-unsafe-script-element-detected
*/

/**
* While rendering the editor content, the {@link module:engine/view/domconverter~DomConverter} detected a `<style>` element that may affect
* the editing experience. To avoid this, the `<style>` element was replaced with `<span data-ck-unsafe-element="style"></span>`.
*
* @error domconverter-unsafe-element-detected
* @param {module:engine/model/element~Element|HTMLElement} unsafeElement The editing view or DOM element
Reinmar marked this conversation as resolved.
Show resolved Hide resolved
* that was renamed.
* @error domconverter-unsafe-style-element-detected
*/

/**
Expand Down
57 changes: 53 additions & 4 deletions packages/ckeditor5-engine/tests/view/domconverter/domconverter.js
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,16 @@ describe( 'DomConverter', () => {

expect( element.innerHTML ).to.equal( '<div>foo<script onclick="foo">bar</script></div>' );
} );

it( 'should keep style element', () => {
const element = document.createElement( 'p' );
const html = '<div>foo<style nonce="foo">bar</style></div>';

converter.renderingMode = 'data';
converter.setContentOf( element, html );

expect( element.innerHTML ).to.equal( '<div>foo<style nonce="foo">bar</style></div>' );
} );
} );

describe( 'editing pipeline', () => {
Expand Down Expand Up @@ -736,16 +746,28 @@ describe( 'DomConverter', () => {
);
} );

it( 'should warn when an unsafe element was detected and renamed', () => {
it( 'should warn when an unsafe script element was detected and renamed', () => {
const element = document.createElement( 'p' );
const html = '<div>foo<script class="foo-class" style="foo-style" data-foo="bar">bar</script></div>';

converter.setContentOf( element, html );

sinon.assert.calledOnce( warnStub );
sinon.assert.calledWithExactly( warnStub,
sinon.match( /^domconverter-unsafe-element-detected/ ),
sinon.match.has( 'unsafeElement', sinon.match.has( 'tagName', 'SCRIPT' ) ),
sinon.match( /^domconverter-unsafe-script-element-detected/ ),
sinon.match.string // Link to the documentation
);
} );

it( 'should warn when an unsafe style element was detected and renamed', () => {
const element = document.createElement( 'p' );
const html = '<div>foo<style class="foo-class" nonce="foo-nonce" data-foo="bar">bar</style></div>';

converter.setContentOf( element, html );

sinon.assert.calledOnce( warnStub );
sinon.assert.calledWithExactly( warnStub,
sinon.match( /^domconverter-unsafe-style-element-detected/ ),
sinon.match.string // Link to the documentation
);
} );
Expand Down Expand Up @@ -918,7 +940,11 @@ describe( 'DomConverter', () => {
.callsFake( () => {} );

console.warn
.withArgs( sinon.match( /^domconverter-unsafe-element-detected/ ) )
.withArgs( sinon.match( /^domconverter-unsafe-script-element-detected/ ) )
.callsFake( () => {} );

console.warn
.withArgs( sinon.match( /^domconverter-unsafe-style-element-detected/ ) )
.callsFake( () => {} );

console.warn.callThrough();
Expand Down Expand Up @@ -966,5 +992,28 @@ describe( 'DomConverter', () => {
'<p>foo<span data-ck-unsafe-element="script" style="foo-style" data-foo="bar">bar</span></p>'
);
} );

it( 'should skip removing the (replacement) attribute representing the unsafe <style> tag', () => {
const domElement = document.createElement( 'p' );
const html = 'foo<style class="foo-class" style="foo-style" data-foo="bar">bar</style>';

converter.setContentOf( domElement, html );

expect( domElement.outerHTML ).to.equal(
'<p>foo<span data-ck-unsafe-element="style" class="foo-class" style="foo-style" data-foo="bar">bar</span></p>'
);

converter.removeDomElementAttribute( domElement.lastChild, 'data-ck-unsafe-element' );

expect( domElement.outerHTML ).to.equal(
'<p>foo<span data-ck-unsafe-element="style" class="foo-class" style="foo-style" data-foo="bar">bar</span></p>'
);

converter.removeDomElementAttribute( domElement.lastChild, 'class' );

expect( domElement.outerHTML ).to.equal(
'<p>foo<span data-ck-unsafe-element="style" style="foo-style" data-foo="bar">bar</span></p>'
);
} );
} );
} );
49 changes: 47 additions & 2 deletions packages/ckeditor5-engine/tests/view/domconverter/view-to-dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,53 @@ describe( 'DomConverter', () => {

sinon.assert.calledOnce( warnStub );
sinon.assert.calledWithExactly( warnStub,
sinon.match( /^domconverter-unsafe-element-detected/ ),
sinon.match.has( 'unsafeElement', sinon.match.has( 'name', 'script' ) ),
sinon.match( /^domconverter-unsafe-script-element-detected/ ),
sinon.match.string // Link to the documentation
);
} );

it( 'should replace style with span and add special data attribute', () => {
const viewScript = new ViewElement( viewDocument, 'style' );
const viewText = new ViewText( viewDocument, 'foo' );
const viewP = new ViewElement( viewDocument, 'p', { class: 'foo' } );

viewP._appendChild( viewScript );
viewP._appendChild( viewText );

converter = new DomConverter( viewDocument, {
renderingMode: 'editing'
} );

const domP = converter.viewToDom( viewP, document );

expect( domP ).to.be.an.instanceof( HTMLElement );
expect( domP.tagName ).to.equal( 'P' );
expect( domP.getAttribute( 'class' ) ).to.equal( 'foo' );
expect( domP.attributes.length ).to.equal( 1 );

expect( domP.childNodes.length ).to.equal( 2 );
expect( domP.childNodes[ 0 ].tagName ).to.equal( 'SPAN' );
expect( domP.childNodes[ 0 ].getAttribute( 'data-ck-unsafe-element' ) ).to.equal( 'style' );
expect( domP.childNodes[ 1 ].data ).to.equal( 'foo' );
} );

it( 'should warn when an unsafe style was filtered out', () => {
const viewStyle = new ViewElement( viewDocument, 'style' );
const viewText = new ViewText( viewDocument, 'foo' );
const viewP = new ViewElement( viewDocument, 'p', { class: 'foo' } );

viewP._appendChild( viewStyle );
viewP._appendChild( viewText );

converter = new DomConverter( viewDocument, {
renderingMode: 'editing'
} );

converter.viewToDom( viewP, document );

sinon.assert.calledOnce( warnStub );
sinon.assert.calledWithExactly( warnStub,
sinon.match( /^domconverter-unsafe-style-element-detected/ ),
sinon.match.string // Link to the documentation
);
} );
Expand Down
24 changes: 23 additions & 1 deletion packages/ckeditor5-engine/tests/view/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3981,7 +3981,11 @@ describe( 'Renderer', () => {
.callsFake( () => {} );

console.warn
.withArgs( sinon.match( /^domconverter-unsafe-element-detected/ ) )
.withArgs( sinon.match( /^domconverter-unsafe-script-element-detected/ ) )
.callsFake( () => {} );

console.warn
.withArgs( sinon.match( /^domconverter-unsafe-style-element-detected/ ) )
.callsFake( () => {} );

console.warn.callThrough();
Expand Down Expand Up @@ -4027,6 +4031,24 @@ describe( 'Renderer', () => {
delete window.spy;
} );

it( 'should replace style element with span and custom data attribute', () => {
window.spy = sinon.spy();

const viewA = new ViewElement( viewDoc, 'style' );

// Assign content of the `<style>` element, as the utility method `setVewData` will fail becasuse of brace characters.
viewA._appendChild( new ViewText( viewDoc, '.foo { color: red; }' ) );
viewRoot._appendChild( viewA );

view.forceRender();

expect( window.spy.calledOnce ).to.be.false;
expect( getViewData( view ) ).to.equal( '<style>.foo { color: red; }</style>' );
expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( '<span data-ck-unsafe-element="style">.foo { color: red; }</span>' );

delete window.spy;
} );

it( 'should rename attributes that can affect editing pipeline', () => {
setViewData( view,
'<container:p onclick="test">' +
Expand Down
4 changes: 3 additions & 1 deletion packages/ckeditor5-html-support/src/generalhtmlsupport.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import ImageElementSupport from './integrations/image';
import MediaEmbedElementSupport from './integrations/mediaembed';
import ScriptElementSupport from './integrations/script';
import TableElementSupport from './integrations/table';
import StyleElementSupport from './integrations/style';

/**
* The General HTML Support feature.
Expand Down Expand Up @@ -46,7 +47,8 @@ export default class GeneralHtmlSupport extends Plugin {
ImageElementSupport,
MediaEmbedElementSupport,
ScriptElementSupport,
TableElementSupport
TableElementSupport,
StyleElementSupport
];
}

Expand Down
73 changes: 73 additions & 0 deletions packages/ckeditor5-html-support/src/integrations/style.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* @license Copyright (c) 2003-2022, CKSource Holding sp. z o.o. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/**
* @module html-support/integrations/style
*/

import { Plugin } from 'ckeditor5/src/core';
import {
createObjectView,
modelToViewBlockAttributeConverter,
viewToModelBlockAttributeConverter,
viewToModelObjectConverter
} from '../converters.js';

import DataFilter from '../datafilter';

/**
* Provides the General HTML Support for `style` elements.
*
* @extends module:core/plugin~Plugin
*/
export default class StyleElementSupport extends Plugin {
/**
* @inheritDoc
*/
static get requires() {
return [ DataFilter ];
}

/**
* @inheritDoc
*/
init() {
const dataFilter = this.editor.plugins.get( DataFilter );

dataFilter.on( 'register:style', ( evt, definition ) => {
const editor = this.editor;
const schema = editor.model.schema;
const conversion = editor.conversion;

schema.register( 'htmlStyle', definition.modelSchema );

schema.extend( 'htmlStyle', {
allowAttributes: [ 'htmlAttributes', 'htmlContent' ]
} );

editor.data.registerRawContentMatcher( {
name: 'style'
} );

conversion.for( 'upcast' ).elementToElement( {
view: 'style',
model: viewToModelObjectConverter( definition )
} );

conversion.for( 'upcast' ).add( viewToModelBlockAttributeConverter( definition, dataFilter ) );

conversion.for( 'downcast' ).elementToElement( {
model: 'htmlStyle',
view: ( modelElement, { writer } ) => {
return createObjectView( 'style', modelElement, writer );
}
} );

conversion.for( 'downcast' ).add( modelToViewBlockAttributeConverter( definition ) );

evt.stop();
} );
}
}
10 changes: 9 additions & 1 deletion packages/ckeditor5-html-support/src/schemadefinitions.js
Original file line number Diff line number Diff line change
Expand Up @@ -822,7 +822,7 @@ export default {
inheritAllFrom: '$htmlObjectInline'
}
},
// TODO it could be probably represented as non-object element, although it has grafical representation,
// TODO it could be probably represented as non-object element, although it has graphical representation,
// so probably makes more sense to keep it as an object.
{
model: 'htmlProgress',
Expand All @@ -839,6 +839,14 @@ export default {
allowWhere: [ '$text', '$block' ],
isInline: true
}
},
{
model: 'htmlStyle',
view: 'style',
modelSchema: {
allowWhere: [ '$text', '$block' ],
isInline: true
}
}
]
};
Loading