Opened 3 years ago

Closed 3 years ago

#13884 closed Bug (fixed)

Copy/paste table in Firefox results in just first cell being pasted

Reported by: Jakub Ś Owned by: Tomasz Jakut
Priority: Normal Milestone: CKEditor 4.5.7
Component: General Version: 4.5.0
Keywords: Firefox Support Cc:

Description

Steps to reproduce

  1. Paste below code in source mode and switch to wysiwyg
    <table border="1" cellpadding="1" cellspacing="1" style="width:500px">
        <tbody>
            <tr>
                <td style="background-color: rgb(102, 102, 204);">s</td>
                <td>s</td>
            </tr>
            <tr>
                <td>s</td>
                <td style="background-color: rgb(255, 255, 0);">s</td>
            </tr>
            <tr>
                <td style="background-color: rgb(255, 204, 0);">s</td>
                <td style="background-color: rgb(255, 204, 0);">s</td>
            </tr>
        </tbody>
    </table>
    
  2. Select whole table with a mouse and Press Ctrl+C (You can also use Copy from context menu)
  3. Press Ctrl+V or right-click below table and select Paste (Ctrl+V into paste dialog and click OK)

Expected result

Whole table should be pasted and formatting preserved.

Actual result

Only first row gets pasted.

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

Problem can be reproduced from CKEditor 4.5.0 in Firefox only.

NOTE: From CKEditor 4.0 - 4.4.8 the above scenario resulted in table being pasted but without width style (background was preserved).

Change History (15)

comment:1 Changed 3 years ago by Jakub Ś

Keywords: Support added
Status: newconfirmed

Temporary workaround is to select table element by clicking table element on element's path. This feature selects whole table and using later the Ctrl+C/Ctrl+V or context menu works as expected.

comment:2 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5

comment:3 Changed 3 years ago by Marek Lewandowski

It possibly can be related with #13883.

comment:4 Changed 3 years ago by Piotr Jasiun

This is because in 4.5.0 custom copy and paste was introduced and it does not handle multi-range selection, it use only the first range. Firefox use multiple ranges to represent table selection - each row is a separate range. This is why only the first row is copied.

To make works custom copy and paste should handle multi-ranges.

comment:5 Changed 3 years ago by Marek Lewandowski

This issue is likely to originate from editor.getSelectedHtml which relies only on a single range. From clipbaord perspective it's called in CKEDITOR.plugins.clipboard.dataTransfer method.

Last edited 3 years ago by Marek Lewandowski (previous) (diff)

comment:6 Changed 3 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:7 Changed 3 years ago by Marek Lewandowski

Pushed tests to t/13884.

comment:8 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5CKEditor 4.5.6

comment:9 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.6CKEditor 4.5.7

comment:10 Changed 3 years ago by Marek Lewandowski

Owner: Marek Lewandowski deleted
Status: assignedconfirmed

comment:11 Changed 3 years ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: confirmedassigned

comment:12 Changed 3 years ago by Tomasz Jakut

Status: assignedreview

Changing editor.getSelectedHtml to handle multiple ranges seems to fix the problem.

I've created new private function createDocFragmentFromRanges which can be used to handle all cases of creating DocumentFragments from mutliple ranges (at the moment it handles only tables in Firefox).

Pushed branch:t/13884.

comment:13 Changed 3 years ago by Marek Lewandowski

Status: reviewreview_failed

Rebased to master and added a another TC in partialtable.md.

  • Partial copy does not necessarily works well.

This fix is a little too simple. Ranges does not need to be set from the very beginning of the selection all the way down to the last element in the selection. It might contain some parts of the document (table in this case) while skipping the other parts.

If you select 2x2 cells part out of a 3x3 table then the whole table gets pasted. Before multirange was broken in 4.5.0 this case was working OK (e.g. see 4.3 tag).

I've added a TC for that.

Last edited 3 years ago by Marek Lewandowski (previous) (diff)

comment:14 Changed 3 years ago by Tomasz Jakut

Status: review_failedreview

I've updated createDocumentFragmentFromRanges function. Now it iterates through all ranges and recreates the copied table from scratch. I've also added unit test for partial table copy. Pushed changes to branch:t/13884.

comment:15 Changed 3 years ago by Marek Lewandowski

Resolution: fixed
Status: reviewclosed

Fixed with git:8a5e2265aa (merged to master).

Few additional comments:

Proposed fix does the job well, but what I'd like to see here is a better integration with already existing code. All the heavy operations are well hidden behind this intefrace.

New code duplicates a little code from getHtmlFromRangeHelpers.tree.rebuild. Ofc the problem with all these methods is that they're focused on a single range.

I think we could change this piece of code:

var that = {
	doc: this.getDocument(),
	// Leave original range object untouched.
	range: range.clone()
};

We could simply add either selection there or simply ranges array, so that each (like getHtmlFromRangeHelpers.tree.rebuild) can be aware that it's a multirange call.

For now we'll merge this code as it's a very important fix that can no longer wait, but there will be a follow up ticket.

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