Skip to content

Commit

Permalink
Merge pull request #9675 from ckeditor/cf/4009
Browse files Browse the repository at this point in the history 
Internal (image): Added post-fixer for the `imageStyle` attribute to ensure a valid value for a given image type.
  • Loading branch information
Magdalena Chrześcian authored May 24, 2021
2 parents 9b873e1 + 6bde6ae commit ded26d8
Show file tree
Hide file tree
Showing 5 changed files with 230 additions and 15 deletions.
23 changes: 18 additions & 5 deletions packages/ckeditor5-image/src/imagestyle/imagestylecommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,32 +93,45 @@ export default class ImageStyleCommand extends Command {
* {@link module:image/imagestyle~ImageStyleConfig#options}).
* @fires execute
*/
execute( options ) {
execute( options = {} ) {
const editor = this.editor;
const model = editor.model;
const imageUtils = editor.plugins.get( 'ImageUtils' );

model.change( writer => {
const requestedStyle = options.value;
const supportedTypes = this._styles.get( requestedStyle ).modelElements;

let imageElement = imageUtils.getClosestSelectedImageElement( model.document.selection );

// Change the image type if a style requires it.
if ( !supportedTypes.includes( imageElement.name ) ) {
this.editor.execute( !supportedTypes.includes( 'imageBlock' ) ? 'imageTypeInline' : 'imageTypeBlock' );
if ( requestedStyle && this.shouldConvertImageType( requestedStyle, imageElement ) ) {
this.editor.execute( imageUtils.isBlockImage( imageElement ) ? 'imageTypeInline' : 'imageTypeBlock' );

// Update the imageElement to the newly created image.
imageElement = imageUtils.getClosestSelectedImageElement( model.document.selection );
}

// Default style means that there is no `imageStyle` attribute in the model.
// https://github.com/ckeditor/ckeditor5-image/issues/147
if ( this._styles.get( requestedStyle ).isDefault ) {
if ( !requestedStyle || this._styles.get( requestedStyle ).isDefault ) {
writer.removeAttribute( 'imageStyle', imageElement );
} else {
writer.setAttribute( 'imageStyle', requestedStyle, imageElement );
}
} );
}

/**
* Returns `true` if requested style change would trigger the image type change.
*
* @param {module:image/imagestyle~ImageStyleOptionDefinition} requestedStyle The name of the style (as configured in
* {@link module:image/imagestyle~ImageStyleConfig#options}).
* @param {module:engine/model/element~Element} imageElement The image model element.
* @returns {Boolean}
*/
shouldConvertImageType( requestedStyle, imageElement ) {
const supportedTypes = this._styles.get( requestedStyle ).modelElements;

return !supportedTypes.includes( imageElement.name );
}
}
58 changes: 57 additions & 1 deletion packages/ckeditor5-image/src/imagestyle/imagestyleediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@

import { Plugin } from 'ckeditor5/src/core';
import ImageStyleCommand from './imagestylecommand';
import { viewToModelStyleAttribute, modelToViewStyleAttribute } from './converters';
import ImageUtils from '../imageutils';
import utils from './utils';
import { viewToModelStyleAttribute, modelToViewStyleAttribute } from './converters';

/**
* The image style engine plugin. It sets the default configuration, creates converters and registers
Expand All @@ -26,6 +27,13 @@ export default class ImageStyleEditing extends Plugin {
return 'ImageStyleEditing';
}

/**
* @inheritDoc
*/
static get requires() {
return [ ImageUtils ];
}

/**
* @inheritDoc
*/
Expand Down Expand Up @@ -56,6 +64,7 @@ export default class ImageStyleEditing extends Plugin {
} );

this._setupConversion( isBlockPluginLoaded, isInlinePluginLoaded );
this._setupPostFixer();

// Register imageStyle command.
editor.commands.add( 'imageStyle', new ImageStyleCommand( editor, this.normalizedStyles ) );
Expand Down Expand Up @@ -96,4 +105,51 @@ export default class ImageStyleEditing extends Plugin {
editor.data.upcastDispatcher.on( 'element:img', viewToModelConverter, { priority: 'low' } );
}
}

/**
* Registers a post-fixer that will make sure that the style attribute value is correct for a specific image type (block vs inline).
*
* @private
*/
_setupPostFixer() {
const editor = this.editor;
const document = editor.model.document;

const imageUtils = editor.plugins.get( ImageUtils );
const stylesMap = new Map( this.normalizedStyles.map( style => [ style.name, style ] ) );

// Make sure that style attribute is valid for the image type.
document.registerPostFixer( writer => {
let changed = false;

for ( const change of document.differ.getChanges() ) {
if ( change.type == 'insert' || change.type == 'attribute' && change.attributeKey == 'imageStyle' ) {
let element = change.type == 'insert' ? change.position.nodeAfter : change.range.start.nodeAfter;

if ( element && element.is( 'element', 'paragraph' ) && element.childCount > 0 ) {
element = element.getChild( 0 );
}

if ( !imageUtils.isImage( element ) ) {
continue;
}

const imageStyle = element.getAttribute( 'imageStyle' );

if ( !imageStyle ) {
continue;
}

const imageStyleDefinition = stylesMap.get( imageStyle );

if ( !imageStyleDefinition || !imageStyleDefinition.modelElements.includes( element.name ) ) {
writer.removeAttribute( 'imageStyle', element );
changed = true;
}
}
}

return changed;
} );
}
}
3 changes: 0 additions & 3 deletions packages/ckeditor5-image/src/imageutils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ export default class ImageUtils extends Plugin {
/**
* Returns a image model element if one is selected or is among the selection's ancestors.
*
* @protected
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection
* @returns {module:engine/model/element~Element|null}
*/
Expand Down Expand Up @@ -212,7 +211,6 @@ export default class ImageUtils extends Plugin {
/**
* Checks if the provided model element is an `image`.
*
* @protected
* @param {module:engine/model/element~Element} modelElement
* @returns {Boolean}
*/
Expand All @@ -223,7 +221,6 @@ export default class ImageUtils extends Plugin {
/**
* Checks if the provided model element is an `imageInline`.
*
* @protected
* @param {module:engine/model/element~Element} modelElement
* @returns {Boolean}
*/
Expand Down
58 changes: 58 additions & 0 deletions packages/ckeditor5-image/tests/imagestyle/imagestylecommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,15 @@ describe( 'ImageStyleCommand', () => {
expect( getData( model ) ).to.equal( '<paragraph>[<imageInline></imageInline>]</paragraph>' );
expect( command.value ).to.equal( defaultInline.name );
} );

it( 'should set default style if no style specified', () => {
setData( model, '<paragraph>[<imageInline imageStyle="${ anyImage.name }"></imageInline>]</paragraph>' );

command.execute();

expect( getData( model ) ).to.equal( '<paragraph>[<imageInline></imageInline>]</paragraph>' );
expect( command.value ).to.equal( defaultInline.name );
} );
} );

describe( 'when a block image is selected', () => {
Expand Down Expand Up @@ -406,6 +415,15 @@ describe( 'ImageStyleCommand', () => {
expect( getData( model ) ).to.equal( '[<imageBlock><caption></caption></imageBlock>]' );
expect( command.value ).to.equal( defaultBlock.name );
} );

it( 'should set default style if no style specified', () => {
setData( model, '[<imageBlock imageStyle="${ anyImage.name }"><caption></caption></imageBlock>]' );

command.execute();

expect( getData( model ) ).to.equal( '[<imageBlock><caption></caption></imageBlock>]' );
expect( command.value ).to.equal( defaultBlock.name );
} );
} );

it( 'should set the style if the selection is inside a caption', () => {
Expand All @@ -417,4 +435,44 @@ describe( 'ImageStyleCommand', () => {
} );
} );
} );

