Opened 3 years ago

Closed 3 years ago

#13590 closed Bug (fixed)

[Paste From Word] Various issues

Reported by: Piotrek Koszuliński Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.5.3
Component: Plugin : Paste from Word Version:
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

This ticket is a followup for http://dev.ckeditor.com/ticket/12740#comment:22

Things to do:

Separate issues should be handled in separate sub-tickets.

Edit: One more ticket to the list: #13616.

Attachments (2)

CkEditor-NumberList.docx (14.8 KB) - added by Wiktor Walc 3 years ago.
ckeditor-numberlist.docx (14.8 KB) - added by Tade0 3 years ago.
Word Viewer quirk

Download all attachments as: .zip

Change History (29)

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

Description: modified (diff)
Status: newconfirmed

comment:2 Changed 3 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:3 in reply to:  description Changed 3 years ago by Tade0

Replying to Reinmar:

This ticket is a followup for http://dev.ckeditor.com/ticket/12740#comment:22

Things to do:

I checked all the tickets on Chrome, Edge, IE, Firefox and the general conclusion is that Chrome discards or distorts some content and the other browsers either do the same and/or produce the error message 'It was not possible to clean up the pasted data due to an internal error'.

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

Note that the error may be caused by different issues. So do not generalise that all these tickets are related to one thing based on that. The same for Chrome actually.

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

Though, it is an interesting finding of course.

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

Description: modified (diff)

Let's add #13616 to the list of tickets to be tested.

comment:7 Changed 3 years ago by Tade0

Status: assignedreview

Added first test case for review.

Changes pushed to branch:t/13590.

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

Status: reviewassigned
  • Wrong commit name - should start from "Tests: "
  • You don't need 1.html having bender-include in 1.js.
  • Naming the test (and commit) "Word Viewer" does not make sense, because it's irrelevant, so it's better to not confuse others.

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

Naming the test (and commit) "Word Viewer" does not make sense, because it's irrelevant, so it's better to not confuse others.

OK, I understand that you got it from #9456 tests. There it made sense because there were more MS Word versions tested. In such case, question is - which MS Word version that Word Viewer mimics.

Changed 3 years ago by Wiktor Walc

Attachment: CkEditor-NumberList.docx added

comment:10 Changed 3 years ago by Wiktor Walc

Please take care of the attached file as well. When pasting from Word Viewer (PL locale, which results in classes like "ListParagraphCxSpPierwsze") into IE10 there are two different lists being created (wrong). Note: raw data does not contain <ol>/<li> elements.

When I pasted it from MS Word 2010 (EN locale) into IE11, I have a single list with two items (correct), raw data contains <ol>/<li>.

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

Changed 3 years ago by Tade0

Attachment: ckeditor-numberlist.docx added

Word Viewer quirk

comment:11 Changed 3 years ago by Tade0

Ignore the attachment added by me.

Anyway, after a few modifications the test case related to #11215 is now green on Chrome, Firefox and IE.

Changes pushed to branch:t/13590.

comment:12 Changed 3 years ago by Tade0

Status: assignedreview

comment:13 Changed 3 years ago by Tade0

Status: reviewassigned

My tests are green, but the whole test suite for the pastefromword plugin isn't.

comment:14 Changed 3 years ago by Tade0

Status: assignedreview

Fixed. Changes pushed to branch:t/13590.

comment:16 Changed 3 years ago by Tade0

As mentioned in #8780:

The problem was caused by the fact, that apparently handling the tab-stops style was done according to this spec: http://www.w3.org/People/howcome/t/970224HTMLERB-CSS/WD-tabs-970117.html

And the real value of that style is usually something similar to "36pt", "22px" etc.

Changes pushed to t/13590.

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

Status: reviewreview_failed
  1. Keep the comments mentioning ticket numbers before test functions.
  2. Format expected HTML, otherwise tests are very hard to read.
  3. You don't need the 13590/_assets/1.html file.
  4. I'm trying to understand this line:
// val = [left|center|right|decimal] <value><unit> Source: W3C, WD-tabs-970117.
var margin = val.match( cssLengthRelativeUnit ) && val.match( /0$|\d*\.?\d+\w+/ );

If, as you wrote yourself, the first value may be missing, then how this condition will behave? Since cssLengthRelativeUnit tries to match a number from the start, then it will return null, so the second regexp won't be executed.

Last edited 3 years ago by Piotrek Koszuliński (previous) (diff)

comment:18 in reply to:  17 Changed 3 years ago by Tade0

Replying to Reinmar:

  1. Keep the comments mentioning ticket numbers before test functions.
  2. Format expected HTML, otherwise tests are very hard to read.

Tried that - makes the test fail, especially when I add indentation.

  1. You don't need the 13590/_assets/1.html file.
  2. I'm trying to understand this line:
// val = [left|center|right|decimal] <value><unit> Source: W3C, WD-tabs-970117.
var margin = val.match( cssLengthRelativeUnit ) && val.match( /0$|\d*\.?\d+\w+/ );

If, as you wrote yourself, the first value may be missing, then how this condition will behave? Since cssLengthRelativeUnit tries to match a number from the start, then it will return null, so the second regexp won't be executed.

Yeah, good call.

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

Yeah, good call.

I've just found out that that change does exactly nothing. I checked out plugins/ to master and the second test is still passing.

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

I've just found out that that change does exactly nothing. I checked out plugins/ to master and the second test is still passing.

My mistake. That test fails on Firefox (it's green on Chrome) and your patch fixes it. However, the code is still incorrect as I pointed out in comment:17.

comment:21 Changed 3 years ago by Tade0

There is a chance that #12762 occurs, because part of the input is interpreted as a bullet symbol.

comment:22 Changed 3 years ago by Tade0

My hypothesis was true. If a <p> had a style mso-list:skip, then it was treated as a list and it's first span as a bullet symbol - hence the data loss.

Changes pushed to branch:t/13590.

comment:23 Changed 3 years ago by Tade0

Good news: test case 4 required only cosmetic changes in the tests.

Changes pushed to ​branch:t/13590.

comment:24 Changed 3 years ago by Tade0

Status: review_failedreview

Last two cases didn't require any additional modifications in the plugin.

A cleaned-up version was pushed to branch:t/13590a.

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

Status: reviewreview_failed

There are:

  • wrong ticket numbers in comments above tests.
  • no ticket numbers in the tags (PS. "editor" and "unit" tags can be removed).

comment:26 Changed 3 years ago by Tade0

Status: review_failedreview

Changes pushed to branch:t/13590a.

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

Resolution: fixed
Status: reviewclosed

Congrats! Fixed also #11215, #8780 and #12762. I was also able to close #9685 and #13616.

Merged to master with git:60cbbe4.

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