Opened 10 years ago
Last modified 10 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)
Change History (13)
comment:1 Changed 10 years ago by
Owner: | set to Piotr Jasiun |
---|---|
Status: | new → assigned |
comment:2 Changed 10 years ago by
Status: | assigned → review |
---|
comment:3 follow-up: 4 Changed 10 years ago by
Status: | review → review_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 Changed 10 years ago by
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 10 years ago by
Status: | review_failed → review |
---|
I refactored moveToText
and fixed tests. Changes in t/12750b.
comment:6 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Fixed on master with git:a5f8c73.
comment:7 Changed 10 years ago by
Milestone: | → CKEditor 4.4.7 |
---|
Changed 10 years ago by
Attachment: | 12750.docx added |
---|
Changed 10 years ago by
comment:8 Changed 10 years ago by
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.
comment:9 Changed 10 years ago by
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.
comment:10 Changed 10 years ago by
Milestone: | CKEditor 4.4.7 |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
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 10 years ago by
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).
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.