Opened 3 years ago

Closed 3 years ago

#16653 closed Bug (fixed)

Paste From Word settings are reversed

Reported by: Tade0 Owned by: kkrzton
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.6.0
Component: General Version:
Keywords: Cc:


Steps to reproduce

  1. Set pasteFromWordRemoveFontStyles to false.
  2. Paste content from Word with font size, color etc.
  3. Set pasteFromWordRemoveFontStyles to true.

Expected result

Styles are removed when the setting is true.

Actual result

Styles are removed when the setting is false.

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

Change History (10)

comment:1 Changed 3 years ago by kkrzton

Status: newconfirmed

comment:2 Changed 3 years ago by kkrzton

Owner: set to kkrzton
Status: confirmedassigned

comment:3 Changed 3 years ago by kkrzton

Seems like an error/typo in the code.

While previously the config.pasteFromWordRemoveFontStylesoption was used like

removeFontStyles = config.pasteFromWordRemoveFontStyles !== false

(plugins/pastefromword/filter/default.js#L684) in the new version of PFW it is used like

removeFontStyles = config.pasteFromWordRemoveFontStyles === false

(plugins/pastefromword/filter/default.js#L417). It appearsed in a new version of PFW to provide backward compatibility (brought back here) so it should work the same way as previously.

The important thing here is that the default value for this config option had changed from false to true so the above seems like a proper change.

Last edited 3 years ago by kkrzton (previous) (diff)

comment:4 Changed 3 years ago by kkrzton

This one is tricky. It seems like the correct interpretation of config.pasteFromWordRemoveFontStyles is

preserveFontStyles = editor && editor.config.pasteFromWordRemoveFontStyles === false; or

removeFontStyles = !( editor && editor.config.pasteFromWordRemoveFontStyles === false );.

The value defaults to true so only when explicitly set to false, font styles should not be removed. Applying such fix shows the correct behaviour in a manual test but results in more than 50% of PFW unit tests failing (even after reversing config.pasteFromWordRemoveFontStyles value for test when it was used).

comment:5 Changed 3 years ago by kkrzton

When setting config.pasteFromWordRemoveFontStyles = false for all tests (except the one which earlier had config.pasteFromWordRemoveFontStyles = false set explicitly), there are only 13 unit tests failing. However, I suppose we should use default value (which is true) for most of the tests.

comment:6 Changed 3 years ago by kkrzton

Pushed changes to t/16653.

To sum up (previous comments may be somehow confusing, stating opposite things to each other as I created them ad hoc):

The pasteFromWordRemoveFontStyles option was in fact reversed and code needed to be updated as it assumed reversed flow (46bff8b).

While this option had been reversed, the config for all tests have to be reversed as well (873fbb8).

However, there are few things I am not sure about:

The code states that Prior to version 4.6.0 this configuration option defaulted to 'false'. According to the commit acb029e and SDK (!/api/CKEDITOR.config-cfg-pasteFromWordRemoveFontStyles) it seems it defaulted to true. Also looking at the docs it seems like now it should have default value false (plugins/pastefromword/filter/default.js#L1653).

As I updated this config value, most of the test had to be updated with pasteFromWordRemoveFontStyles: false option. I assume most of the cases should be tested with default value (as it is the most widely used also).

I see the intention might be to set default value to false (as opposite to true earlier). I will just recheck it and provide another fix branch.

comment:7 Changed 3 years ago by kkrzton

Pushed changes to t/16653b.

Assumed that the default pasteFromWordRemoveFontStyles is false (opposite to t/16653). It required very little changes in the existing code. I will just to make sure what default value it should be.

comment:8 Changed 3 years ago by kkrzton

Status: assignedreview

comment:9 Changed 3 years ago by kkrzton

The correct branch for R is t/16653b.

comment:10 Changed 3 years ago by Marek Lewandowski

Resolution: fixed
Status: reviewclosed

LGTM! Fixed in git:68643fdfb6680e95f2b40788754e3fc5d1e9eca0, merged to major.

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