describe( 'shouldConvertImageType()', () => {
const imgSrc = 'assets/sample.png';

describe( 'for an inline image', () => {
it( 'should return true if the requested type is other than imageInline', () => {
setData( model, `<paragraph>[<imageInline src="${ imgSrc }"></imageInline>]</paragraph>` );

const image = model.document.selection.getSelectedElement();

expect( command.shouldConvertImageType( onlyBlock.name, image ) ).to.be.true;
} );

it( 'should return false if the requested type equals imageInline', () => {
setData( model, `<paragraph>[<imageInline src="${ imgSrc }"></imageInline>]</paragraph>` );

const image = model.document.selection.getSelectedElement();

expect( command.shouldConvertImageType( defaultInline.name, image ) ).to.be.false;
} );
} );

describe( 'for a block image', () => {
it( 'should return true if the requested type is other than imageBlock', () => {
setData( model, `[<imageBlock src="${ imgSrc }"></imageBlock>]` );

const image = model.document.selection.getSelectedElement();

expect( command.shouldConvertImageType( onlyInline.name, image ) ).to.be.true;
} );

it( 'should return false if the requested type equals imageBlock', () => {
setData( model, `[<imageBlock src="${ imgSrc }"><caption></caption></imageBlock>]` );

const image = model.document.selection.getSelectedElement();

expect( command.shouldConvertImageType( onlyBlock.name, image ) ).to.be.false;
} );
} );
} );
} );
103 changes: 97 additions & 6 deletions packages/ckeditor5-image/tests/imagestyle/imagestyleediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,21 @@
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor';
import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';

