Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#13528 closed Bug (fixed)

[Firefox][pasteFromWord] Content is pasted without formating

Reported by: Piotr Jasiun Owned by: Piotr Jasiun
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.5.2
Component: General Version: 4.5.0
Keywords: Cc:

Description

When I paste the content from Word to Firefox I loose the formatting.

Everything is fine when I use paste from Word button. Also on version 4.4.8 it works fine. It also works on Chrome.

It is most probably because of using new Clipboard API on Firefox, and trusting Firefox that the content is correct.

Change History (26)

comment:1 Changed 3 years ago by Frederico Caldeira Knabben

We're clearly in need of a manual test for PFW.

comment:2 Changed 3 years ago by Jakub Ś

Status: newconfirmed

I have been able to reproduce it from 4.5.0 (works fine in 4.5 beta).

comment:3 Changed 3 years ago by Piotr Jasiun

Unfortunately on the Firefox when we paste content from Word we get only text/plain data. I already reported this bug to the Firefox team: https://bugzilla.mozilla.org/show_bug.cgi?id=1183686

comment:4 Changed 3 years ago by Piotr Jasiun

And unfortunately it means we need to rethink changes introduced in #13187.

comment:5 Changed 3 years ago by Piotr Jasiun

https://bugzilla.mozilla.org/show_bug.cgi?id=1183686 was marked as duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=586587 which is already assigned what give us a chance of for the relatively quick fix.

Last edited 3 years ago by Piotr Jasiun (previous) (diff)

comment:6 Changed 3 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:7 Changed 3 years ago by Jakub Ś

Starting from version 4.5.0, it is no longer possible to paste Tables from Excel in Firefox and Blink.

Up to version 4.4.8 it was possible to paste Raw Table (not our fault - browsers other than IE see no styling). At the moment only Raw text is pasted.

comment:8 Changed 3 years ago by Piotr Jasiun

Status: assignedreview

It means that all of the browsers when we use Clipboard API do not give us as much data as paste bin. I believe this is the point we should decide to give up using Clipboard API to get HTML data for external paste.

We could try get HTML data if it is available, this was the first approach for #13187, but I believe we will find then the situation where HTML data are available but are incorrect and trying to recognize if this data is correct would be a dead end. We changed the code for recognizing if Clipboard API should be used already twice and each time we found new bugs. It is simply too buggy.

I think it was too drastic change and my idea is to rollback to using paste bin again for external paste if there are no files. It does not mean that Clipboard API is useless now:

  • Clipboard API data will be still available in the paste event, so http://sdk.ckeditor.com/samples/draganddrop.html will not be broken,
  • Clipboard API will be still used for internal copy/cut and paste, we have no bugs reported for it,
  • Clipboard API will be still used for drag and drop of any type.

I tested these changes and everything seems to work fine, but because we change the way we handle paste we should deeply test pasting during the testing phase for 4.5.2.

Changes in t/13528.

comment:9 Changed 3 years ago by Jakub Ś

After changes in that branch pasting in Chrome and Firefox is again possible from MS Excel and MS Word.

Last edited 3 years ago by Jakub Ś (previous) (diff)

comment:10 Changed 3 years ago by Piotrek Koszuliński

Priority: NormalHigh

comment:11 Changed 3 years ago by Piotrek Koszuliński

This is a huge disappointment, but as we can see in https://bugzilla.mozilla.org/show_bug.cgi?id=586587 there's a chance that a patch will land soon.

Therefore, I'm rather against another drastic change (especially that it touches Chrome and FF). We cannot jump between A and Z all the time. When FF's bug is fixed we'll try to bring back the integration with clipboard API again and again we'll do a drastic change (again on Chrome and FF). I believe that we should do smaller steps now. Introduce a temporary (hopefully) workaround, but only for this specific case.

So I want to ask this – what are the other currently known issues regarding access to clipboard? If on FF and Chrome (on which AFAICS isHtmlInExternalDataTransfer was true) this is the only one, then I would consider extending the check in pasteDataFromClipboard. Do you thing that it is possible?

comment:12 in reply to:  11 Changed 3 years ago by Alfonso Martínez de Lizarrondo

Replying to Reinmar:

This is a huge disappointment, but as we can see in https://bugzilla.mozilla.org/show_bug.cgi?id=586587 there's a chance that a patch will land soon.

I only see see a proposed patch but it hasn't even asked for review at the moment.

After the patch is reviewed, every detail is fixed, etc... it still has to land on the Nighlty builds, go to Aurora, Beta and finally I think that 12 weeks since the first landing it will end in the next release of Firefox.

So the people that upgrades to that future Firefox might be free of this problem (remember that there's also a ESR version that will take much more time to be updated), but meanwhile pasting is broken with CKEditor 4.5 so at the moment the best option is to not upgrade CKEditor until this problem is fixed.

comment:13 Changed 3 years ago by Piotrek Koszuliński

Of course, this problem must be fixed in CKEditor right now, because we can never be sure if the patch in Firefox will finally land or not and when. My point was that the patch should not be that drastic again. In few months, when patch lands in Firefox, we'll be reverting it again. If it's possible, I would want this specific issue to be workarounded, not the whole implementation turned upside down.

comment:14 Changed 3 years ago by Piotrek Koszuliński

Status: reviewassigned

comment:15 Changed 3 years ago by Piotr Jasiun

After a talk we decided to give Clipboard API a chance. I pushed changes to t/13528b, but they need to be deeply tested, because there are plenty of cases that can not be tested automatically: paste from Word, copy and paste on Android, iOS, Android in desktop mode, also browsers behave differently on different OSes when it comes to clipboard (#13583).

Last edited 3 years ago by Piotr Jasiun (previous) (diff)

comment:16 Changed 3 years ago by Piotr Jasiun

Status: assignedreview

comment:17 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I rebased and pushed one commit to the branch.

The thing I noticed is that if we have files on FF, the pastebin should not be used. Otherwise, we may end up with a file name in the pastebin, breaking support for pasting files.

Please also make sure to keep the changes minimal. This is – touch Firefox and preferably nothing more.

Last edited 3 years ago by Piotrek Koszuliński (previous) (diff)

comment:18 Changed 3 years ago by Piotr Jasiun

Status: review_failedreview

Agreed. Pushed changes to t/13528b.

comment:19 Changed 3 years ago by Piotr Jasiun

I also pushed tests for the same branch.

comment:20 Changed 3 years ago by Piotr Jasiun

And tested it manually on Windows, Linux, iOS and Android. Pasting works fine.

comment:21 in reply to:  17 Changed 3 years ago by Piotr Jasiun

Replying to Reinmar:

Please also make sure to keep the changes minimal. This is – touch Firefox and preferably nothing more.

In fact the check if paste event contains files can be limited to the Firefox (9caad3a) and on the one hand the minimum change means minimum risk. On the other, it makes sense only because Firefox is the only browser which support pasting files. If we make the check more generic (use Clipboard API if data transfer contains files) pasting files will works as soon as browsers will support it.

comment:22 Changed 3 years ago by Piotrek Koszuliński

On Chrome we require that the DT isn't empty, so as soon as some files will appear there it's going to work, won't it? As for the rest of browsers... let's wait. It would make sense to loosen that check to cover Edge and Safari as well, but that would mean awful lot of testing again. I guess that none of us want this :D

comment:23 Changed 3 years ago by Piotrek Koszuliński

BTW. I realised that the easiest way to validate this ticket on FF is to paste from some other browser, so I extended the MT. I also tagged it with 4.5.2, so we'll again cover it.

comment:24 Changed 3 years ago by Piotrek Koszuliński

Fixed on master with git:933b853.

comment:25 Changed 3 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

comment:26 Changed 3 years ago by Piotr Jasiun

Note that this is a small change in the API. This ticket removed CKEDITOR.plugins.clipboard.isHtmlInExternalDataTransfer property and introduced CKEDITOR.plugins.clipboard.canClipboardApiBeTrusted function. I believe that function is more flexible it prevent future changes.

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