Opened 5 years ago

Last modified 4 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 5 years ago.
12750.png (809 bytes) - added by Wiktor Walc 5 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: newassigned

comment:2 Changed 5 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 5 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 5 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 5 years ago by Piotr Jasiun

Status: review_failedreview

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

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

Resolution: fixed
Status: reviewclosed

Fixed on master with git:a5f8c73.

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

Milestone: CKEditor 4.4.7

Changed 5 years ago by Wiktor Walc

Attachment: 12750.docx added

Changed 5 years ago by Wiktor Walc

Attachment: 12750.png added

comment:8 Changed 5 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 5 years ago by Wiktor Walc (previous) (diff)

comment:9 Changed 5 years ago by Wiktor Walc

Uh, forget my last comment, It turned out that during pasting "Paste from Word" was skipped - reported in #12765.

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

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

comment:10 Changed 4 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 4 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 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy