Opened 16 years ago
Closed 16 years ago
#3256 closed Bug (fixed)
Justify across table cells incorrect
Reported by: | Garry Yao | Owned by: | Garry Yao |
---|---|---|---|
Priority: | Must have (possibly next milestone) | Milestone: | CKEditor 3.0 |
Component: | Core : Styles | Version: | |
Keywords: | Confirmed Review+ | Cc: |
Description (last modified by )
Reproducing Procedures
- Open the replace by class example page;
- Make the following content with the selection by click and drag over table cells;
<table border="1" cellpadding="1" cellspacing="1" style="width: 200px;"> <tbody> <tr> ^<td> text</td> <td> text</td>^ </tr> </tbody> </table>
- Click on 'Justify Right' to apply alignment.
- Actual Result: Javascript Error occurred.
- Expected Result:
<table> <tbody> <tr> <td style="text-align: right;"> text</td> </tr> <tr> <td style="text-align: right;"> text</td> </tr> </tbody> </table>
Attachments (5)
Change History (18)
comment:1 Changed 16 years ago by
Keywords: | Confirmed added |
---|---|
Owner: | set to Garry Yao |
Status: | new → assigned |
comment:2 Changed 16 years ago by
Component: | General → Core : Styles |
---|---|
Description: | modified (diff) |
Keywords: | Review? added |
Changed 16 years ago by
Attachment: | 3256.patch added |
---|
comment:3 Changed 16 years ago by
Two updates:
- Skip bookmark nodes in the following case which are harmful for the iterating:
<table _moz_resizing="true"> <tbody> <tr> <span _fck_bookmark="1" style="display: none;"> </span> <td> text </td> <span _fck_bookmark="1" style="display: none;"> </span> </tr> </tbody> </table>
- Make the justify style happen on multiple ranges instead of only the first.
comment:4 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
I've tested it over a table with three columns. If I select two rows of it, and then align right, only the cells at the left get aligned and a js error is thrown.
Changed 16 years ago by
Attachment: | 3256_2.patch added |
---|
comment:5 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|
It's again a matter of multiple ranges mangled by bookmark creation process.
Changed 16 years ago by
Attachment: | 3256_3.patch added |
---|
comment:7 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
Maybe a simpler solution could be found, which would avoid making that many changes to the code. It should be possible to fix the ranges when creating the bookmarks, so all necessary changes could go on createBookmarks only.
Changed 16 years ago by
Attachment: | 3256_4.patch added |
---|
comment:8 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|
Use an less obtrusive way to update the affected ranges inspired by Fred.
comment:9 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
The idea and even the implementation is quite good. As discussed with Garry we could just try simplifying it further. We've coded it at four hands and a new patch should come soon.
Changed 16 years ago by
Attachment: | 3256_5.patch added |
---|
comment:10 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|
comment:11 Changed 16 years ago by
Priority: | Normal → High |
---|
comment:12 Changed 16 years ago by
Keywords: | Review+ added; Review? removed |
---|
Please fix the minor style problems in L80 of _source/plugins/justify/plugin.js before committing - binary operators should have spaces around them, and block brackets should occupy their own lines. So it should look like this:
for ( var i = ranges.length - 1 ; i >= 0 ; i-- ) {
Update the Expected Result of this TC.