Opened 9 years ago

Last modified 9 years ago

#12750 reopened Bug

Paste from Word: strikethrough and underscore should have the same color as font

Reported by: Piotr Jasiun Owned by: Piotr Jasiun
Priority: Normal Milestone:
Component: General Version:
Keywords: Cc:

Description

Maintain color of strikethrough and underscore the same as font color of the text when copying from Word into CKEditor.

TC:

  • In MS Word, type in “Test font” with font family = Calibri, size = 14px and font color = red and then underscore and strikethrough the words “Test font”.
  • Copy from Word and paste the same in CK Editor. The font color remains red as expected but the underscore and strikethrough changes to default black.

Solution: the color of strikethrough (<s>) and underline (<u>) will be maintained, but only if colors, underline, strikethrough are applied to exactly the same selection (portion of text).

Attachments (2)

12750.docx (12.3 KB) - added by Wiktor Walc 9 years ago.
12750.png (809 bytes) - added by Wiktor Walc 9 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 9 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: newassigned

comment:2 Changed 9 years ago by Piotr Jasiun

Status: assignedreview

I have implemented filter which move strike and underscore elements close to the child text nodes. Works in all tested cases. Changes in t/12750b.

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

Status: reviewreview_failed

The patch is amazing. Due to the nature of the MSWord filter, I expected something much uglier. Kudos for finding so simple solution.

The condition needs some improvements:

  • hasNotTextChild -> hasNoTextChild
  • The hasNoTextChild variable actually means more, because the element also has to have only one child,
  • But the above is not important, because instead of checking:
    hasManyChildren || hasOneNonTextChild
    

I think we could check the opposite:

!( children.length == 0 || children.length == 1 && children[ 0 ].type == TEXT )

in other words - we don't want to move an element if:

isEmpty || hasTextOnly

I find this condition much clearer (and the variable names make sense :D).

Other:

  • element.moved -> element._.moved
  • Tests fail on IEs (I checked 8, 11) because of styles comparison problems. Avoid hex colors and .0 values.

comment:4 in reply to:  3 Changed 9 years ago by Piotr Jasiun

Replying to Reinmar:

  • hasNotTextChild -> hasNoTextChild
  • The hasNoTextChild variable actually means more, because the element also has to have only one child,

This is why this variable is called hasNotTextChild instead of hasNoTextChild: it means that element has child and the type of the child is "not text". Anyway the name of variable is definitely bad if I need to explain it so I will change.

comment:5 Changed 9 years ago by Piotr Jasiun

Status: review_failedreview

I refactored moveToText and fixed tests. Changes in t/12750b.

comment:6 Changed 9 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Fixed on master with git:a5f8c73.

comment:7 Changed 9 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.7

Changed 9 years ago by Wiktor Walc

Attachment: 12750.docx added

Changed 9 years ago by Wiktor Walc

Attachment: 12750.png added

comment:8 Changed 9 years ago by Wiktor Walc

The attached document gave me

<p><s><u><span style="color:red; font-size:14pt">Test font</span></u></s></p>

Word 2010, IE11 / Win 7.

Last edited 9 years ago by Wiktor Walc (previous) (diff)

comment:9 Changed 9 years ago by Wiktor Walc

Uh, forget my last comment, It turned out that during pasting "Paste from Word" was skipped.

When using the "Paste from Word" button everything worked as expected.

Version 0, edited 9 years ago by Wiktor Walc (next)

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

Milestone: CKEditor 4.4.7
Resolution: fixed
Status: closedreopened

We're struggling with #12842 - a regression introduced by this fix. Since it's unlikely that with the current PFW implementation we'll be able to fix #12842 in a clean way I decided to revert #12750. We may still eventually close both tickets, but slightly better solution to #12842 needs to be found than the one we have now

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

I pushed branch:t/12750 with the current state from the point at which we previously merged it to master.

I also pushed branch:t/12750b with the fix proposed for #12842 (in the comment no. 5).

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy