Opened 14 years ago
Closed 13 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 )
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)
Change History (23)
comment:1 Changed 14 years ago by
| Component: | General → UI : Dialogs |
|---|---|
| Description: | modified (diff) |
| Status: | new → confirmed |
| Version: | 3.5 (SVN - 3.5.x) → 3.0 |
Changed 14 years ago by
| Attachment: | 6462.patch added |
|---|
comment:2 Changed 14 years ago by
| Owner: | set to Garry Yao |
|---|---|
| Status: | confirmed → review |
comment:3 Changed 14 years ago by
| Cc: | Damian Satya Minnekanti jamcunni@… added; damo satya removed |
|---|
comment:4 Changed 13 years ago by
| Status: | review → 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 13 years ago by
| Attachment: | 6462_2.patch added |
|---|
comment:5 Changed 13 years ago by
| Status: | review_failed → review |
|---|
comment:6 Changed 13 years ago by
| Status: | review → 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 13 years ago by
| Attachment: | 6462_3.patch added |
|---|
comment:7 Changed 13 years ago by
| Status: | review_failed → 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 13 years ago by
| Status: | review → 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.
Changed 13 years ago by
| Attachment: | 6462_4.patch added |
|---|
comment:10 Changed 13 years ago by
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 13 years ago by
| Status: | review_failed → review |
|---|
comment:12 Changed 13 years ago by
| Status: | review → review_passed |
|---|
comment:13 Changed 13 years ago by
| Milestone: | → CKEditor 3.6.1 |
|---|
comment:14 follow-up: 15 Changed 13 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | review_passed → closed |
Fixed with [6979].
comment:15 Changed 13 years ago by
comment:16 Changed 13 years ago by
| Resolution: | fixed |
|---|---|
| Status: | closed → reopened |
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 13 years ago by
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 Changed 13 years ago by
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 13 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | reopened → closed |
Part of the changes are reverted with [6987].
Flash dialog is also affected.