Opened 7 years ago

Closed 7 years ago

#5418 closed Bug (fixed)

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

Reported by: Satya Minnekanti Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.4
Component: General Version: 3.2
Keywords: IBM Cc: Damian, joek, pomu@…

Description

To reproduce the Defect

  1. Open Ajax Sample
  1. Copy and Paste a Table from Word in to the Editor.
  1. 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.

Attachments (9)

5418_1.patch (5.9 KB) - added by Tobiasz Cudnik 7 years ago.
5418_2.patch (7.5 KB) - added by Tobiasz Cudnik 7 years ago.
5418_3.patch (8.6 KB) - added by Tobiasz Cudnik 7 years ago.
5418_4.patch (8.5 KB) - added by Tobiasz Cudnik 7 years ago.
5418_5.patch (6.5 KB) - added by Frederico Caldeira Knabben 7 years ago.
5418_6.patch (11.6 KB) - added by Frederico Caldeira Knabben 7 years ago.
5418_7.patch (14.6 KB) - added by Frederico Caldeira Knabben 7 years ago.
5418_8.patch (14.7 KB) - added by Frederico Caldeira Knabben 7 years ago.
5418_9.patch (15.2 KB) - added by Frederico Caldeira Knabben 7 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.3CKEditor 3.4

comment:2 Changed 7 years ago by pomu0325

Cc: pomu@… added

comment:3 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik

comment:4 Changed 7 years ago by Tobiasz Cudnik

Keywords: Pending added
Status: newassigned

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.

comment:5 Changed 7 years ago by 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

comment:6 Changed 7 years ago by Garry Yao

Keywords: Confirmed added; Pending removed

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

comment:7 Changed 7 years ago by 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>

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5418_1.patch added

comment:8 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added

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.

comment:9 Changed 7 years ago by Garry Yao

Keywords: Review- added; Review? removed

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

comment:10 Changed 7 years ago by 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.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5418_2.patch added

comment:11 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

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.

comment:12 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

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)

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5418_3.patch added

comment:13 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

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.

comment:14 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

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?

comment:15 in reply to:  14 Changed 7 years ago by 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.

comment:16 Changed 7 years ago by Frederico Caldeira Knabben

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

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5418_4.patch added

comment:17 Changed 7 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

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.

comment:18 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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.

comment:19 Changed 7 years ago by Frederico Caldeira Knabben

Owner: changed from Tobiasz Cudnik to Frederico Caldeira Knabben
Status: review_failedassigned

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 5418_5.patch added

comment:20 Changed 7 years ago by Frederico Caldeira Knabben

Status: assignedreview

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.

comment:21 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

Please attach the new plugin as well.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 5418_6.patch added

comment:22 Changed 7 years ago by Frederico Caldeira Knabben

Status: review_failedreview

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

comment:23 Changed 7 years ago by Sa'ar Zac Elias

Keywords: Confirmed removed
Status: reviewreview_failed
  • 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.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 5418_7.patch added

comment:24 in reply to:  23 Changed 7 years ago by Frederico Caldeira Knabben

Status: review_failedreview

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.

comment:25 Changed 7 years ago by Frederico Caldeira Knabben

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

comment:26 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

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.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 5418_8.patch added

comment:27 Changed 7 years ago by Frederico Caldeira Knabben

Status: review_failedreview

comment:28 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

L127-128 are still unprotected.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 5418_9.patch added

comment:29 Changed 7 years ago by Frederico Caldeira Knabben

Status: review_failedreview

comment:30 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

comment:31 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [5753].

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy