Opened 8 years ago

Closed 8 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 8 years ago.
3256_2.patch (5.2 KB) - added by Garry Yao 8 years ago.
3256_3.patch (8.7 KB) - added by Garry Yao 8 years ago.
3256_4.patch (4.0 KB) - added by Garry Yao 8 years ago.
3256_5.patch (4.2 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by Garry Yao

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

comment:2 Changed 8 years ago by Garry Yao

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

Update the Expected Result of this TC.

Changed 8 years ago by Garry Yao

Attachment: 3256.patch added

comment:3 Changed 8 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 8 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 8 years ago by Garry Yao

Attachment: 3256_2.patch added

comment:5 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

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

Changed 8 years ago by Garry Yao

Attachment: 3256_3.patch added

comment:6 Changed 8 years ago by Garry Yao

Reflecting the latest patch from #3475 for easy reviewing.

comment:7 Changed 8 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 8 years ago by Garry Yao

Attachment: 3256_4.patch added

comment:8 Changed 8 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 8 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 8 years ago by Garry Yao

Attachment: 3256_5.patch added

comment:10 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:11 Changed 8 years ago by Garry Yao

Priority: NormalHigh

comment:12 Changed 8 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 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3662].

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