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 Garry Yao)

Reproducing Procedures

  1. Open the replace by class example page;
  2. 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>
    
  3. 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)

3256.patch (4.2 KB) - added by Garry Yao 16 years ago.
3256_2.patch (5.2 KB) - added by Garry Yao 16 years ago.
3256_3.patch (8.7 KB) - added by Garry Yao 16 years ago.
3256_4.patch (4.0 KB) - added by Garry Yao 16 years ago.
3256_5.patch (4.2 KB) - added by Garry Yao 16 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 16 years ago by Garry Yao

Keywords: Confirmed added
Owner: set to Garry Yao
Status: newassigned

comment:2 Changed 16 years ago by Garry Yao

Component: GeneralCore : Styles
Description: modified (diff)
Keywords: Review? added

Update the Expected Result of this TC.

Changed 16 years ago by Garry Yao

Attachment: 3256.patch added

comment:3 Changed 16 years ago by Garry Yao

Two updates:

  1. 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>
    
  2. Make the justify style happen on multiple ranges instead of only the first.

comment:4 Changed 16 years ago by Frederico Caldeira Knabben

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 Garry Yao

Attachment: 3256_2.patch added

comment:5 Changed 16 years ago by Garry Yao

Keywords: Review? added; Review- removed

It's again a matter of multiple ranges mangled by bookmark creation process.

Changed 16 years ago by Garry Yao

Attachment: 3256_3.patch added

comment:6 Changed 16 years ago by Garry Yao

Reflecting the latest patch from #3475 for easy reviewing.

comment:7 Changed 16 years ago by Frederico Caldeira Knabben

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 Garry Yao

Attachment: 3256_4.patch added

comment:8 Changed 16 years ago by Garry Yao

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 Frederico Caldeira Knabben

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 Garry Yao

Attachment: 3256_5.patch added

comment:10 Changed 16 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:11 Changed 16 years ago by Garry Yao

Priority: NormalHigh

comment:12 Changed 16 years ago by Martin Kou

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-- )
            {

comment:13 Changed 16 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3662].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy