Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font styles are not honored when pasting from MS Word #6165

Closed
jswiderski opened this issue Jan 30, 2020 · 10 comments · Fixed by ckeditor/ckeditor5-font#61
Closed

Font styles are not honored when pasting from MS Word #6165

jswiderski opened this issue Jan 30, 2020 · 10 comments · Fixed by ckeditor/ckeditor5-font#61
Assignees
Labels
package:font package:paste-from-office support:3 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@jswiderski
Copy link

📝 Provide detailed reproduction steps (if any)

  1. Add, to article.js plugins: PasteFromOffice, Font and Highlight (last one just in case).
  2. Configure CKEditor font as shown in attached article.js file. The configuration used should exclude possibility of something not being configured - article.zip.
  3. Paste contents of attached MS Word file - testfile.zip

✔️ Expected result

When pasting from MS Word file, font styling should be preserved.

❌ Actual result

No styling gets preserved.

issue2

📃 Other details

  • Browser: Any
  • OS: Any
  • CKEditor version: Latest
  • Installed CKEditor plugins: PasteFromOffice, Font, Highlight

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@jswiderski jswiderski added the type:bug This issue reports a buggy (incorrect) behavior. label Jan 30, 2020
@Mgsy
Copy link
Member

Mgsy commented Jan 30, 2020

I can confirm it. However, should it be handled? It's a bug or feature request?

@jswiderski jswiderski added the support:1 An issue reported by a commercially licensed client. label Jan 30, 2020
@jswiderski
Copy link
Author

jswiderski commented Jan 30, 2020

I believe this is a bug as preserving font styles should be one of basic functionalities of the PasteFromOffice plugin.

NOTE: The problem has also been reported on our support channel.

@Reinmar
Copy link
Member

Reinmar commented Jan 30, 2020

Strongly related to:

The latter is in fact what most people would expect. But the former would be useful as well. In other words, we have 3 levels of content filtering:

@Reinmar Reinmar added this to the nice-to-have milestone Jan 30, 2020
@Reinmar Reinmar modified the milestones: nice-to-have, iteration 30 Feb 18, 2020
@Reinmar
Copy link
Member

Reinmar commented Feb 21, 2020

Let's introduce config options for those two features (font family, font size) which will allow developers to disable the value matching.

@Reinmar
Copy link
Member

Reinmar commented Mar 2, 2020

This should be an opt-in behaviour. Enabled through a config option.

Let's also use that config option in the PFW and PFGDocs documentation pages.

@pomek
Copy link
Member

pomek commented Mar 4, 2020

I wrote two converters that keep values of style="font-family: *" in our model and use it directly in the view.

Model:

image

View:

image

editor.getData():

"<p><span style="font-family:docs-Roboto, Arial;">Roboto: Test – didn't define in config</span></p><p><span style="font-family:Courier New;">Courier New: Test – visible in fontFamily dropdown</span></p>"

More or less the same logic could be applied to font-size part of the feature. Is that correct way?

@pomek
Copy link
Member

pomek commented Mar 5, 2020

Answering my own question.

More or less the same logic could be applied to font-size part of the feature. Is that correct way?

It could be if font size would be defined as numbers. We have two cases that need to be handled:

  1. A user pasted a text: font-size has specified a unit. We store a value font-size: * in the model and pass it directly to the view.
  2. The user set the font-size of a text using the font size dropdown. As for now, we store a number in the model. During the view conversion, we need to attach a unit to it. Should it be px? Should we have an option for customizing the unit?

Unfortunately, the font-size feature allows configuring presets. They are converted to classes in the view so the converter must be a little smarter.

  1. We can detect what kind of value is store as the fontSize attribute in the text. Based on that, add a class or define the style (font-size: *). It should work.
    image
  2. Another possible solution could be a breaking change in the plugin configuration. If we will keep in the model numeric values, we can convert values to the style="font-size: *" attribute.
  3. The last one, I guess, an unacceptable solution is dropping support for presets and keep numeric values only.

@Reinmar
Copy link
Member

Reinmar commented Mar 6, 2020

The most important assumption here is that the feature works in either the "only preconfigured values" or the "any value" modes. Not in both at the same time.

If you're in the former more, you have model values working exactly like today. If you switch the mode, the model values need to keep the actual font-size style value.

@pomek
Copy link
Member

pomek commented Mar 9, 2020

The last question – how the font size plugin configuration should look if "any value" mode is enabled? Keeping presets does not seem like a correct way.

Could we replace presets with numbers: fontSize: { options: [ 10, 12, 14, 'default', 18, 20, 22 ] } or?

@pomek
Copy link
Member

pomek commented Mar 9, 2020

An answer to the question:

how the font size plugin configuration should look if "any value" mode is enabled?

Was in the sentence:

If you switch the mode, the model values need to keep the actual font-size style value.

This way I went.

@Reinmar Reinmar modified the milestones: iteration 30, iteration 31 Mar 12, 2020
Reinmar added a commit to ckeditor/ckeditor5-font that referenced this issue Mar 25, 2020
Feature: Introduced an option for both `FontSize` and `FontFamily` plugins that allow preserving any `font-family` and `font-size` values when pasting or loading content. Closes ckeditor/ckeditor5#6165. Closes ckeditor/ckeditor5#2278.
@lslowikowska lslowikowska added support:3 An issue reported by a commercially licensed client. and removed support:1 An issue reported by a commercially licensed client. labels Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:font package:paste-from-office support:3 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants