[Edge] List gets pasted as a set of paragraphs

[Jakub Ś]

Steps to reproduce

1. Open attached file and copy the list
2. Paste the list into CKEditor 4.6

Expected result

List with ul/ol li

Actual result

Paragraphs:

<p style="margin-left:48px; margin-right:0px"><span style="color:#000000"><span style="font-family:Calibri"><span style="font-size:medium">1.</span></span></span><span style="color:#000000"> </span><span style="color:#000000"><span style="font-family:Calibri"><span style="font-size:medium">Test1</span></span></span></p>

<p style="margin-left:48px; margin-right:0px"><span style="color:#000000"><span style="font-family:Calibri"><span style="font-size:medium">2.</span></span></span><span style="color:#000000"> </span><span style="color:#000000"><span style="font-family:Calibri"><span style="font-size:medium">Test2</span></span></span></p>

<p style="margin-left:48px; margin-right:0px"><span style="color:#000000"><span style="font-family:Calibri"><span style="font-size:medium">3.</span></span></span><span style="color:#000000"> </span><span style="color:#000000"><span style="font-family:Calibri"><span style="font-size:medium">test3</span></span></span></p>

Other details (browser, OS, CKEditor version, installed plugins)

Problem can be reproduced on Edge from CKEditor 4.6.

[Marek Lewandowski]

The problem is that Edge doesn't paste any usable meta information for Word-sourced content. You can see it ​in extracted fixture.

At first, the only thing allowing to figure out that it's a list is parsing numbering +  s - but that's not a viable solution, as Word will not put &nbsps if list item margin (space between numbering and list text) was set to 0.

Fixture pushed to ​t/16682 .

[Tade0]

Added some heuristics that should relatively reliably detect lists pasted from Edge.

TODO: Make this deactivatable via config option.

Changes pushed to ​branch:t/16682.

[Tade0]

Added config, fixed some minor issues.

Changes pushed to ​branch:t/16682.

[Tade0]

There's an Edge case in Edge(ticket:16745) that is required to be covered by this ticket. Reassigning.

[Tade0]

Improved Edge list item detection. Test for this ticket will break in ticket:16745.

[Marek Lewandowski]

Rebased ​branch:t/16682 to the latest master branch.

Pushed some fixes to the branch.

General

• First off ​http://tests.ckeditor.dev:1030/tests/plugins/pastefromword/generated/tickets6 is failing.
• This is not word2013 that you were testing, but word2016 under disguise of word365.
• There's ​commented out code commited.
• We don't ship code with @todo ​markers.
• ​Replacing CKEDITOR.env.edge is not a good way to create these tests.
  1. Instead of adding new test suite edge.js, simply reuse generic.j (add word2016 version and edge browser).
  2. Use _should.ignore to ignore edge tests that relies on edge workarounds. E.g.

     _should: {
     	ignore: {
     		'test Ordered_list_multiple word2016 edge': !CKEDITOR.env.edge
     	}
     }
     

• ​filter - did you mean ​CKEDITOR.tools.array.filter?
• ​map:
  • You're often using it simply as a ​forEach.
  • There's ​one usage which uses map - feel free to add polyfill to CKEDITOR.tools.array. Just in case, I'll add that it must be covered with unit tests.
• I don't see any test for disabled pasteFromWord_heuristicsEdgeList config.

Code

Code style:

• With new code we're not initing multiple vars ​in a single line of code.
• ​Inconsistent method name - why uppercased?

Docs

• ​There's no doc for `CKEDITOR.plugins.pastefromword.heuristics` It also must contain @since tag.
• You want to put ​at least one line break between method description and doc tags.
• We're not ​documenting methods in a single line.

I'll stop the review here, as it's enough to things to fix before real review.

[Tade0]

Corretcted errors, added some methods and tests.

Changes pushed to ​branch:t/16682.

[Marek Lewandowski]

Code style was pretty much good, the general idea about the fix looks to be ok but there were quite a few issues with execution. The code would be executed for non-edge browsers in certian situation, list levels were resolved to a negative number etc.

I've fixed this, added an extra fixture that was failing for old solution (it created 2 lists for the same thing, and levels were wrong), added unit tests for heuristic logic.

There are still some things to be adressed:

• tests needs to be reorganized as it's getting messy there
• when ACF is disabled list items get margin, which is not something we want to have

Both of these needs to be extracted into a seaprate issues.

Rebased and pushed changes to ​t/16682 branch for a R.

[Tade0]

List with indent changed in the Layout tab in Word

[Tade0]

Attached a file that causes the following error:

SCRIPT5007: Unable to get property 'attributes' of undefined or null reference
default.js (764,4)

This regression is introduced somewhere after this commit: ​https://github.com/cksource/ckeditor-dev/commit/6d506dc5b854a971ec8c6a565c8cb76420ada079

[Tade0]

Other things:

1. guessIndentationStep could conceivably return a negative value(level 1 item after level 3 item). This may be the case with the error described in the previous post. My guess is that from this point on:
   ​https://github.com/cksource/ckeditor-dev/blob/t/16682/plugins/pastefromword/filter/default.js#L1655
   the code does not account for the fact, that list margins not proportional.

2. Issues with CKEDITOR.tools.array.unique(these are distractions really as this function is not used):
   • it has O(N²)complexity - with a comparator function this could(and should) be reduced to O(Nlog₂N).
   • uses forEach instead of reduce.

[Marek Lewandowski]

I've checked given fixture and the issue is there.

So the thing is that this fixture produces an edge case magin:0, which made val equal to 0 ​inside of levels lookup map having levels equal to 0 it was messing up the transformations later on.

The fix here is to ​bump every level if a case like this happens.

I've added proper integration and unit test for this. Also there's an integration test for negative margins (which are possible to create in Word).

[Marek Lewandowski]

Since array.unique method is no longer used, there's no reason to keep it around. I've pushed some more changes to cleanup the code and gave it a review.

Fixed with ​git:7f3050f.