Opened 2 years ago

Closed 22 months ago

#16675 closed Bug (fixed)

Copy formatting - tables issue

Reported by: Wiktor Walc Owned by: Tomasz Jakut
Priority: Normal Milestone: CKEditor 4.6.2
Component: General Version: 4.6.0
Keywords: Cc:

Description (last modified by Wiktor Walc)

Steps to reproduce

  1. <h2 style="text-align: center;"><span style="font-family:Arial,Helvetica,sans-serif">CONSOLIDATED STATEMENTS OF CASH FLOWS&nbsp;</span></h2>
    
    <p style="text-align: center;"><span style="font-family:Arial,Helvetica,sans-serif">(in millions of $)</span></p>
    
    <table cellpadding="3" cellspacing="1" style="height:80px; width:800px">
    	<tbody>
    		<tr>
    			<td style="background-color:#eeeeee; text-align:center; width:436px"><span style="font-size:14px"><span style="font-family:Arial,Helvetica,sans-serif"><strong>INVESTING ACTIVITIES</strong></span></span></td>
    			<td style="background-color:#eeeeee; text-align:center; width:96px"><strong><span style="font-size:14px"><span style="font-family:Arial,Helvetica,sans-serif">Q1 2015</span></span></strong></td>
    			<td style="background-color:#eeeeee; text-align:center; width:95px"><span style="font-size:14px"><span style="font-family:Arial,Helvetica,sans-serif"><strong>Q2 2015</strong></span></span></td>
    			<td style="background-color:#eeeeee; text-align:center; width:92px"><span style="font-size:14px"><span style="font-family:Arial,Helvetica,sans-serif"><strong>Q3 2015</strong></span></span></td>
    			<td style="background-color:#eeeeee; text-align:center; width:92px"><span style="font-size:14px"><span style="font-family:Arial,Helvetica,sans-serif">Q4 <span style="display:none">&nbsp;</span><strong>2015</strong></span></span></td>
    		</tr>
    		<tr>
    			<td style="width:436px">Purchases of property and equipment</td>
    			<td style="width:96px">1,841</td>
    			<td style="width:95px">1,195</td>
    			<td style="width:92px">1,456</td>
    			<td style="width:89px">1,980</td>
    		</tr>
    		<tr>
    			<td style="width:436px">Acquisitions, net of cash acquired, and other</td>
    			<td style="width:96px">84</td>
    			<td style="width:95px">105</td>
    			<td style="width:92px">87</td>
    			<td style="width:89px">107</td>
    		</tr>
    		<tr>
    			<td style="width:436px">Sales and maturities of marketable securities</td>
    			<td style="width:96px">1,431</td>
    			<td style="width:95px">1,045</td>
    			<td style="width:92px">1,765</td>
    			<td style="width:89px">1,823</td>
    		</tr>
    		<tr>
    			<td style="width:436px">Purchases of marketable securities</td>
    			<td style="width:96px">2,076</td>
    			<td style="width:95px">1,122</td>
    			<td style="width:92px">1,587</td>
    			<td style="width:89px">1,290</td>
    		</tr>
    		<tr>
    			<td style="width:436px">Net cash provided by (used in) investing activities</td>
    			<td style="width:96px">2,570</td>
    			<td style="width:95px">1,377</td>
    			<td style="width:92px">987</td>
    			<td style="width:89px">1,560</td>
    		</tr>
    	</tbody>
    </table>
    
    <p>&nbsp;</p>
    
    <table cellpadding="3" cellspacing="1">
    	<tbody>
    		<tr>
    			<td>INVESTING ACTIVITIES</td>
    			<td>Q1 2016</td>
    			<td>Q2 2016</td>
    			<td>Q3 2016</td>
    			<td>Q4 2016</td>
    		</tr>
    		<tr>
    			<td>Purchases of property and equipment</td>
    			<td>1,841</td>
    			<td>1,195</td>
    			<td>&nbsp;</td>
    			<td>&nbsp;</td>
    		</tr>
    		<tr>
    			<td>Acquisitions, net of cash acquired, and other</td>
    			<td>84</td>
    			<td>105</td>
    			<td>&nbsp;</td>
    			<td>&nbsp;</td>
    		</tr>
    		<tr>
    			<td>Sales and maturities of marketable securities</td>
    			<td>1,431</td>
    			<td>1,045</td>
    			<td>&nbsp;</td>
    			<td>&nbsp;</td>
    		</tr>
    		<tr>
    			<td>Purchases of marketable securities</td>
    			<td>2,076</td>
    			<td>1,122</td>
    			<td>&nbsp;</td>
    			<td>&nbsp;</td>
    		</tr>
    		<tr>
    			<td>Net cash provided by (used in) investing activities</td>
    			<td>2,570</td>
    			<td>1,377</td>
    			<td>&nbsp;</td>
    			<td>&nbsp;</td>
    
  2. http://ckeditor.com/latest/samples/index.html
  3. Paste the code in source mode and follow the gif (copy formatting from Q3 to Q4)

Expected result

Actual result

Other details (browser, OS, CKEditor version, installed plugins)

Attachments (1)

copyformatting2.gif (98.4 KB) - added by Wiktor Walc 2 years ago.

Download all attachments as: .zip

Change History (14)

Changed 2 years ago by Wiktor Walc

Attachment: copyformatting2.gif added

comment:1 Changed 2 years ago by Wiktor Walc

Description: modified (diff)

comment:2 Changed 2 years ago by Wiktor Walc

Milestone: CKEditor 4.6.1
Version: 4.6.0

comment:3 Changed 2 years ago by Wiktor Walc

Description: modified (diff)

comment:4 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2
Status: newconfirmed

Confirmed, however we won't be able to contain it within 4.6.1 - looking for 4.6.2.

comment:5 Changed 22 months ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: confirmedassigned

comment:6 Changed 22 months ago by Tomasz Jakut

The issue seems to be connected with the way in which styles are applied inside the editor. They're applied from the text node to the top of DOM tree. In rare cases (such as this one) it causes the change of the whole selection and breaks the walker inside getNodeAndApplyCmd function.

comment:7 Changed 22 months ago by Tomasz Jakut

Status: assignedreview

Pushed proposed solution to branch:t/16675.

The solution is simple: I've added additional check to the getNodeAndAppyCmd function that checks if the range has correct end container. If it's incorrect, the whole function is restricted to the first matching element.

comment:8 Changed 22 months ago by kkrzton

Status: reviewreview_failed

Generally the fix works fine and solves the issue.

However, I have some doubts which I would like to be clarified. If I understand correctly, in this case the range which is used has incorrect end container. Such range is created while applying styles (but before any changes in a DOM) on the selected element. For the table context, first, styles on the text context are applied (all styling elements are removed and new/copied ones are applied) and then styles on the table context are applied. The same range (from applyFormatting event) is used for applying styles in both contexts, however, after first styles were applied the range is already outdated (the styles elements it was based on were removed by style application on text context).

In this case, the issue is somehow caused by design (the flow/order of operations). If we are okay with that, the proposed solution should be sufficient (but we need to make sure if there are no similar, uncovered cases with different elements like e.g. lists). The other solution would be to adjust the way the styles are applied but it would require much more work probably. WDYT @t.jakut?

comment:9 Changed 22 months ago by Tomasz Jakut

Status: review_failedreview

Yes, it could be said that the defect is caused by the design of our mechanism for applying styles. The current fix covers also lists (as they are also handled by getNodeAndApplyCmd. I've added unit test for them, too.

BTW I've just discovered alternative method: bookmarks (just like it's done for lists). I've pushed it to the branch:t/16675-b. I think that this new approach is less dirty and should cover all situations that could be caused by our style system.

comment:10 Changed 22 months ago by kkrzton

Status: reviewreview_failed

The branch:t/16675-b is the way to go IMHO. Much cleaner solution and it adds a proper fix (updating the range when it has changed).

However, some unit tests are failing on FF, Safari, IE8, IE11 and Edge:

test applying styles from one cell to other (classic)
test applying styles from one cell to other (inline)

Btw. I think the test name should be test applying styles from one cell to another instead of test applying styles from one cell to other.

comment:11 Changed 22 months ago by Tomasz Jakut

Status: review_failedreview

I've updated test and pushed to branch:t/16675-b. I've abandoned approach with comparing content of the editor in favor of just checking if the last cell got styles or not (no more problems with incompatibilities between browsers…).

comment:12 Changed 22 months ago by kkrzton

Status: reviewreview_passed

All tests passing, LGTM!

comment:13 Changed 22 months ago by kkrzton

Resolution: fixed
Status: review_passedclosed

Merged to master with 851352c.

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