We can't change Border size,Cell Spacing & Cell Padding for Pasted Tables from Word

[Satya Minnekanti]

To reproduce the Defect

1. Open Ajax Sample

2. Copy and Paste a Table from Word in to the Editor.

3. Go to Table Properties and Change Table Border Size to 10 and Cell Spacing & Cell Padding also to 10.

Expected Result:

Table Border,Cell Spacing and Cell Padding between the cells should increase according to the Values we have set on Table Properties dialog.

Actual Result:

Table Border,Cell Spacing and Cell Padding between Table Cells remains same.But the Vales for Border, Cell Spacing & Cell Padding are set properly in HTML Source.

[Tobiasz Cudnik]

Could you please attach the document which for sure creates this issue ? I've tested on my own simple example and i can't reproduce it on any IE version.

[Satya Minnekanti]

we could reproduce this issue when you set pastefromWordRemoveFontStyles=false and pastefromWordRemoveStyles=false in the config.

Its a configuration option,The nightly editor build removes all styles

CKEditor use the table 'border' attribute which should not be used anymore. The style attribute should be used that causes a conflict. We really should have fields in the Table dialog that allows you to change the styles

border-width, border-color, border-type

[Garry Yao]

Let's just add our Advanced tab page in table dialog, borrow from Link dialog.

[Alfonso Martínez de Lizarrondo]

It's important to remember that setting the attribute border=1 in a <table> also sets the border for its contents, but using the style "border:1px solid black" only shows the outer border of the table.

<table cellpadding="1" cellspacing="1" style="border: 1px solid black; width: 200px;">
	<tbody>
		<tr>
			<td>
				&nbsp;</td>
			<td>
				&nbsp;</td>
		</tr>
		<tr>
			<td>
				&nbsp;</td>
			<td>
				&nbsp;</td>
		</tr>
		<tr>
			<td>
				&nbsp;</td>
			<td>
				&nbsp;</td>
		</tr>
	</tbody>
</table>
<table border="1" cellpadding="1" cellspacing="1" style="width: 200px;">
	<tbody>
		<tr>
			<td>
				&nbsp;</td>
			<td>
				&nbsp;</td>
		</tr>
		<tr>
			<td>
				&nbsp;</td>
			<td>
				&nbsp;</td>
		</tr>
		<tr>
			<td>
				&nbsp;</td>
			<td>
				&nbsp;</td>
		</tr>
	</tbody>
</table>

[Tobiasz Cudnik]

Considering Alfonso's comment we can't simply replace present border field with 3 new ones defining borders by CSS values. I think both ways should be doable and i'm attaching a patch which uses Advanced tab from Link dialog. It also honors showborders plugin.

[Garry Yao]

Please port the ​internal commit for 'style' field as well.

[Alfonso Martínez de Lizarrondo]

Be careful about what properties are added to the dialog because most of the ones in the patch aren't meaningful for a table.

I would add id, class and style, but also I think that providing a combo like the Div dialog to select a predefined "Style" can be very useful.

[Tobiasz Cudnik]

New patch provides:

• synchronization between fields (style, width, height)
• only 3 new fields: id, class, style (by text)

While trying to add predefined "Style" field i've faced many issues in this case because of values synchronization. We would need some strong datastore for decent merging while not affecting edited document until dialog's submit.

[Alfonso Martínez de Lizarrondo]

There are still some code copied from the link dialog like several options in advParamsMap , the var styles = {} ; and its comment.

The code to remove the cke_show_border class can be a simple replace, and it should be safe to assume that it isn't already present when committing the changes, so it can be appended at the end.

But I don't think that this patch really addresses the original report, so that part should be clarified to avoid confusion.
This patch provides the long requested id, class and style editing to the table dialog and that's good.
I guess that it will need to also provide the style editing to the cell dialog, and after that, what should we do if the table has initially the border set as a style and then the user tries to set the border size attribute?

Using only css to generate the formatting should be an option and not the only way to do it because it's doesn't work correctly in tables (and there are already too many complains about the use of styles to output the dimensions for images)

[Tobiasz Cudnik]

Third patch:

• adds language direction switching
• does some code cleanup like Alfonso suggested
• moves whole advanced tab code into new plugin
• fixes issue with removing whole content from synchronized field

This patch covers also #5912, where setting DIR attribute on a table is needed.

I don't think that cell related behaviors are related to this ticket. The scope of this one which is changing style attribute value is covered.

[Alfonso Martínez de Lizarrondo]

The patch doesn't work, the language entries fail to load because the "editor" variable isn't defined there.

For the advCSSClasses element is adding an extra hbox (line 239); The widths of the main "rows" are 45%, 45% instead of 50% that seems more logical for just two children.

To simplify a little the code you can define the child arrays as variables and then use them instead of using the calling scope:

