Ticket #6462 (closed Bug: fixed)

Opened 4 years ago

Last modified 3 years ago

Table dialog does not allow percentage values for cellpadding

Reported by: JoeK Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.1
Component: UI : Dialogs Version: 3.0
Keywords: IBM Cc: damo, satya, jamcunni@…

Description (last modified by garry.yao) (diff)

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

6462.patch (9.3 KB) - added by garry.yao 4 years ago.
6462_2.patch (9.9 KB) - added by garry.yao 4 years ago.
6462_3.patch (20.1 KB) - added by garry.yao 4 years ago.
6462_4.patch (21.0 KB) - added by garry.yao 4 years ago.

Change History

comment:1 Changed 4 years ago by garry.yao

  • Status changed from new to confirmed
  • Version changed from 3.5 (SVN - 3.5.x) to 3.0
  • Component changed from General to UI : Dialogs
  • Description modified (diff)

Flash dialog is also affected.

Changed 4 years ago by garry.yao

comment:2 Changed 4 years ago by garry.yao

  • Status changed from confirmed to review
  • Owner set to garry.yao

comment:3 Changed 4 years ago by james c

  • Cc damo, satya, jamcunni@… added; damo satya removed

comment:4 Changed 4 years ago by Saare

  • Status changed from review to review_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 4 years ago by garry.yao

comment:5 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

comment:6 Changed 4 years ago by Saare

  • Status changed from review to review_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 4 years ago by garry.yao

comment:7 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

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

  • Status changed from review to review_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 follow-up: ↓ 10 Changed 4 years ago by fredck

I would opt to not allow % values for height.

Changed 4 years ago by garry.yao

comment:10 in reply to: ↑ 9 Changed 4 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 4 years ago by garry.yao

  • Status changed from review_failed to review

comment:12 Changed 4 years ago by Saare

  • Status changed from review to review_passed

comment:13 Changed 4 years ago by fredck

  • Milestone set to CKEditor 3.6.1

comment:14 follow-up: ↓ 15 Changed 4 years ago by garry.yao

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

Fixed with [6979].

comment:15 in reply to: ↑ 14 Changed 4 years ago by satya

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

  • Status changed from closed to reopened
  • Resolution fixed deleted

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 follow-up: ↓ 18 Changed 4 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 4 years ago by fredck

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 3 years ago by garry.yao

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

Part of the changes are reverted with [6987].

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