[Paste From Word] Various issues

[Piotrek Koszuliński]

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

Things to do:

• Review of all the tickets mentioned in ​http://dev.ckeditor.com/ticket/12740#comment:3. Which can be closed after 4.5.0, which are duplicates of the two following errors.
• All issues with ​http://dev.ckeditor.com/attachment/ticket/12740/WSGCN-3550%20Test%20document%20minimal.docx (such as error on Firefox, incorrect HTML pasted)
• All issues with ​http://dev.ckeditor.com/attachment/ticket/12740/CKEditor%20-%20Internal%20Error%20on%20Paste%20as%20CTRL-V(1).docx

Separate issues should be handled in separate sub-tickets.

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

[Tade0]

Replying to Reinmar:

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

Things to do:

• Review of all the tickets mentioned in ​http://dev.ckeditor.com/ticket/12740#comment:3. Which can be closed after 4.5.0, which are duplicates of the two following errors.

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'.

[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.

[Piotrek Koszuliński]

Though, it is an interesting finding of course.

[Piotrek Koszuliński]

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

[Tade0]

Added first test case for review.

Changes pushed to ​branch:t/13590.

[Piotrek Koszuliński]

• 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.

[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.

[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>.

[Tade0]

Word Viewer quirk

[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.

[Tade0]

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

[Tade0]

Fixed. Changes pushed to ​branch:t/13590.

[Tade0]

For case #2 (#8780) I'm examining this fragment: ​https://github.com/cksource/ckeditor-dev/commit/70c50306cc4081124debf935216700769d12cfe5#diff-019dde4f2aec1fcc8adfec50fae51e58R357

Which apparently causes a crash on Firefox.

[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.

[Piotrek Koszuliński]

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.

[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.

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.

Yeah, good call.

[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.

[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.

[Tade0]

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

[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.

[Tade0]

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

Changes pushed to ​​branch:t/13590.

[Tade0]

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

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

[Piotrek Koszuliński]

There are:

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

[Tade0]

Changes pushed to ​branch:t/13590a.

[Piotrek Koszuliński]

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

Merged to master with ​git:60cbbe4.