Opened 7 years ago

Closed 6 years ago

#6462 closed Bug (fixed)

Table dialog does not allow percentage values for cellpadding

Reported by: Joe Kavanagh Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.1
Component: UI : Dialogs Version: 3.0
Keywords: IBM Cc: Damian, Satya Minnekanti, jamcunni@…

Description (last modified by Garry Yao)

1) Open an instance of the editor.

2) Switch to source mode.

3) Paste in the following table HTML markup:

<TABLE cellspacing="20" cellpadding="20%">
<TR> <TD>Data1 <TD>Data2 <TD>Data3
</TABLE>

4) Switch to wysiwyg mode.

5) Right click on the table and choose Table Properties to open the Table dialog.

6) Click the OK button to close the table without making any changes.

A validation message for the cellpadding field is displayed. Only numeric values can be entered. Percentages are valid values for cellpadding. (See http://www.w3.org/TR/html401/struct/tables.html#adef-cellpadding)

Attachments (4)

6462.patch (9.3 KB) - added by Garry Yao 7 years ago.
6462_2.patch (9.9 KB) - added by Garry Yao 6 years ago.
6462_3.patch (20.1 KB) - added by Garry Yao 6 years ago.
6462_4.patch (21.0 KB) - added by Garry Yao 6 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 7 years ago by Garry Yao

Component: GeneralUI : Dialogs
Description: modified (diff)
Status: newconfirmed
Version: 3.5 (SVN - 3.5.x)3.0

Flash dialog is also affected.

Changed 7 years ago by Garry Yao

Attachment: 6462.patch added

comment:2 Changed 7 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

comment:3 Changed 7 years ago by James

Cc: Damian Satya Minnekanti jamcunni@… added; damo satya removed

comment:4 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

The patch should be updated;
By overlooking the code I've found that:

  • CKEDITOR.tools.cssLength could be safely used inside the getValue overload.
  • We could leave the default values as "1" IMO,
  • Similar change could be ported to the iframe dialog, resolving #7114.

Changed 6 years ago by Garry Yao

Attachment: 6462_2.patch added

comment:5 Changed 6 years ago by Garry Yao

Status: review_failedreview

comment:6 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • Actually when dealing with attributes (not style properties), we should only accept integers percentages values. For cellpadding/spacing we could also accept relative values.
  • In IE, setting percetage value for cellpadding does not work.
  • Click on the iframe button. Set width as 100. Hit OK. Note that the image is not displayed correctly.

Changed 6 years ago by Garry Yao

Attachment: 6462_3.patch added

comment:7 Changed 6 years ago by Garry Yao

Status: review_failedreview

Updates targeting Saar's review which distinguishes between HTML length (only pixel,%) and CSS length unit (all)

In IE, setting percetage value for cellpadding does not work.

We're aligning with the specs only, leaving the browsers to decide how it works.

Click on the iframe button. Set width as 100. Hit OK. Note that the image is not displayed correctly.

Now fixed by auto pixeling when necessary.

Additionally, I also propose for obsoleting the length type combo in table dialog to make all dialogs looks more consistent, while I'd like to include also width/height of the image dialog, but considering that it's now heavily coupled with the ratio lock functionality, let's leave it to another ticket instead.

comment:8 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • Since it is now possible to enter percents in the height field of the table dialog, it is sensless to have the "pixels" label there. It should be either removed, or the field should remain as is, accepting only integers.
  • Create an iframe with width=100%, height=20px. Click on it with RMB, open the iframe dialog. Note that the fields appear as integers.

comment:9 Changed 6 years ago by Frederico Caldeira Knabben

I would opt to not allow % values for height.

Changed 6 years ago by Garry Yao

Attachment: 6462_4.patch added

comment:10 in reply to:  9 Changed 6 years ago by Garry Yao

Replying to fredck:

I would opt to not allow % values for height.

Fred told me it's for simplicity that he proposed it as it's not a practical usage for percentage based height in real world, while when considering consistency, it's not a big problem of having it.

comment:11 Changed 6 years ago by Garry Yao

Status: review_failedreview

comment:12 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

comment:13 Changed 6 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.1

comment:14 Changed 6 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [6979].

comment:15 in reply to:  14 Changed 6 years ago by Satya Minnekanti

Replying to garry.yao:

Fixed with [6979].

@garry.yao this is not working in all IE's(6,7,8&9). When we open Table dialog it's showing Cell padding as just 20 insted of 20%. It works in other browsers. Please re open this ticket

comment:16 Changed 6 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: closedreopened

There is no browser nowadays that accepts percent values for cellpadding. If you set "50%", all browsers will accept it as "50", completely ignoring the percent.

In IE the issue is more dramatic. It replaces "50%" with "50" at DOM level, so the percent information is simply lost. That's why the dialog always show an integer only for it. This is easy to check with the following sample:

<html>
<head>
	<script type="text/javascript">
window.onload = function()
{
	alert( document.body.innerHTML );
}
	</script>
</head>
<body>
	<table cellpadding="50%" width="400" border="1"><tr><td>Test</td></tr></table>
</body>
</html> 

I would opt to revert the change and close this ticket as "invalid".

comment:17 Changed 6 years ago by Garry Yao

At this ticket we touches more than just particular field, I'd opt to keep other changes (regard wider length units) while reverting only the cellspacing/cellpadding stuff.

comment:18 in reply to:  17 Changed 6 years ago by Frederico Caldeira Knabben

Replying to garry.yao:

At this ticket we touches more than just particular field, I'd opt to keep other changes (regard wider length units) while reverting only the cellspacing/cellpadding stuff.

True. The fix went outside the ticket boundaries, so it's ok to revert it partially.

comment:19 Changed 6 years ago by Garry Yao

Resolution: fixed
Status: reopenedclosed

Part of the changes are reverted with [6987].

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