Ticket #3256 (closed Bug: fixed)

Opened 5 years ago

Last modified 5 years ago

Justify across table cells incorrect

Reported by: garry.yao Owned by: garry.yao
Priority: High Milestone: CKEditor 3.0
Component: Core : Styles Version:
Keywords: Confirmed Review+ Cc:

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

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

3256.patch (4.2 KB) - added by garry.yao 5 years ago.
3256_2.patch (5.2 KB) - added by garry.yao 5 years ago.
3256_3.patch (8.7 KB) - added by garry.yao 5 years ago.
3256_4.patch (4.0 KB) - added by garry.yao 5 years ago.
3256_5.patch (4.2 KB) - added by garry.yao 5 years ago.

Change History

comment:1 Changed 5 years ago by garry.yao

  • Keywords Confirmed added
  • Status changed from new to assigned
  • Owner set to garry.yao

comment:2 Changed 5 years ago by garry.yao

  • Keywords Review? added
  • Component changed from General to Core : Styles
  • Description modified (diff)

Update the Expected Result of this TC.

Changed 5 years ago by garry.yao

comment:3 Changed 5 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 5 years ago by fredck

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

comment:5 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

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

Changed 5 years ago by garry.yao

comment:6 Changed 5 years ago by garry.yao

Reflecting the latest patch from #3475 for easy reviewing.

comment:7 Changed 5 years ago by fredck

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

comment:8 Changed 5 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 5 years ago by fredck

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

comment:10 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:11 Changed 5 years ago by garry.yao

  • Priority changed from Normal to High

comment:12 Changed 5 years ago by martinkou

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

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

Fixed with [3662].

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