import ImageStyleEditing from '../../src/imagestyle/imagestyleediting';
import ImageBlockEditing from '../../src/image/imageblockediting';
import ImageInlineEditing from '../../src/image/imageinlineediting';
import ImageStyleCommand from '../../src/imagestyle/imagestylecommand';
import imageStyleUtils from '../../src/imagestyle/utils';
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';

import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';

import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
import ImageEditing from '../../src/image/imageediting';
import ImageResizeEditing from '../../src/imageresize/imageresizeediting';
import ImageUtils from '../../src/imageutils';

describe( 'ImageStyleEditing', () => {
describe( 'plugin', () => {
Expand All @@ -37,6 +37,10 @@ describe( 'ImageStyleEditing', () => {
expect( ImageStyleEditing.pluginName ).to.equal( 'ImageStyleEditing' );
} );

it( 'requires ImageUtils ', () => {
expect( ImageStyleEditing.requires ).to.deep.equal( [ ImageUtils ] );
} );

afterEach( () => {
editor.destroy();
} );
Expand Down Expand Up @@ -201,6 +205,93 @@ describe( 'ImageStyleEditing', () => {
} );
} );

describe( 'model post-fixer', () => {
let editor, model, document;

beforeEach( async () => {
editor = await ModelTestEditor.create( {
plugins: [ ImageBlockEditing, ImageInlineEditing, ImageStyleEditing, Paragraph ],
image: {
styles: {
options: [
{ name: 'forBlock', modelElements: [ 'imageBlock' ] },
{ name: 'forInline', modelElements: [ 'imageInline' ] }
]
}
}
} );

model = editor.model;
document = model.document;
} );

afterEach( () => {
editor.destroy();
} );

it( 'should remove imageStyle attribute with invalid value', () => {
setModelData( model, '<imageBlock src="/assets/sample.png" imageStyle="foo"></imageBlock>' );

const image = document.getRoot().getChild( 0 );

expect( image.hasAttribute( 'imageStyle' ) ).to.be.false;
} );

it( 'should remove imageStyle attribute with invalid value (after changing attribute value)', () => {
setModelData( model, '<imageBlock src="/assets/sample.png"></imageBlock>' );

const image = document.getRoot().getChild( 0 );

model.change( writer => {
writer.setAttribute( 'imageStyle', 'foo', image );
} );

expect( image.hasAttribute( 'imageStyle' ) ).to.be.false;
} );

it( 'should remove imageStyle attribute with invalid value (after changing image type)', () => {
setModelData( model, '[<imageBlock src="/assets/sample.png" imageStyle="full"></imageBlock>]' );

editor.execute( 'imageTypeInline' );

const image = document.getRoot().getChild( 0 ).getChild( 0 );

expect( image.hasAttribute( 'imageStyle' ) ).to.be.false;
} );

it( 'should remove imageStyle attribute with value not allowed for a block image', () => {
setModelData( model, '[<imageBlock src="/assets/sample.png" imageStyle="forInline"></imageBlock>]' );

const image = document.getRoot().getChild( 0 );

expect( image.hasAttribute( 'imageStyle' ) ).to.be.false;
} );

it( 'should remove imageStyle attribute with value not allowed for an inline image', () => {
setModelData( model, '<paragraph>[<imageInline src="/assets/sample.png" imageStyle="forBlock"></imageInline>]</paragraph>' );

const image = document.getRoot().getChild( 0 ).getChild( 0 );

expect( image.hasAttribute( 'imageStyle' ) ).to.be.false;
} );

it( 'should not remove imageStyle attribute with value allowed for a block image', () => {
setModelData( model, '[<imageBlock src="/assets/sample.png" imageStyle="forBlock"></imageBlock>]' );

const image = document.getRoot().getChild( 0 );

expect( image.getAttribute( 'imageStyle' ) ).to.equal( 'forBlock' );
} );

it( 'should not remove imageStyle attribute with value allowed for an inline image', () => {
setModelData( model, '<paragraph>[<imageInline src="/assets/sample.png" imageStyle="forInline"></imageInline>]</paragraph>' );

const image = document.getRoot().getChild( 0 ).getChild( 0 );

expect( image.getAttribute( 'imageStyle' ) ).to.equal( 'forInline' );
} );
} );

describe( 'conversion', () => {
let editor, model, viewDocument, document;

Expand Down

0 comments on commit ded26d8

Please sign in to comment.