Skip to content

Commit

Permalink
Merge pull request #9173 from ckeditor/i/9166-resizing-small-images
Browse files Browse the repository at this point in the history
Fix (image): An image should never overflow the widget boundaries while changing its size. Closes #9166.

Fix (image): The size label should be displayed above the image if it doesn't fit inside. See #9166.

Fix (image): An Image caption placeholder text should not wrap or overflow. Closes #9162.

Internal (widget): Renamed `Resizer#_sizeUI` to `Resizer#_sizeView` to better reflect the name of the class. Closes #9162.
  • Loading branch information
oleq authored Mar 16, 2021
2 parents 87c458d + 53355b8 commit bc9eeb9
Show file tree
Hide file tree
Showing 5 changed files with 255 additions and 71 deletions.
14 changes: 11 additions & 3 deletions packages/ckeditor5-image/theme/image.css
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
/* Make sure there is some space between the content and the image. Center image by default. */
margin: 1em auto;

/* Make sure the caption will be displayed properly (See: https://github.com/ckeditor/ckeditor5/issues/1870). */
min-width: 50px;

& img {
/* Prevent unnecessary margins caused by line-height (see #44). */
display: block;
Expand All @@ -21,9 +24,6 @@

/* Make sure the image never exceeds the size of the parent container (ckeditor/ckeditor5-ui#67). */
max-width: 100%;

/* Make sure the caption will be displayed properly (See: https://github.com/ckeditor/ckeditor5/issues/1870). */
min-width: 50px;
}
}

Expand Down Expand Up @@ -61,6 +61,14 @@
& .image > figcaption.ck-placeholder::before {
padding-left: inherit;
padding-right: inherit;

/*
* Make sure the image caption placeholder doesn't overflow the placeholder area.
* See https://github.com/ckeditor/ckeditor5/issues/9162.
*/
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}

/*
Expand Down
74 changes: 8 additions & 66 deletions packages/ckeditor5-widget/src/widgetresize/resizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
* @module widget/widgetresize/resizer
*/

import View from '@ckeditor/ckeditor5-ui/src/view';
import Template from '@ckeditor/ckeditor5-ui/src/template';
import Rect from '@ckeditor/ckeditor5-utils/src/dom/rect';
import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays';
Expand All @@ -16,6 +15,7 @@ import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
import mix from '@ckeditor/ckeditor5-utils/src/mix';

import ResizeState from './resizerstate';
import SizeView from './sizeview';

/**
* Represents a resizer for a single resizable object.
Expand All @@ -41,7 +41,7 @@ export default class Resizer {
*
* @protected
* @readonly
* @member {module:widget/widgetresize/resizer~SizeView} #_sizeUI
* @member {module:widget/widgetresize/sizeview~SizeView} #_sizeView
*/

/**
Expand Down Expand Up @@ -153,7 +153,7 @@ export default class Resizer {
begin( domResizeHandle ) {
this.state = new ResizeState( this._options );

this._sizeUI.bindToState( this._options, this.state );
this._sizeView._bindToState( this._options, this.state );

this._initialViewWidth = this._options.viewElement.getStyle( 'width' );

Expand Down Expand Up @@ -307,8 +307,7 @@ export default class Resizer {
* @protected
*/
_cleanup() {
this._sizeUI.dismiss();
this._sizeUI.isVisible = false;
this._sizeView._dismiss();

const editingView = this._options.editor.editing.view;

Expand Down Expand Up @@ -440,20 +439,18 @@ export default class Resizer {
}

/**
* Sets up the {@link #_sizeUI} property and adds it to the passed `domElement`.
* Sets up the {@link #_sizeView} property and adds it to the passed `domElement`.
*
* @private
* @param {HTMLElement} domElement
*/
_appendSizeUI( domElement ) {
const sizeUI = new SizeView();
this._sizeView = new SizeView();

// Make sure icon#element is rendered before passing to appendChild().
sizeUI.render();
this._sizeView.render();

this._sizeUI = sizeUI;

domElement.appendChild( sizeUI.element );
domElement.appendChild( this._sizeView.element );
}

/**
Expand All @@ -475,61 +472,6 @@ export default class Resizer {

mix( Resizer, ObservableMixin );

/**
* A view displaying the proposed new element size during the resizing.
*
* @extends {module:ui/view~View}
*/
class SizeView extends View {
constructor() {
super();

const bind = this.bindTemplate;

this.setTemplate( {
tag: 'div',
attributes: {
class: [
'ck',
'ck-size-view',
bind.to( 'activeHandlePosition', value => value ? `ck-orientation-${ value }` : '' )
],
style: {
display: bind.if( 'isVisible', 'none', visible => !visible )
}
},
children: [ {
text: bind.to( 'label' )
} ]
} );
}

bindToState( options, resizerState ) {
this.bind( 'isVisible' ).to( resizerState, 'proposedWidth', resizerState, 'proposedHeight', ( width, height ) =>
width !== null && height !== null );

this.bind( 'label' ).to(
resizerState, 'proposedHandleHostWidth',
resizerState, 'proposedHandleHostHeight',
resizerState, 'proposedWidthPercents',
( width, height, widthPercents ) => {
if ( options.unit === 'px' ) {
return `${ width }×${ height }`;
} else {
return `${ widthPercents }%`;
}
}
);

this.bind( 'activeHandlePosition' ).to( resizerState );
}

dismiss() {
this.unbind();
this.isVisible = false;
}
}

// @private
// @param {String} resizerPosition Expected resizer position like `"top-left"`, `"bottom-right"`.
// @returns {String} A prefixed HTML class name for the resizer element
Expand Down
114 changes: 114 additions & 0 deletions packages/ckeditor5-widget/src/widgetresize/sizeview.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/**
* @license Copyright (c) 2003-2021, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/**
* @module widget/widgetresize/sizeview
*/

import View from '@ckeditor/ckeditor5-ui/src/view';

/**
* A view displaying the proposed new element size during the resizing.
*
* @protected
* @extends {module:ui/view~View}
*/
export default class SizeView extends View {
constructor() {
super();

/**
* The visibility of the view defined based on the existence of the host proposed dimensions.
*
* @private
* @observable
* @readonly
* @member {Boolean} #_isVisible
*/

/**
* The text that will be displayed in the `SizeView` child.
* It can be formatted as the pixel values (e.g. 10x20) or the percentage value (e.g. 10%).
*
* @private
* @observable
* @readonly
* @member {Boolean} #_label
*/

/**
* The position of the view defined based on the host size and active handle position.
*
* @private
* @observable
* @readonly
* @member {String} #_viewPosition
*/

const bind = this.bindTemplate;

this.setTemplate( {
tag: 'div',
attributes: {
class: [
'ck',
'ck-size-view',
bind.to( '_viewPosition', value => value ? `ck-orientation-${ value }` : '' )
],
style: {
display: bind.if( '_isVisible', 'none', visible => !visible )
}
},
children: [ {
text: bind.to( '_label' )
} ]
} );
}

/**
* A method used for binding the `SizeView` instance properties to the `ResizeState` instance observable properties.
*
* @protected
* @param {module:widget/widgetresize~ResizerOptions} options
* An object defining the resizer options, used for setting the proper size label.
* @param {module:widget/widgetresize/resizerstate~ResizeState} resizeState
* The `ResizeState` class instance, used for keeping the `SizeView` state up to date.
*/
_bindToState( options, resizeState ) {
this.bind( '_isVisible' ).to( resizeState, 'proposedWidth', resizeState, 'proposedHeight', ( width, height ) =>
width !== null && height !== null );

this.bind( '_label' ).to(
resizeState, 'proposedHandleHostWidth',
resizeState, 'proposedHandleHostHeight',
resizeState, 'proposedWidthPercents',
( width, height, widthPercents ) => {
if ( options.unit === 'px' ) {
return `${ width }×${ height }`;
} else {
return `${ widthPercents }%`;
}
}
);

this.bind( '_viewPosition' ).to(
resizeState, 'activeHandlePosition',
resizeState, 'proposedHandleHostWidth',
resizeState, 'proposedHandleHostHeight',
// If the widget is too small to contain the size label, display the label above.
( position, width, height ) => width < 50 || height < 50 ? 'above-center' : position
);
}

/**
* A method used for cleaning up. It removes the bindings and hides the view.
*
* @protected
*/
_dismiss() {
this.unbind();
this._isVisible = false;
}
}
109 changes: 109 additions & 0 deletions packages/ckeditor5-widget/tests/widgetresize/sizeview.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/**
* @license Copyright (c) 2003-2021, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

import SizeView from '../../src/widgetresize/sizeview';
import ResizerState from '../../src/widgetresize/resizerstate';

describe( 'SizeView', () => {
let sizeView, state;

beforeEach( () => {
sizeView = new SizeView();
state = new ResizerState();

sizeView._bindToState( {}, state );
sizeView.render();
} );

describe( 'constructor()', () => {
it( 'sets a proper template structure', () => {
const template = sizeView.template;

expect( template.tag ).to.equal( 'div' );
expect( template.attributes.class ).to.have.length( 3 ).and.include( 'ck', 'ck-size-view', '' );
expect( template.attributes.style[ 0 ] ).to.have.property( 'display' );
expect( template.children[ 0 ] ).to.have.property( 'text' );
} );
} );

describe( 'view label', () => {
it( 'should have proper text if the resizing unit is not pixels', () => {
state.update( {
handleHostWidth: 50,
handleHostHeight: 50,
widthPercents: 20
} );

expect( sizeView.element.innerText ).to.equal( '20%' );
} );

it( 'should have proper text if the resizing unit is pixels', () => {
sizeView._dismiss();
sizeView._bindToState( { unit: 'px' }, state );

state.update( {
handleHostWidth: 50,
handleHostHeight: 50,
widthPercents: 20
} );

expect( sizeView.element.innerText ).to.equal( '50×50' );
} );
} );

describe( 'view visibility', () => {
it( 'should not be visible if the state proposedHeight and proposedWidth are null', () => {
state.update( {
width: null,
height: null
} );

expect( sizeView.element.style.display ).to.equal( 'none' );
} );

it( 'should be visible if the state proposedHeight and proposedWidth are not null', () => {
state.update( {
width: 50,
height: 50
} );

expect( sizeView.element.style.display ).to.equal( '' );
} );
} );

describe( 'view position', () => {
it( 'should have a valid class if the widget width is less than 50px', () => {
state.activeHandlePosition = 'top-right';
state.update( {
handleHostWidth: 49,
handleHostHeight: 200
} );

expect( sizeView.element.classList.contains( 'ck-orientation-above-center' ) ).to.be.true;
} );

it( 'should have a valid class if the widget height is less than 50px', () => {
state.activeHandlePosition = 'top-right';
state.update( {
handleHostWidth: 200,
handleHostHeight: 49
} );

expect( sizeView.element.classList.contains( 'ck-orientation-above-center' ) ).to.be.true;
} );

it( 'should have a valid class if the widget dimensions are greater than 50px/50px', () => {
const position = 'top-right';

state.activeHandlePosition = position;
state.update( {
handleHostWidth: 200,
handleHostHeight: 200
} );

expect( sizeView.element.classList.contains( `ck-orientation-${ position }` ) ).to.be.true;
} );
} );
} );
Loading

0 comments on commit bc9eeb9

Please sign in to comment.