Opened 13 months ago

Closed 12 months ago

Last modified 12 months ago

#16624 closed Bug (fixed)

[PFW] Improve integration with Color Button plugin

Reported by: Marek Lewandowski Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.6.1
Component: Plugin : Paste from Word Version: 4.6.0
Keywords: Cc:

Description (last modified by Marek Lewandowski)

New PFW implementation puts font background as span with background css property. It's not handled by any of our features (colorbutton in particular), and therefore ACF strips it in the process.

If we'd set it as span background-color everything would work OK.

Additionally we should respect `config.colorButton_backStyle`, that way it will always use a proper style.

  1. Open PFW full manual test case.
  2. Open any document with font background in Word (e.g. last two lines in ckeditor-dev/tests/plugins/pastefromword/generated/_fixtures/Fonts/Fonts.docx).
  3. Select some text with background in Word.
  4. Copy it.
  5. Paste into the manual test editor.

Expected: Pasted rich text preserves text background.

Actual: Background gets removed.

Reproduced on Chrome @ Win10.

Change History (14)

comment:1 Changed 13 months ago by Marek Lewandowski

Description: modified (diff)

comment:2 Changed 13 months ago by Marek Lewandowski

Description: modified (diff)

comment:3 Changed 13 months ago by Marek Lewandowski

Description: modified (diff)

comment:4 Changed 13 months ago by kkrzton

Description: modified (diff)

I can confirm it also happens on FF.

comment:5 Changed 13 months ago by kkrzton

Description: modified (diff)

comment:6 Changed 13 months ago by Marek Lewandowski

Status: newconfirmed

comment:7 Changed 12 months ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:8 Changed 12 months ago by Marek Lewandowski

Status: assignedreview

So the source of the problem was that Word is pasting text background as e.g. background:yellow. When adding a colorbutton plugin it adds background-color to the ACF but not background.

There were two possibilities to fix it:

  • Adding an additional handling to the Paste from Word plugin - converting background property to background-color if possible.
  • Adding a more generic fix in the colorbutton plugin itself. If span's background contains only color then it make sense to convert it to background-color so that colorbutton plugin can handle it.

I went with the second option as it will enhance more cases than Paste from Word only. It's important to highlight that this transformation can only happen if color is the only property specified by the background property, in any other case we should abort transformation and leave it untouched.

This feature is controlled with a config option in case someone would like to disable it, with a config.colorButton_normalizeBackground and it defaults to true.

With that I've added two namespaces in CKEDITOR.tools that I've been always thinking about, first is CKEDITOR.tools.array for all the array-related methods and the other is CKEDITOR.tools.style for any style related stuff. It's just the beginning, and there are just a few methods. We'll put more functions with a follow-up ticket, also we will deprecate moved methods directly in CKEDITOR.tools, but that's out of scope of this ticket.

Pushed to t/16624.

comment:9 Changed 12 months ago by Jakub Ś

#16602 was marked as duplicate.

comment:10 Changed 12 months ago by kkrzton

Status: reviewreview_failed

The second approach indeed seems better IMHO.

I was only wondering if this fix should be tied to colorbutton plugin. Without the plugin it won't work. However, the colorbutton plugin is the one adding background ACF rule so it seems like a proper place for the fix. The only case in which it will not work is when the same rule is added manually (without colorbutton plugin), but then we should assume that other ACF rules fixing the issue also should be add manually.

The solution looks pretty solid, just a few things to check/fix:

  • The CKEDITOR.tools.style.background returns only first color. This is ok and probably intended, however, the method description is a little misleading. Return Parsed background values as an array - as every color is a separate value, one may expect that it will return all parsed colors.
  • For the array tools we could document what parameters will be passed to the callback function. While it uses same parameters as native implementation I am not sure if it is really needed.

comment:11 in reply to:  10 Changed 12 months ago by Marek Lewandowski

Status: review_failedreview

Replying to k.krzton:

I was only wondering if this fix should be tied to colorbutton plugin. Without the plugin it won't work. However, the colorbutton plugin is the one adding background ACF rule so it seems like a proper place for the fix.

It make sense for colorbutton plugin as this is the plugin that provides background-related feature. Also it's colorbuttons implementation detail that it requires background-color rather than just background.

The only case in which it will not work is when the same rule is added manually (without colorbutton plugin), but then we should assume that other ACF rules fixing the issue also should be add manually.

Could you elaborate the example above? I'm not sure if I got it right.

Thanks, fixed in git:b24993d.

  • The CKEDITOR.tools.style.background returns only first color. This is ok and probably intended, however, the method description is a little misleading. Return Parsed background values as an array - as every color is a separate value, one may expect that it will return all parsed colors.

Absolutely, I did not reflect changes made with git:e3cf0c9d73e4725ee9ba5bfe9475b0277ae683e8 in the API docs. I made a more "fun" but also more complex initial implementation, but in the end the benefit did not justify the complexity.

Initially it supported props like: background: red, blue url(foo.png) repeat-x, url(bar.png) top right and it would return an array with 3 objects { color: 'xyz', unprocessed: 'foo' }.

As for the current implementation, returingin a single color is intended just like you noted. Look at browser behavior, those will use only first color, and ignore any others. So we pick first color to color property, and remove any other from unprocessed not to pollute it with garbage.

  • For the array tools we could document what parameters will be passed to the callback function. While it uses same parameters as native implementation I am not sure if it is really needed.

Make sense, it will be more readable, fixed in git:2ff808d.

Pushed changes back to t/16624.

comment:12 Changed 12 months ago by kkrzton

Status: reviewreview_passed

LGTM!


The only case in which it will not work is when the same rule is added manually (without colorbutton plugin), but then we should assume that other ACF rules fixing the issue also should be add manually.

What I meant here is that you can add ACF rule to allow background-color property to be preserved while pasting (without including colorbutton plugin). Then you may expect that copying text with background color from Word will preserved the color while this rule is present. However, it won't because pasted content will have background property which will be removed.

But I also assumed here that if someone manually sets ACF config like this, he may easily adjust it also to preserve background property.

comment:13 Changed 12 months ago by kkrzton

Resolution: fixed
Status: review_passedclosed

Merged to master with 22b22b3.

comment:14 Changed 12 months ago by Marek Lewandowski

Description: modified (diff)
Summary: [PFW] Convert span background to background-color if possible[PFW] Improve integration with Color Button plugin

Clarified description.

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