Adding inline styles and attributes to widget with applyStyle #1681

engineering-this · 2018-02-15T13:10:22Z

What is the purpose of this pull request?

Feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

I have extended widget.applyRemoveStyle method so when passed CKEDITOR.style instance has property 'style' or 'attribute' it will add or remove by those properties to widget.

Closes #1674

engineering-this · 2018-02-15T13:11:49Z

Initially I was supposed to work on adding/removing styles and attributes as separate tickets, but since doing them both looks almost the same I squashed it into one PR.

engineering-this · 2018-02-15T13:21:09Z

plugins/widget/plugin.js

+					if ( !updatedClasses[ cl ] )
+						changed = updatedClasses[ cl ] = 1;
+				} else {
+					if ( updatedClasses[ cl ] ) {


Not part of my code, although I see now that I can change it to else if { with other changes requested by reviewer.

Yes, I would take this opportunity to refactor it, as you mentioned it should look something like:

while ( ( cl = classes.pop() ) ) { if ( apply ) { if ( !updatedClasses[ cl ] ) { changed = updatedClasses[ cl ] = 1; } } else if ( updatedClasses[ cl ] ) { delete updatedClasses[ cl ]; changed = 1; } }

f1ames

This issue is a little more complex as modifications in applyRemoveStyle method affects 3 other methods:

Docs for CKEDITOR.widget.applyStyle and CKEDITOR.widget.removeStyle should be updated.
There is also CKEDITOR.widget.checkStyleActive method which should be adjusted. Now it only checks if widget element contains classes from a style. It should check also for styles and attributes (not sure, but maybe some method from CKEDITOR.style could be useful, e.g. checkElementMatch).

f1ames · 2018-02-19T13:27:32Z

CHANGES.md

+
+New Features:
+
+* [#1674](https://github.com/ckeditor/ckeditor-dev/issues/1674): The [Widget](https://ckeditor.com/cke4/addon/widget) plugin accepts inline styles and HTML attributes from [`CKEDITOR.style.customHandlers.widget`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_style_customHandlers_widget.html).


Wouldn't it be more clear to say that now widgets are fully compatible or fully apply CKEDITOR.style when applied via applyStyle/removeStyle method? (Previously only classes were added, now all attributes and styles are added)

f1ames · 2018-02-19T13:32:01Z

plugins/widget/plugin.js

+					if ( !updatedClasses[ cl ] )
+						changed = updatedClasses[ cl ] = 1;
+				} else {
+					if ( updatedClasses[ cl ] ) {


Yes, I would take this opportunity to refactor it, as you mentioned it should look something like:

while ( ( cl = classes.pop() ) ) { if ( apply ) { if ( !updatedClasses[ cl ] ) { changed = updatedClasses[ cl ] = 1; } } else if ( updatedClasses[ cl ] ) { delete updatedClasses[ cl ]; changed = 1; } }

f1ames · 2018-02-19T13:39:13Z

plugins/widget/plugin.js


 		// Ee... Something is wrong with this style.
-		if ( !classes )
+		if ( !classes && !styles && !attributes )


If you could also fix the codestyle here as we rather use if wtih {.

f1ames · 2018-02-19T13:39:35Z

plugins/widget/plugin.js

+					}
+				}
+			}
+			if ( changed )


And here (if { ... }).

f1ames · 2018-02-19T13:40:56Z

plugins/widget/plugin.js

+				widget.setData( 'classes', updatedClasses );
+		}
+
+		function toggleStyleAttribute ( element, property, StyleOrAttr, apply ) {


This function is defined/created every time applyRemoveStyle is executed. I would move it outside as a helper same as getStyleClasses.

Also i don't see a reason why StyleOrAttr starts with uppercase. It is a standard param like any other in this function.

f1ames · 2018-02-19T13:43:57Z

plugins/widget/plugin.js

+		function toggleStyleAttribute ( element, property, StyleOrAttr, apply ) {
+			apply = apply ? 'set' : 'remove';
+			StyleOrAttr = StyleOrAttr ? 'Style' : 'Attribute';
+			StyleOrAttr = apply + StyleOrAttr;


You can just make one-liner out of it:

StyleOrAttr = apply + ( StyleOrAttr ? 'Style' : 'Attribute' );

f1ames · 2018-02-19T14:23:20Z

plugins/widget/plugin.js

+			StyleOrAttr = StyleOrAttr ? 'Style' : 'Attribute';
+			StyleOrAttr = apply + StyleOrAttr;
+
+			for ( var key in property ) {


Instead of iterating you may use apply/removeStyles and apply/removeAttributes.

There is no method removeStyles only removeStyle.

Method setAttributes takes object containing pairs as attribute, while removeAttributes takes array as argument.

Applying all that will make our logic more complicated than it is right now so I will stay with current approach.

f1ames

Remember to check if other unit tests are passing after modifying behaviour of existing methods. It is likely that some tests checked behaviour which was changed. I run some widget unit tests and see some are failing:

Also as checkStyleActive was modified, we need new unit tests covering new behaviour.

f1ames · 2018-02-21T12:17:44Z

CHANGES.md

+
+New Features:
+
+* [#1674](https://github.com/ckeditor/ckeditor-dev/issues/1674): The [Widget](https://ckeditor.com/cke4/addon/widget) plugin now fully applies [`CKEDITOR.style.customHandlers.widget`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_style_customHandlers_widget.html) excepts for `element` when using methods `applyStyle` or `removeStyle`.


The Widget plugin now fully supports CKEDITOR.style's attibuttes/styles via applyStyle, removeStyle and checkStyleActive methods.

☝️ Maybe something like this?

Please remember about proper linking to docs (so also method names should be linked).

f1ames · 2018-02-21T12:21:02Z

plugins/widget/plugin.js

 		 *
-		 * By default this method handles only classes defined in the style. It clones existing
-		 * classes which are stored in the {@link #property-data widget data}'s `classes` property,
+		 * Since 4.10 this method applies all attributes styles and classes from {@link CKEDITOR.style}.


all attributes and styles from {@link CKEDITOR.style}.

As class is basically attribute maybe it may be skipped here (also CKEDITOR.style does not have separate class property so mentioning it may be misleading - in opposite to styles which are fine here).

Could you apply to all 3 methods descriptions.

f1ames · 2018-02-21T12:21:26Z

plugins/widget/plugin.js

-		 * classes which are stored in the {@link #property-data widget data}'s `classes` property,
+		 * Since 4.10 this method applies all attributes styles and classes from {@link CKEDITOR.style}.
+		 *
+		 * Previously this method handled only classes defined in the style.


classes defined in {@link CKEDITOR.style} attributes.

Could you apply to all 3 methods descriptions.

f1ames · 2018-02-21T12:22:37Z

plugins/widget/plugin.js

@@ -1026,20 +1026,19 @@
 		},

 		/**
-		 * Applies the specified style to the widget. It is highly recommended to use the


Maybe we can preserve this info about recommended methods?

Could you apply to all 3 methods descriptions.

I second this: info about recommended methods should stay.

f1ames · 2018-02-21T13:43:36Z

plugins/widget/plugin.js

+		 *
+		 * Previously this method handled only classes defined in the style.
+		 *
+		 * This method adds classes by cloning existing ones which are stored in the {@link #property-data widget data}'s `classes` property,


Now it sounds somehow misleading, that classes are added by cloning existing ones. I would stay with previous version, like:

This method clones existing classes ( ... ), adds new classes, and calls...

f1ames · 2018-02-21T14:42:24Z

plugins/widget/plugin.js

-					return false;
-			}
-			return true;
+			return CKEDITOR.style.prototype.checkElementMatch.call( style, this.element );


Why not just return style.checkElementMatch( this.element );

I'm using style.prototype because using it like that would refer to style.customHandler.widget.checkElementMatch instead. But after reworking my PR I won't call it.

f1ames · 2018-02-21T15:30:56Z

plugins/widget/plugin.js

-				return false;
+			style = CKEDITOR.tools.copy( style );
+			style.element = this.element.getName();
+			style._.definition.ignoreReadonly = true;


Here is already an example the modifying copy (style._.definition.ignoreReadonly), will also modify source object. See below:

f1ames · 2018-02-21T15:30:58Z

plugins/widget/plugin.js

-
-			if ( !classes )
-				return false;
+			style = CKEDITOR.tools.copy( style );


CKEDITOR.tools.copy does a shallow copy so it is not completely safe to use it (as changing copy may change source object). There are two solutions here:

Use CKEDITOR.tools.clone which does deep copy.

Create new style using definition from source one: new CKEDITOR.style( CKEDITOR.tools.clone( style._.definition ) ) (still we need to clone the definition).

I'm for the second one as cloning whole CKEDITOR.style object may be expensive. And if you look on CKEDITOR.style constructor it doesn't perform anything fancy or computational heavy so it should be faster than cloning whole object.

f1ames · 2018-02-21T15:46:43Z

plugins/widget/plugin.js

+		toggleStyleAttribute( widget.element, attributes, 0, apply );
+	}
+
+	function toggleStyleAttribute ( element, property, styleOrAttr, apply ) {


Having a Boolean property with name like styleOrAttr is misleading. Without looking at function code is not possible to assume what styleOrAttr=false or styleOrAttr=true really means. But, it is also quite difficult to come up with proper name in such cases 🤔

You may go same way as apply param - if true it means apply/set if not - remove. So here it would be attributes - true means apply attributes, false - styles (far from perfect IMHO).

The more readable way (but a little bit "inflated" too) would be something like:

function toogleAttributes( element, properties, apply ) { toogleProperties( element, properties, apply, 'Attribute' ); } function toogleStyles( element, properties, apply ) { toogleProperties( element, properties, apply, 'Style' ); } function toogleProperties( element, properties, apply, propertyType ) { ... }

or actually, you may just stay with toogleProperties (skipping toogleAttributes and toggleStyles) and call it with proper params in the first place.

One more thing, property param, should be properties as it is an object containing multiple properties.

f1ames · 2018-02-21T15:50:24Z

tests/plugins/widget/customstyleinlineandattribute.js

+	};
+
+	bender.test( {
+		'test apply remove inline style': function() {


I would add few tests here checking how it behaves when widget already has some attributes, styles defined. You may check if different attributes are preserved and same ones are replaced, if styles are merged with existing once and same values replaced, etc.

f1ames · 2018-02-22T12:51:40Z

After F2F talk, we came up with two conclusions:

Style object (it may contain styles and attributes) should be applied to widget wrapper (the outermost wrapper element). Main reason for this is that styles applied to widget element may not work as expected (take float for example, which will reposition element inside widget instead of the whole widget).
When downcasting the widget all styles should be applied to widget element (as wrapper is removed). In order to do so, one idea is to store the merged definition of all applied styles so it can be easily applied. On upcast, definition should be created based on existing styles/attributes. When merging styles definitions, keep in mind there might be conflicts so styles applied later have priority as they overwrites what was before in case of conflicts.

f1ames · 2018-02-22T16:45:26Z

As there were some doubts if we should store all applied styles as a one, merged object or just as an array of all applied styles, after looking how CKEDITOR.style works, going with one object with all applied/removed styles properties merged seems to be a proper approach.

First of all it works the same way as just applying styles to an element. If a style contains property which is already present in the element, the new value overwrites old one of existing property (same will happen when merging newly applied style object to cached style definition).

Two short scenarios just to illustrate how it should work (again based on CKEDITOR.style):

Example 1

var style1 = new CKEDITOR.style( { element: 'p', styles: { color: 'red' } } ),
    style2 = new CKEDITOR.style( { element: 'p', styles: { color: 'blue' } } )
    style3 = new CKEDITOR.style( { element: 'p', styles: { color: 'blue', 'font-size': '10px' } } );

// Here we assume that the styles were applied (in order of declaration)
// to current selection/element so we can operate on it.
// It has the following styles: 'style="color: blue; font-size: 10px;"'
editor.removeStyle( s1 ); // Does nothing
editor.removeStyle( s2 ); // Would remove "color: blue" leaving 'style="font-size: 10px"'
editor.removeStyle( s3 ); // Would remove whole styles

Of course if we run editor.removeStyle( s2 ); and then editor.removeStyle( s3 );, the second one will have no effect as the style (as a whole) is not present on the selection/element.

Example 2

Having styles with some common property, like:

var style1 = new CKEDITOR.style( { element: 'p', styles: { color: 'blue', 'font-size': '10px' } } ),
    style2 = new CKEDITOR.style( { element: 'p', styles: { color: 'blue', float: 'left' } } );

It is possible to only remove one style:

editor.removeStyle( s1 ); // Leaves 'style="float: left"'.
editor.removeStyle( s2 ); // Does nothing.

// Or in the reverse order.
editor.removeStyle( s2 ); // Leaves 'style="font-size: 10px"'.
editor.removeStyle( s1 ); // Does nothing.

For quick experiments with styles (especially removing) you may use this codpen.

One more thing, after F2F talk with @engineering-this, we decide to not reuse CKEDITOR.style.checkElementMatch when checking if style is applied to widget, but go with simpler approch whcih will just compare given style to widget current style cache (which holds current "styling state").

engineering-this · 2018-02-23T15:32:32Z

Currently I'm working on getting styles from widget.element and passing them to widget.wrapper on upcast and reverse on downcast. There is a case when the widget is generated from img and then its linked. When downcasting widget.element is now a, so I can't pass styles back to widget that way, because our source output will be different than input. Also when such widget would be unlinked it would lose all its styles because of that.

f1ames · 2018-02-23T17:08:33Z

The problem @engineering-this mentioned in #1681 (comment) is quite tricky. We were thinking about storing somehow (in e.g. styleDefiniton.element) element name (so when it gets linked we still now what was the element), but it won't work in some cases as you can have linked widget on start or paste and then you don't have any element name stored from earlier.

I see two solutions:

Find a reliable way to determine which element is a proper widget.element which should have styles applied (skipping a is not an option ofc as widget can have it in its structure by default).
Use styles in a way that utilizes styleDefinition.element so even if widget gets linked (or its HTML changes somehow) styles will be applied to the element pointed by styles (btw. it looks like it should work like this as styleDefiniton.element plays important role in other places where styles are used).

engineering-this · 2018-03-02T11:40:09Z

plugins/widget/plugin.js

@@ -1055,9 +1054,10 @@
 		 * {@link CKEDITOR.style#checkActive} method instead of using this method directly,
 		 * because unlike style's method, this one does not perform any checks.


Not sure if this part should stay or not. Now this method checks much more than just classes.

engineering-this · 2018-03-02T11:43:42Z

plugins/widget/plugin.js

+			if ( attributes ) {
+				for ( item in attributes ) {
+					if ( item !== 'class' && item !== 'style' ) {
+						if ( !( item in styleDefinition.attributes ) ) {


Could make two if's as one, but it is easier to read this way.

I wonder if it wouldn't be more readable to create helper function and make it one if.

engineering-this · 2018-03-05T10:30:13Z

plugins/widget/plugin.js

+			// If there are no styles remove it to make sure we don't have `styles=''` in our output.
+			delete styleDefinition.styles;
+		}
+		wrapper.attributes[ 'data-style-definition' ] = JSON.stringify( styleDefinition );


This might be considered as 'ugly' solution, but I couldn't find better way to store widget.styleDefinition before widget is initialized.

At least it should be internal CKE attribute, [data**-cke**-style-definition].

What's more, information about partial styles (classes) are inside widget's data. Maybe we should reuse this mechanism and include all styles there.

engineering-this · 2018-03-05T11:27:37Z

This could possibly close #1566 too. After review I'd like other devs to discuss if we need any further integration between widget and CKEDITOR.style.

Comandeer

Additionally there are no tests for integration with Undo, which does not work correctly (part of styles are lost after undoing/redoing).

Comandeer · 2018-03-14T14:55:45Z

plugins/widget/plugin.js

+			if ( attributes ) {
+				for ( item in attributes ) {
+					if ( item !== 'class' && item !== 'style' ) {
+						if ( !( item in styleDefinition.attributes ) ) {


I wonder if it wouldn't be more readable to create helper function and make it one if.

Comandeer · 2018-03-14T15:11:38Z

plugins/widget/plugin.js

 	// @param {CKEDITOR.style} style Custom widget style.
-	// @param {Boolean} apply Whether to apply or remove style.
+	// @param {Boolean} whenever to apply or remove style.


These API docs are incorrect/incomplete.

Comandeer · 2018-03-14T15:23:40Z

tests/plugins/widget/customstyleinlineandattribute.js

+	}
+
+	// function setTest( bot, testStyles, testAttributes, config ) {
+	function setTest( bot, config ) {


Maybe it's just me, but I'll rewrite this function to replace ifs with action dictionary:

function setTest() { var tests = { apply: function() { […] }, remove: function() { […] } }; while ( index < config.length ) { var conf = config[ index ], action = CKEDITOR.tools.objectKeys( conf )[ 0 ]; tests[ action ]( conf[ action ] ); } }

More complex, but IMO also more readable.

Comandeer · 2018-03-14T15:54:49Z

plugins/widget/plugin.js

+					var styleDefinition = JSON.parse( wrapper.$.getAttribute( 'data-style-definition' ) );
+					delete wrapper.$.attributes[ 'data-style-definition' ];
+
+					wrapper.removeAttribute( 'data-style-definition' );


Why there are doubled removal of this attribute?

Comandeer · 2018-03-14T15:55:13Z

plugins/widget/plugin.js

@@ -830,7 +834,7 @@
 	 * @param [startupData] Initial widget data. This data object will overwrite the default data and
 	 * the data loaded from the DOM.
 	 */
-	function Widget( widgetsRepo, id, element, widgetDef, startupData ) {
+	function Widget( widgetsRepo, id, element, widgetDef, startupData, styleDefinition ) {


There is no API docs for the last parameter.

Comandeer · 2018-03-14T16:05:06Z

plugins/widget/plugin.js

+			 *
+			 * @readonly
+			 * @since: 4.10.0
+			 * @property {CKEDITOR.plugins.widget.styleDefinition}


There is no such type.

Comandeer · 2018-03-14T16:18:47Z

plugins/widget/plugin.js

+			// If there are no styles remove it to make sure we don't have `styles=''` in our output.
+			delete styleDefinition.styles;
+		}
+		wrapper.attributes[ 'data-style-definition' ] = JSON.stringify( styleDefinition );


At least it should be internal CKE attribute, [data**-cke**-style-definition].

What's more, information about partial styles (classes) are inside widget's data. Maybe we should reuse this mechanism and include all styles there.

Comandeer · 2018-03-14T16:48:27Z

plugins/image2/plugin.js

@@ -932,6 +938,10 @@

 				// Image can be wrapped in link <a><img/></a>.
 				image = el.getFirst( 'img' ) || el.getFirst( 'a' ).getFirst( 'img' );
+
+				el.lockedStyle = el.lockedStyle || {};


Shouldn't it be bound to the widget, not the element itself?

At this point widget hasn't been initialized and there is no instance to which I could bind this attribute.

But upcast method takes data as a second parameter. These locked styles could be treated as a part of widget's data.

Comandeer

Additionally there are no unit tests for undo integration.

Comandeer · 2018-03-20T14:01:40Z

plugins/widget/plugin.js

+				if ( CKEDITOR.tools.objectKeys( styles ).length ) {
+					wrapper.setStyles( styleDefinition.styles );
+				} else {
+					delete styleDefinition.styles


Missing semicolon.

Comandeer · 2018-03-20T14:42:07Z

plugins/widget/plugin.js

+		var classes = widget.styleDefinition ? widget.styleDefinition.attributes[ 'class' ] : undefined;
+			classes = classes && classes.split( /\s+/ ) ;
+		if ( classes ) {
+			if ( !!remove ) {


This logic seems a little bit reversed.

Comandeer · 2018-03-20T15:14:29Z

plugins/image2/plugin.js

@@ -932,6 +938,10 @@

 				// Image can be wrapped in link <a><img/></a>.
 				image = el.getFirst( 'img' ) || el.getFirst( 'a' ).getFirst( 'img' );
+
+				el.lockedStyle = el.lockedStyle || {};


But upcast method takes data as a second parameter. These locked styles could be treated as a part of widget's data.

Comandeer · 2018-03-20T15:30:17Z

plugins/widget/plugin.js


+				var wrapper = widgetsRepo.wrapElement( toBeWrapped[ 0 ], toBeWrapped[ 1 ] );
+				handleStylesOnUpcast( toBeWrapped[ 0 ], wrapper );


I wonder if it shouldn't be moved into upcast iterator, constructed in createUpcastIterator. Thanks to that we'd be able to replace some magic properties (e.g. lockedStyles) with reusing widget's data.

I've moved lockedStyle to data and accessed it from here.

But now parsing data is done manually. If we move handleStylesOnUpcast to mentioned iterator, widget system would parse it for us

I can't access widgets wrapper from there, because its not yet created. I could split this logic into two steps. First in iterator create styleDefinition object and save it in data. Then inside initOn add styles to actual html elements.

Comandeer

I wonder if styleDefinition also couldn't be moved to widget data, similar to lockedStyle, and whole style handling logic would then be placed inside initial data handler. WDYT, @engineering-this and @mlewand?

Comandeer · 2018-03-22T15:57:29Z

plugins/widget/plugin.js

+		var classes = widget.styleDefinition ? widget.styleDefinition.attributes[ 'class' ] : undefined;
+		if ( classes ) {
+			classes = classes && classes.split( /\s+/ ) ;
+			if ( !!remove ) {


That logic is still reversed…

engineering-this · 2018-03-23T15:54:23Z

@Comandeer @mlewand

I wonder if styleDefinition also couldn't be moved to widget data, similar to lockedStyle, and whole style handling logic would then be placed inside initial data handler. WDYT

Doing so results in many tests failing
http://localhost:1030/#tests/is:unit,path:/tests/plugins/widget,path:/tests/plugins/image2,path:/tests/plugins/imagebase,path:/tests/plugins/easyimage
I'd rather avoid changing that. Currently I'm removing this attribute right when I don't use it. Putting it in data would affect tests that have hardcoded expected html output.

Comandeer · 2018-04-09T10:02:19Z

plugins/widget/plugin.js

+
+			if ( attributes ) {
+				for ( item in attributes ) {
+					if ( isNotInAttributes( item, styleDefinition.attributes ) ) {


It seems a little bit odd to have negative function. IMO !isInAttributes is more readable.

What's more: why only attrbitues have appropriate helper function and every other styles have just inline conditional?

Comandeer · 2018-04-09T11:26:45Z

plugins/widget/plugin.js

-		 * Removes the specified style from the widget. It is highly recommended to use the
-		 * {@link CKEDITOR.editor#removeStyle} or {@link CKEDITOR.style#remove} methods instead of
-		 * using this method directly, because unlike editor's and style's methods, this one
-		 * does not perform any checks.


IMO it's better to leave this comment as this method still does not perform any checks.

Comandeer · 2018-04-09T11:27:15Z

plugins/widget/plugin.js

@@ -1026,20 +1026,19 @@
 		},

 		/**
-		 * Applies the specified style to the widget. It is highly recommended to use the


I second this: info about recommended methods should stay.

Comandeer · 2018-04-09T11:59:04Z

plugins/widget/plugin.js

@@ -1873,6 +1914,10 @@
 	// REPOSITORY helpers -----------------------------------------------------
 	//

+	function isNotInAttributes( item, attributes ) {
+		return item !== 'class' && item !== 'style' && !( item in attributes );


As I said, it's rather odd to have negative function.

Comandeer · 2018-04-09T12:11:51Z

plugins/widget/plugin.js

+						} );
+						classes = classes.join( ' ' );
+						if ( classes ) {
+							styleDefinition.attributes[ 'class' ] = classes;


Isn't it a duplication of the existing classes logic (storing them inside widget's data)?

It is, but as styleDefinition is meant to hold all widgets styles it should contain also classes. While we can't get rid of data.classes it will be duplicated...

Comandeer · 2018-04-09T12:24:43Z

plugins/widget/plugin.js


+				var wrapper = widgetsRepo.wrapElement( toBeWrapped[ 0 ], toBeWrapped[ 1 ] );
+				handleStylesOnUpcast( toBeWrapped[ 0 ], wrapper );


But now parsing data is done manually. If we move handleStylesOnUpcast to mentioned iterator, widget system would parse it for us

Comandeer · 2018-04-09T12:27:50Z

plugins/widget/plugin.js

 					delete updatedClasses[ cl ];
 					changed = 1;
 				}
 			}
+			if ( changed ) {
+				widget.setData( 'classes', updatedClasses );


Is it really needed now? We already store such information in styleDefinition.

I mean: we have now two sources of truth – our new styleDefinition object and legacy classes data in widget's data.

Getting rid of data.classes wouldn't break users custom plugins that extend widget? I'm not sure how to approach this.

I'm quite sure that this data is used only internally by our plugin to know, which styles are applied to the widget. I don't even see any info about data.classes in our docs. @mlewand, am I right or I'm missing something?

https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_plugins_widget.html#method-setData

I've found only this references to data.classes.

@Comandeer @engineering-this We can't get rid of data.classes it has been already exposed. Among all it's used by the image2 plugin.

Community had already a good time on picking it up, and relying on that. Because of that we should keep it for compatibility reasons.

Comandeer · 2018-04-09T13:13:50Z

plugins/widget/plugin.js

+	// @param {CKEDITOR.plugins.widget} Widget instance.
+	// @param {Object} Pairs of inline styles or attributes.
+	// @param {Boolean} whenever to apply or remove style.
+	function toggleStyleAttribute( widget, properties, attribute, apply ) {


Parameters names and use of this function is rather unclear. toggleStyleAttribute should… well, toggle [style] attribute. But it seems that this function is used to apply/remove attributes/styles.

Comandeer

Easy Image has the same issue as image2: data.classes in newly created widget does not contain .easyimage class, yet styleDefinition – does. Maybe such behavior is hardcoded into widget plugin itself?
This situation exists, so it means that we don't have tests for synchronisation between data.classes and styleDefinition :P
The fact that captioned image inserted via image2 plugin is aligned couldn't be derived from inspecting styleDefinition. It does not contain any information about it. Yet text-align: <align> is added to the widget wrapper and data.align is set. I'm not sure how to deal with this situation, because in getData such image is transformed into:

<div style="text-align:center">
<figure class="image" style="display:inline-block"><img alt="Comandeer" height="96" src="https://www.comandeer.pl/images/custom/comandeer.jpg" width="96" />
<figcaption>Podpis</figcaption>
</figure>
</div>

Therefore image is not aligned in output HTML. Yet I feel it still a mismatch between old style system and new one.

What's even more interesting, for aligned (not captioned) inline image2, styleDefinition contains styles for p element. The output HTML for this widget is:

<p style="text-align:center"><img alt="Comandeer" height="96" src="https://www.comandeer.pl/images/custom/comandeer.jpg" width="96" /></p>

What's more interesting, styleableElements for this widget are img figure, which makes such styleDefinition even more inexplicable.

If it's not enough, after switching to source mode and back to WYSIWYG one, styleDefiniton still contains p element, but without any attributes and styles.
I even wonder if we could make styleDefiniton a dictionary of style definitions, with styles dedicated to elements (parts?) of the widget and styles dedicated for widget's wrapper. In case of aligned image2 it could look like:

{
	wrapper: {
		styles: {
			'text-align': 'center'
		}
	},

	figure: {
		styles: {
			'display': 'inline-block'
		}
	}
}

It would eliminate styleDefinition.lockedStyle and carry far more information about actual styles. Downside: we'd need to rewrite nearly everything we already have…

There is no manual test for undo integration.
We could also add styleDefinition inspector to tests, because inspecting it in browser's console is a little bit troublesome.

Comandeer · 2018-04-23T13:20:57Z

plugins/widget/plugin.js

+			styleDefinition.styles = CKEDITOR.tools.parseCssText( style );
+		} else {
+			// If there are no styles remove it to make sure we don't have `styles=''` in our output.
+			// delete styleDefinition.styles;


Some dead code here.

Oh, I cheated myself here. I was looking for this line in my code and couldn't find it... It took me a minute to remember. I fixed this and amended, but didn't push 😁

engineering-this · 2018-04-24T08:10:00Z

I even wonder if we could make styleDefiniton a dictionary of style definitions, with styles dedicated to elements (parts?) of the widget and styles dedicated for widget's wrapper. In case of aligned image2 it could look like:

I though about it, but then what in case of two elements with same html tag in widget. For that case I am thinking about something like that:

wrapper: {
	name: 'div',
	styleDefinition: {},
	0: {
		name: 'p',
		styleDefinition: {}
	},
	1: {
		name: 'p',
		styleDefinition: {},
		0: { some other nested child },
		1: { some other nested child },
	}
}

But that looks way too over complicated, and we are basically creating some kind copy of DOM tree.
@mlewand WDYT?

Changed applystyle tests for widget and easyimage, so there is visible output presenting current data and styleDefinition of focused widget.

engineering-this · 2018-04-25T08:13:57Z

I've updated manual test and added new test. So @mlewand you can take a look on this. In the meantime I will fix this one:

Easy Image has the same issue as image2: data.classes in newly created widget does not contain .easyimage class, yet styleDefinition – does. Maybe such behavior is hardcoded into widget plugin itself?

awasson · 2020-08-04T17:10:33Z

I see that this issue has been frozen.

Does that mean that the issue of image tags not being able to take classes from the StylesCombo because IMG becomes IMAGE in the editor has been resolved? It has been an issue with the CKEditor that is bundled with Drupal 8 from day one and I'm keen about resolving it.

Cheers,
Andrew

Dumluregn · 2020-08-05T07:41:45Z

Hello!

Unfortunately, the issue itself is still valid (#1674). We've just frozen this PR as the work done here wasn't finished two years ago and our codebase changed a lot since then, so it would take a lot of time to just rebase this PR and resolve all the conflicts.

awasson · 2020-08-06T16:56:01Z

Hello @Dumluregn,

Thanks for the information. I've been following the issue from the point of view coming from Drupal 8 where CKEditor is bundled. There is an issue in the Drupal Issue Queue (2642808) that goes back to 2015 and it has stalled in anticipation of a resolution from the CKEditor side.

On Drupal we can preprocess the stylesSet array before rendering the editor so we are able to loop through the img classes and create a duplicate set for the image widget, formatted in such a way that CKEditor can work with them. The following example of this takes a class for an img tag and duplicates it for an image widget.

Original stylesSet

"stylesSet":[ { "name":"This Class", "element":"img", "attributes":{ "class":"this-class" } } ]

StylesSet rendered in CKEditor.
"stylesSet":[ { "name":"This Class", "element":"img", "attributes":{ "class":"this-class" } }, { "name":"This Class Widget", "type":"widget", "widget":"image", "attributes":{ "class":"this-class" } } ]

I'm not sure if this will help with the native CKEditor but this works within Drupal.

engineering-this commented Feb 15, 2018

View reviewed changes

mlewand requested a review from f1ames February 19, 2018 11:42

f1ames suggested changes Feb 19, 2018

View reviewed changes

engineering-this requested a review from f1ames February 20, 2018 16:50

f1ames suggested changes Feb 21, 2018

View reviewed changes

engineering-this force-pushed the t/1674 branch from 614c6b7 to 829678b Compare March 2, 2018 11:37

engineering-this commented Mar 2, 2018

View reviewed changes

engineering-this force-pushed the t/1674 branch 3 times, most recently from 84bf62d to c817380 Compare March 5, 2018 10:24

engineering-this commented Mar 5, 2018

View reviewed changes

engineering-this force-pushed the t/1674 branch from c817380 to d4e6407 Compare March 5, 2018 10:35

Comandeer requested changes Mar 14, 2018

View reviewed changes

engineering-this force-pushed the t/1674 branch 2 times, most recently from 0b87074 to 9442085 Compare March 19, 2018 16:08

engineering-this requested a review from Comandeer March 19, 2018 16:31

Comandeer requested changes Mar 20, 2018

View reviewed changes

engineering-this requested a review from Comandeer March 22, 2018 12:28

Comandeer requested changes Mar 22, 2018

View reviewed changes

engineering-this force-pushed the t/1674 branch from 22306a8 to ddb8bad Compare March 23, 2018 16:00

engineering-this requested a review from Comandeer March 23, 2018 16:01

Comandeer requested changes Apr 9, 2018

View reviewed changes

engineering-this added 9 commits April 20, 2018 14:59

Moved lockedStyle from element to data

a219d81

Refactored test, added test case for undo integration

480d9d6

Fixed adding class name to widget.styleDefinition.attributes

c7d4dda

Fixed typo

edaa942

Added changelog entry

c8e18bc

Refactoring code and improving docs

e36432e

Refactoring

70b8eac

Fixed test after changes

d37905e

Fixed widget styles integration with mathjax, copypaste and image2

df5389f

engineering-this force-pushed the t/1674 branch 3 times, most recently from 505fb38 to f5b6bde Compare April 20, 2018 13:04

Added test case for widget styles on easyimage

0dce367

engineering-this requested a review from Comandeer April 20, 2018 13:10

Comandeer requested changes Apr 23, 2018

View reviewed changes

engineering-this force-pushed the t/1674 branch from f5b6bde to 0dce367 Compare April 24, 2018 14:04

engineering-this added 6 commits April 25, 2018 09:09

Added style test with image2 plugin

83651a9

Changed applystyle tests for widget and easyimage, so there is visible output presenting current data and styleDefinition of focused widget.

Removed undostyle test, added undo plugin to all applystyle tests

d049a31

Fixed error when adding styles after undo

b7f3b1c

Simplified applystyle tests descriptions

ca4b2c4

Added docs entry to test helper

ebd1370

Added missing undo/redo buttons

3cb054e

Fixed styledef classes desynchronized from data classes in easyimage

276567a

f1ames closed this Mar 20, 2020

f1ames added the pr:frozen ❄ The pull request on which work was suspended due to various reasons. label Mar 20, 2020


		New Features:

		* [#1674](https://github.com/ckeditor/ckeditor-dev/issues/1674): The [Widget](https://ckeditor.com/cke4/addon/widget) plugin accepts inline styles and HTML attributes from [`CKEDITOR.style.customHandlers.widget`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_style_customHandlers_widget.html).

		@@ -1055,9 +1054,10 @@
		* {@link CKEDITOR.style#checkActive} method instead of using this method directly,
		* because unlike style's method, this one does not perform any checks.


		var wrapper = widgetsRepo.wrapElement( toBeWrapped[ 0 ], toBeWrapped[ 1 ] );
		handleStylesOnUpcast( toBeWrapped[ 0 ], wrapper );

Adding inline styles and attributes to widget with applyStyle #1681

Adding inline styles and attributes to widget with applyStyle #1681

Conversation

engineering-this commented Feb 15, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

engineering-this commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames commented Feb 22, 2018

f1ames commented Feb 22, 2018

Example 1

Example 2

engineering-this commented Feb 23, 2018 • edited Loading

f1ames commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineering-this commented Mar 5, 2018

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineering-this commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineering-this commented Feb 23, 2018 •

edited

Loading