Opened 7 years ago

Closed 6 years ago

#5418 closed Bug (fixed)

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

Reported by: satya Owned by: fredck
Priority: Normal Milestone: CKEditor 3.4
Component: General Version: 3.2
Keywords: IBM Cc: damo, 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 6 years ago.
5418_2.patch (7.5 KB) - added by tobiasz.cudnik 6 years ago.
5418_3.patch (8.6 KB) - added by tobiasz.cudnik 6 years ago.
5418_4.patch (8.5 KB) - added by tobiasz.cudnik 6 years ago.
5418_5.patch (6.5 KB) - added by fredck 6 years ago.
5418_6.patch (11.6 KB) - added by fredck 6 years ago.
5418_7.patch (14.6 KB) - added by fredck 6 years ago.
5418_8.patch (14.7 KB) - added by fredck 6 years ago.
5418_9.patch (15.2 KB) - added by fredck 6 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 7 years ago by fredck

  • Milestone changed from CKEditor 3.3 to CKEditor 3.4

comment:2 Changed 7 years ago by pomu0325

  • Cc pomu@… added

comment:3 Changed 6 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik

comment:4 Changed 6 years ago by tobiasz.cudnik

  • Keywords Pending added
  • Status changed from new to assigned

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 6 years ago by satya

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 6 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 6 years ago by alfonsoml

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 6 years ago by tobiasz.cudnik

comment:8 Changed 6 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 6 years ago by garry.yao

  • Keywords Review- added; Review? removed

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

comment:10 Changed 6 years ago by alfonsoml

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 6 years ago by tobiasz.cudnik

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

  • 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 6 years ago by tobiasz.cudnik

comment:13 Changed 6 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 follow-up: Changed 6 years ago by alfonsoml

  • 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 6 years ago by fredck

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 6 years ago by fredck

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

Changed 6 years ago by tobiasz.cudnik

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

  • Status changed from review to review_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 6 years ago by fredck

  • Owner changed from tobiasz.cudnik to fredck
  • Status changed from review_failed to assigned

Changed 6 years ago by fredck

comment:20 Changed 6 years ago by fredck

  • Status changed from assigned to review

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 6 years ago by Saare

  • Status changed from review to review_failed

Please attach the new plugin as well.

Changed 6 years ago by fredck

comment:22 Changed 6 years ago by fredck

  • Status changed from review_failed to review

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

comment:23 follow-up: Changed 6 years ago by Saare

  • Keywords Confirmed removed
  • Status changed from review to review_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 6 years ago by fredck

comment:24 in reply to: ↑ 23 Changed 6 years ago by fredck

  • Status changed from review_failed to review

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 6 years ago by fredck

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

comment:26 Changed 6 years ago by Saare

  • Status changed from review to review_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 6 years ago by fredck

comment:27 Changed 6 years ago by fredck

  • Status changed from review_failed to review

comment:28 Changed 6 years ago by Saare

  • Status changed from review to review_failed

L127-128 are still unprotected.

Changed 6 years ago by fredck

comment:29 Changed 6 years ago by fredck

  • Status changed from review_failed to review

comment:30 Changed 6 years ago by Saare

  • Status changed from review to review_passed

comment:31 Changed 6 years ago by fredck

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [5753].

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