var contents = [];
		var result =
		{
			id : 'advanced',
...
			elements :
				[
					{
...						children : contents
					}
				]
		};

...

contents.push({
				type : 'hbox',
				widths : [ '45%', '45%' ],
				children : []
			})

and also that children could be defined as another variable, so instead of

result.elements[ 0 ].children[ position ].children.push

it's just

row2Children.push

I've tried to remove the

          else if ( attrName == 'class' ) 
                   value += ' cke_show_border'; 

in line 86 because it doesn't take into account if the showborders plugin is enabled or the border of the table itself and it seems that the hooks created by the showborders plugin are enough to take care of that class. Can you verify if it's really needed?

I think that the idea about splitting the advanced tab into its own plugin is to allow this plugin to be easily reused by other dialogs. Right?
That looks like a good idea, although it might need some adjustments that can be handled at that time when we test it correctly (like moving language entries from .link to .common, as long as there are no problems due to the reuse of entries)

The problem right now is that it's included into the core, so it means a little penalty. Fred should give his OK for this change.

Lastly, I don't understand the need of

		// Check if we've already read element value.
		if ( !this._elementStyleRead && element )
		{
			value = element.hasAttribute( attrName ) && element.getAttribute( attrName );
			this._elementStyleRead = 1;
		}

It seems that this is handled while reading value

	// Try to get existing value first in case of inChange commit.
	var value = this.getValue() || element && element.hasAttribute( attrName ) && element.getAttribute( attrName ) || '';

What's the problem with that case?
If there's a problem, shouldn't that be done for other elements also?

[Frederico Caldeira Knabben]

Replying to alfonsoml:

You did a wonderful review Alfonso ;)

The problem right now is that it's included into the core, so it means a little penalty. Fred should give his OK for this change.

In fact I've recommended creating this new plugin. It may bring some size penalty right now, but hopefully we'll be able to compensate it later, by changing other dialogs to use it. The important now is keeping the code as simple and as small we possible, right because of that. KISS is very important here.

[Frederico Caldeira Knabben]

Btw... let's have proper curly braces in the if blocks in the code.

[Tobiasz Cudnik]

I've implemented Alfonso's suggestion also those one related to code removal (2 pieces).

Also the showborders plugin is checked before adding it's class. There is still a reason for doing it manually because of the way showborders plugin integrated with the dialog. To reproduce, comment out the code, add some class to the dialog class field and hit OK. Then table will miss it, what can be seen using CSS inspector.

[Frederico Caldeira Knabben]

We must definitely avoid making this generic plugin so depending on specific dialog features, like having hardcoded field names. Also, it should be possible to separate the show borders stuff from it.

[Frederico Caldeira Knabben]

With this patch, I've simplified some things:

• Making the dialogadvtab plugin simpler. In fact it should be as simple as possible. It doesn't need to automate everything, but should provide some API tools that can be used by dialog sto make the integration easier.

• It means that part of the code has been moved to the table dialog. That could be some small code duplication for other plugins, but will also help us having less code on core, and more flexibility.

• The showborders plugin now handles all it's stuff.

Be sure to have config.language='en' to test it properly.

[Sa'ar Zac Elias]

Please attach the new plugin as well.

[Frederico Caldeira Knabben]

Oh, TortoiseSVN played with me this time (again!). Here you have it.

[Sa'ar Zac Elias]

• The style field is not updated properly:
  • Add the following to source:

    <table style="z-index: 30; width: 200px">
    	<tbody>
    		<tr>
    			<td>
    				test</td>
    		</tr>
    	</tbody>
    </table>
    

  • Open the table properties dialog. Notice that the style field does not have the z-index property in it.
• The advanced tab can not be removed using the dialogDefinition.
• config.removePlugins = 'dialogadvtab' does not work.

[Frederico Caldeira Knabben]

Replying to Saare:

• The advanced tab can not be removed using the dialogDefinition.

Very good point. That's caused by our bad practice of making everything required on dialogs. We must code them in a way that it's flexible to end developers to customize them with easy.

• config.removePlugins = 'dialogadvtab' does not work.

Here it's a matter of interpretation of what's best. In the previous patch, the dialogadvtab plugin was a requirement for the table plugin. It means that, if you remove the table plugin, the dialogadvtab is also removed automatically.

But, probably people will have move benefit on having an easy way to remove all advanced tabs from all dialogs with a single plugin removal configuration. So I've changed the code to make it work in the way you expected.

[Frederico Caldeira Knabben]

Btw, the patch is now targeted to the 3.4.x branch, because this fix should end up there.

[Sa'ar Zac Elias]

If the 'textHeight' or 'txtWidth' (with the 'cmbWidthType') fields are removed, an error is thrown when the dialog is opened and when changing the 'style' field.

[Sa'ar Zac Elias]

L127-128 are still unprotected.

[Frederico Caldeira Knabben]

Fixed with [5753].