Paste from word should only normalize input data

[Piotrek Koszuliński]

Transformations and clean up are done by #9829 and #9989. Thus, PFW has to be rewritten so it won't double these core features.

Spec: ​https://gist.github.com/Reinmar/cc012b20bf37be84a523 (WIP)

Issues fixed by new implementation

#14660

[Anna Tomanek]

Adding #10267 as a test case for this issue.

[Piotrek Koszuliński]

This ticket is about rewriting PFW from scratch and this is immense amount of work, so it will not be included in 4.2.

[Piotrek Koszuliński]

#10327 should go before this ticket.

[Piotrek Koszuliński]

<td style="background:Yellow">...</td>
or:
<span style="background:Yellow">...</span>

We need transformation that will extract background-color which we understand, from background which we remove. We should add these transformations for more CSS properties. I'm not only sure how can we automatically create them from e.g. config.colorButton_backStyle. We may need to add new settings for styles which would guide the filter.

Duplicate: #10364.

[Piotrek Koszuliński]

Another rooms for improvements can be find in the way we fixed #10348:

• transformations could accept elements list instead of one element only,
• transformations for align attribute on blocks is needed.

[Jakub Ś]

#10558 was marked as duplicate.

[Frederico Caldeira Knabben]

We’re not supposed to drastically change the way things are done right now. Most of the changes are to happen in the cleanup logic.

The following are some notes about how things (will) work.

• Basic workflow:
  1. Listens for the “paste” with priority “3”
  2. Sniffs the paste date for word stuff.
  3. If from word, calls CKEDITOR.cleanWord.
  4. Updates the paste event data with the cleaned data.

• It allows to have custom "cleanWord” implementations through configuration and on-demand file loading. I’m unsure about the point about this as one could eventually simply write a plugin that would override the default plugin behavior. Anyway, we don’t have to change this right now and it can be a v5 thing.

• The default cleanWord implementation will not do the deep cleanup it does today. It’ll instead “normalize” the HTML in a way that it has proper semantics, using proper HTML features. The cleanup part will be left to ACF. This is all about this ticket, in fact.

• The default cleanWord implementation is based on data processing, mainly.

• What to do with the current configurations:
  • pasteFromWordPromptCleanup: I don’t feel that this is necessary any more… actually it may still exist but by default it may depend on ACF, if it’s enabled.
  • pasteFromWordRemoveFontStyles: we can keep this
  • pasteFromWordNumberedHeadingToList: ???
  • pasteFromWordRemoveStyles: I have the impression that this is not needed any more. Only if we want to keep it so one could decide to have cleaner paste from Word, even if the editor support style features.

So the current idea is developing tests and write the cleanup logic from scratch, reusing what’s is reusable from the current implementation.

[Wim Leers]

Any idea on the ETA?

This is one of the things we need to ship Drupal 8. There's no beta yet, so there's still some time, but we definitely need to close ​https://drupal.org/node/1955300 before release.

[Piotrek Koszuliński]

The ETA is 4.5 (so July or August - we may still change ETA for 4.5). More delay is very unlikely because of our plans regarding 4.5 and 5.

[Piotrek Koszuliński]

We're sad when we have to do that, but we weren't able to find resources for Paste From Word for the last couple of months. We have to release the drag and drop and file upload support which is ready for in ~75% as soon as possible, so I'm postponing PFW to the next major release.

[Piotrek Koszuliński]

This may be still a useful resource ​https://msdn.microsoft.com/en-us/library/Aa155477?f=255&MSPPError=-2147217396

[Piotrek Koszuliński]

I wrote a first draft of the spec (or rather guidelines): ​https://gist.github.com/Reinmar/cc012b20bf37be84a523

[Wim Leers]

Awesome!

[Tade0]

The status of the work is as follows:

A set of virtual machines has been deployed, containing the most crucial combinations of MS Word/IE versions. An application is available on each of the VMs that accepts *.docx files as an input and outputs the HTML that would normally be generated when the contents of a Word file are pasted into the editor. An additional command-line utility enables remote access to the applications.

The point of this exercise is to automatically generate fixtures for unit tests. A growing number of example *.docx files is now available for processing.

Next step: Creating the first batch of tests using the generated fixtures.

[Tade0]

Status update:

Most of the core features of the plugin have been rewritten, as confirmed by 17 test cases(each one checked with Word 2007, 2013 and browsers: Chrome, Firefox, IE8, IE11).

[Piotrek Koszuliński]

I made a quick review of the code that you pushed to ​branch:t/9991. Here's the result:

1. The fixtures MS Word files are missing in the repo.
2. We need instructions on how to generate HTML fixtures from the MS Word files (not public, because it requires our infrastructure).
3. The branch must be rebased before putting it on review.
4. Tests should not be inside tests/tickets/9991. The proper place is inside PFW plugin's dir. Note that they should be separeted from the tests for the legacy filter.
5. Tests for the previous filter should be tagged and configured to use the previous filter (they are failing right now).
6. Remember about strict mode.
7. Don't put a blank line at the beginning of a function or block (default.js L14, L336).
8. Separate blocks with a blank line (applies to blocks like elements, elementNames, all element names in default.js).
9. setSymbol() (switch block) – what if a list start with item "3" so no b., no II. etc. Using regexp than case item statements will be more flexible.
10. L138 children[0] => children[ 0 ] (no JSCS rule for that because the existing one is broken).
11. L159-177 - use one nope function.

However, more important is what has to be done with the task in general:

1. We need to make sure that the new filter is at least as good as the old one. AFAICS the existing tests are oriented around the simple cases. We need to create tests for at least some of the fixed bugs in the old PFW filter. So the first thing that we need to do is to stabilise the new filter.
2. The second thing is to actually implement the spec that I wrote (comment:18), because currently the new filter works in nearly the same manner as the old one, so it normalize the wordy-HTML to standard-HTML and then also it does the transformation and filtering which it should not do. So we need to take the current filter and move some of its code to ACF's transformations and filtering. This task can be done in a separate ticket, because this will be mostly transparent for the users. It will however make the PFW feature more configurable (through the ACF's settings).

[Tade0]

Replying to Reinmar:

I made a quick review of the code that you pushed to ​branch:t/9991. Here's the result:

1. The fixtures MS Word files are missing in the repo.
2. We need instructions on how to generate HTML fixtures from the MS Word files (not public, because it requires our infrastructure).
3. The branch must be rebased before putting it on review.
4. Tests should not be inside tests/tickets/9991. The proper place is inside PFW plugin's dir. Note that they should be separeted from the tests for the legacy filter.
5. Tests for the previous filter should be tagged and configured to use the previous filter (they are failing right now).
6. Remember about strict mode.
7. Don't put a blank line at the beginning of a function or block (default.js L14, L336).

We need a JSCS rule for that then, spanning the whole project.

1. Separate blocks with a blank line (applies to blocks like elements, elementNames, all element names in default.js).

Same as above.

1. setSymbol() (switch block) – what if a list start with item "3" so no b., no II. etc. Using regexp than case item statements will be more flexible.

Simply using regexps doesn't solve this. What if a list starts with an "i."? No way of telling if it's a roman numeral or a letter. And this is only the tip of the iceberg. This function was designed for the limited scope that this iteration of PFW has. I'll correct it as soon as we have the proper test cases.

[Piotrek Koszuliński]

Note that introducing some JSCS rules may require fixing 1000 files. Although, simple stuff like this may, most likely, be fixed with automatic JSCS fixer.

[Piotrek Koszuliński]

And feel free of course to propose such changes :)

[Piotrek Koszuliński]

Simply using regexps doesn't solve this. What if a list starts with an "i."? No way of telling if it's a roman numeral or a letter.

It's an edge case. Check first for roman literars and later for alpha numeric case and you'll match well in satisfying number of cases.

[Tade0]

Rebased and fixed. Changes pushed to ​branch:t/9991a.

[Piotrek Koszuliński]

I've just checked the case from #7262 out of curiosity and the result is really bad in Chrome:

<pre style="text-align:justify">
<!--[if--><span style="font-family:symbol"><span style="font-size:12.0pt">&middot;      </span></span><!--[endif]--><span style="font-family:tahoma"><span style="font-size:12.0pt">Text1</span></span><!--![endif]--><!--![if--></pre>

<p><!--[if--><!--[endif]--></p>

<pre style="text-align:justify">
<!--[if--><span style="font-family:symbol"><span style="font-size:12.0pt">&middot;      </span></span><!--[endif]--><span style="font-family:tahoma"><span style="font-size:12.0pt">Text2</span></span><!--![endif]--><!--![if--></pre>

<p><!--[if--><!--[endif]--></p>

<pre style="text-align:justify">
<!--[if--><span style="font-family:symbol"><span style="font-size:12.0pt">&middot;      </span></span><!--[endif]--><span style="font-family:tahoma"><span style="font-size:12.0pt">Text3</span></span><!--![endif]--><!--![if--></pre>

<p><!--[if--><!--[endif]--><!--![endif]--><!--![if--><!--![endif]--><!--![if--><!--![endif]--><!--![if--></p>

As I mentioned – we need A LOT more tests before we're able to accept this new filter.

I remember we were discussing generating fixtures for all the docx files that we have. How's that work going?

[Marek Lewandowski]

Problem with leaking comments seems to be fixed. Please cleanup unused branches on remote, rename t/9991b to t/9991 and remove all other branches from the remote.

Did some reviewing last couple of days and here are the notes so far:

problems:

• failing tests,
• font plugin:
  • ACF selectors must be as narrow as possible - ​this one essentially allows to put any mess inside span[style]. Same thing for other plugins.
  • add new transformation to existing ​font plugin features instead ​defining new entries
  • transformations should ensure that target transformation can be applied. Assune a situation when ACF is set in a way that it allows font[http://docs.ckeditor.com/#!/api/CKEDITOR.filter-method-addTransformations face] but does not allow span{font-family} with current implementation it would be converted to span (no matter what) and then... removed by ACF. You can do this with [check property].

• ​plugin.js it should assign element.styles[ 'text-align' ],
• ​default.js use ​hasClass instead,
• ​default.js - I believe returning false does filter out the element, so you don't need to rely on internals (so that removing element name deletes the elem),

style:

• empty comment in ​default.js,
• instead disabling ​default.js jshint error use element.attributes[ 'class' ] for reserved keywords,
• ​default.js always use braces + convertToFakeListItem should go to next line,
• simply use ifs instead of doing one-liner expression like [in plugin.js](​https://github.com/cksource/ckeditor-dev/blob/32561ba217c5bf607f21e7be2241c00fe28284fd/plugins/font/plugin.js#L215)

[Tade0]

Fixed, changes pushed to ​branch:t/9991.

One thing that hasn't been changed was:

• ​default.js use ​hasClass instead,

hasClass wouldn't work here, because the aim here is not to do an exact match, but to check whether there's a class that contains the string MsoListParagraph.

[Marek Lewandowski]

• you shouldn't create new CKEDITOR_MOCK global variable. Instead create CKEDITOR.plugins.pastefromword static namespace and put methods over there. However mark members that you don't want to be used outside of pfw as "@private".
  • Now within this namespace, create members like CKEDITOR.plugins.pastefromword.lists, where all list-related members should go.
  • createLists is too long and does too many things. You should extract multiple parts:
    • ​fake list item transfomration part should be extracted to retainListItems where it would return listElements arary.
    • ​list grouping code should also be extracted into a separate member.

• we can't be so eager to drop some attributes from resetStyles
  • ​white background - you could easily produce nested content that will be broken by this rule.

See whiteBg.png

• same thing goes for ​font color

• ​this switch can be simplifed down to if. I can see that idea behind it might be for simplicity to extend down the road, but we can keep it simple here, refactorization will happen as needed.
• let's ​simplify it down to normalizedStyles( element ) || false;
• there area 2 "13590CkEditor-NumberList" directories in ​Tickets directory with a different casing. It looks like redundancy.

[Tade0]

Applied the changes from comment:32, also implemented the rest of the features that were required to cover all the closed tickets concerning PFW.

Some cases have inputs(chiefly from IE11) that still create conflicts. These tickets are: 7131, 7209, 7872, 7581("stupid list" case).

Most probably there is no real way to achieve the desired output in all these cases, so eventually we will have to decide which are more important and cover them.

Changes pushed to: ​branch:t/9991.

EDIT: rebased with major branch.

[Marek Lewandowski]

Note: It make sense to check if TC from #14544 is still valid here.

[Marek Lewandowski]

List causing an exception

[Marek Lewandowski]

I've been reviewing PFW again for a while now, and I found a case where pasting content would cause an exception. It's easy to fix for me, however I need to prepare proper tests to prevent us from regressions.

@Tade0 could you please provide some sort of instructions on generating a proper Word test?

I'm adding the docx fixture as an attachement ("broken-list.docx").

[Tade0]

Manual for the client side of the PFW fixture generator.

[Tade0]

I've included an attachment that should serve as a guide how to generate tests.

[Marek Lewandowski]

Test Custom_list_markers

I've fixed a bug where this TC would cause an exception. With that I discovered couple of things:

• List should be converted to unordered list (it's identified as such in MS Word). Instead it's converted to two ordered lists, and list gets start value 19. That's actually a regression to a current implementation.
• Since we're not supporting custom markers yet, we should filter out custom markers. Those could be recognized by:
  • Checking if src starts with "​file://" protocol and have an alt equal to "*".

[Tade0]

Added this TC and made changes so that there is an unordered list there.

Changes pushed to ​branch:t/9991.

As for the src starting with file:// - regular images are also pasted like that, so it can't be counted as a distinguishing feature. That alt is uncommon enough though.

[Marek Lewandowski]

Replying to Tade0:

As for the src starting with file:// - regular images are also pasted like that, so it can't be counted as a distinguishing feature. That alt is uncommon enough though.

Yes, this is why both conditions must be met.

[Tade0]

Replying to m.lewandowski:

Replying to Tade0:

As for the src starting with file:// - regular images are also pasted like that, so it can't be counted as a distinguishing feature. That alt is uncommon enough though.

Yes, this is why both conditions must be met.

Thing is, it's extremely hard not to meet that first condition, therefore it doesn't really check anything(it would still be possible to create a test case which would break here).

Alternatively, as an additional condition it could be checked whether the image is wrapped in a <span> with style="mso-list:Ignore" - that's a telltale sign that the image is used as a symbol.

[Tade0]

Implemented the solution described in the previous comment.

Changes pushed to ​branch:t/9991.

[Tade0]

Since the tests are prone to breaking due to the sheer amount of them I'll cut them into manageable pieces.

[Marek Lewandowski]

Fixed with ​git:f2af0353f8fb20231c7ca6de080c6cad273195eb merged to major.