Provide support for pasting lists from google docs #72

msamsel · 2019-07-30T09:57:42Z

Suggested merge commit message (convention)

Feature: Provide support for pasting lists from Google Docs. Closes ckeditor/ckeditor5#2499 .

Additional information

When you copy-paste content from Safari, sometimes there is nothing to recognize that content cames from Google Docs. For example, try to copy-paste content from table.
That's why I create generic normalizer, which is applied if no other normalizer was run.
I saw 2 options here, either allow on multiple normalizers to run over the content or create a normalizer which will be executed if no other is found. I decide for the second option to not complicate code too much and have sure that only one normalizer will modify the data.content.

coveralls · 2019-07-30T10:01:50Z

Pull Request Test Coverage Report for Build 271

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 252:	0.0%
Covered Lines:	208
Relevant Lines:	208

💛 - Coveralls

msamsel · 2019-07-30T10:08:42Z

@Mgsy can you take a look for it? I copy-paste some general content and seems to work fine. However, maybe there is some strange browser or content structure, which might not be converted to lists properly.

… properly recognized byt other normalizers. What can happen for partial copy-paste from Safari.

Mgsy · 2019-07-30T13:38:32Z

It crashes if a pasted list item is indented more than once.

Uncaught TypeError: Cannot read property 'childCount' of null
    at moveNestedListToListItem (common.js:48)
    at moveNestedListToListItem (common.js:52)
    at moveNestedListToListItem (common.js:52)
    at GoogleDocsNormalizer.execute (googledocsnormalizer.js:36)
    at Clipboard.editor.plugins.get.on.priority (pastefromoffice.js:69)
    at Clipboard.fire (emittermixin.js:207)
    at Document.listenTo.priority (clipboard.js:78)
    at Document.fire (emittermixin.js:207)
    at Document.handleInput (clipboardobserver.js:51)
    at Document.fire (emittermixin.js:207)

Clipboard HTML:

<html>
<body>
<!--StartFragment--><meta charset="utf-8"><b style="font-weight:normal;" id="docs-internal-guid-51600ef9-7fff-81d2-c69b-dfa6f7e8dedc"><ol style="margin-top:0pt;margin-bottom:0pt;"><li dir="ltr" style="list-style-type:decimal;font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;margin-left: 36pt;"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt;"><span style="font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;white-space:pre-wrap;">Test</span></p></li><ol style="margin-top:0pt;margin-bottom:0pt;"><ol style="margin-top:0pt;margin-bottom:0pt;"><li dir="ltr" style="list-style-type:lower-roman;font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;margin-left: 36pt;"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt;"><span style="font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;white-space:pre-wrap;">test</span></p></li></ol></ol></ol></b><!--EndFragment-->
</body>
</html>

msamsel · 2019-07-30T13:45:19Z

It crashes if a pasted list item is indented more than once.

OMG why they even allow on such a strange structure 😱. What is worse, that CKEditor doesn't support such a structure. And we could either remove those extra indentions or remove entire node with all children below :(

Mgsy · 2019-07-30T13:47:20Z

What is worse, that CKEditor doesn't support such a structure. And we could either remove those extra indentions or remove entire node with all children below :(

The same problem occurs for Word, actually, in this PR you'll find two proposals how to solve this issue - #53. We agreed that this kind of structures should be normalized.

msamsel · 2019-07-30T15:00:35Z

I wonder about the normalization process in some specific situation:

This content has the following structure:

<ol>
    <li>A1</li>
    <ol>
        <ol>
            <ol>
                <li>B4</li>
            </ol>
            <li>C3</li>
            <ol>
                <li>D4</li>
            </ol>
        </ol>
        <li>E2</li>
    </ol>
</ol>

Element A1 possesses 2 wrong elements: B4 and C3.
D4 - is a child of C3 and E2 is a child of A1.

From the first look on task: #53 (comment) proposed normalization moves element B4 as a child of A1. However, it also ⚠️ makes element C3 a child of B4 ⚠️.
So the structure after normalization would look like:

Elements B4 and C3 are positioned incorrectly then both should become siblings and should be located as children of A1. So the correct structure should look like:

@Reinmar, @Mgsy WDYT?

// cc @scofalik last time you were not asked about your opinion. I won't repeat that mistake! WDYT?

msamsel · 2019-07-31T13:31:57Z

I've implemented the 2nd approach, which seems to be less time-consuming.
So right now if there is a nested list then might be 2 options:

If it has a preceding list item sibling, then given list became a nested list inside the previous list item.
If the first child of the list is another list, then the nested list is unwrapped. So all children of the nested list became children of the current list.

jodator · 2019-07-31T14:32:20Z

The normalization seems to look OK :) Probably @Mgsy could verify that. I can see that if user paste sub-list (ckeditor/ckeditor5#1930) nothing gets pasted.

Error in the console:

common.js:62 Uncaught TypeError: Cannot read property 'childCount' of null
    at moveNestedListToListItem (common.js:62)
    at moveNestedListToListItem (common.js:74)
    at GoogleDocsNormalizer.execute (googledocsnormalizer.js:36)
    at Clipboard.editor.plugins.get.on.priority (pastefromoffice.js:69)
    at Clipboard.fire (emittermixin.js:207)
    at Document.listenTo.priority (clipboard.js:78)
    at Document.fire (emittermixin.js:207)
    at Document.handleInput (clipboardobserver.js:51)
    at Document.fire (emittermixin.js:207)
    at ClipboardObserver.fire (domeventobserver.js:96)

pasted content:

<meta http-equiv="content-type" content="text/html; charset=utf-8"><meta charset="utf-8"><ul style="margin-top:0pt;margin-bottom:0pt;" id="docs-internal-guid-8a672c83-7fff-1b35-788b-9afb3dcf7487"><ul style="margin-top:0pt;margin-bottom:0pt;"><ul style="margin-top:0pt;margin-bottom:0pt;"><ul style="margin-top:0pt;margin-bottom:0pt;"><ul style="margin-top:0pt;margin-bottom:0pt;"><ul style="margin-top:0pt;margin-bottom:0pt;"><ul style="margin-top:0pt;margin-bottom:0pt;"><li dir="ltr" style="list-style-type:disc;font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt;"><span style="font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;white-space:pre-wrap;">E</span></p></li></ul></ul><li dir="ltr" style="list-style-type:circle;font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt;"><span style="font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;white-space:pre-wrap;">F</span></p></li></ul></ul></ul></ul></ul>

msamsel · 2019-07-31T14:54:04Z

@jodator When I create normalize I wondered if a situation like this might happen. I couldn't find a way to generate content which has nested tags from the beginning :D. And now I see it's much easier than I thought ;)

jodator · 2019-08-01T12:05:48Z

You can find more details about why this normalizer was introduced in "Additional information" section of PR.

Oh sorry I've missed that - so remove the ifs ATM as in my comments there. We decide what to do with this problem later.

…alizer.

jodator

Almost there ;) Some minor docs improvements, extract unwrapping element to the engine's UpcastWriter and the Safari case is still on my mind as I would try to make things as specific as possible.

src/filters/list.js

jodator · 2019-08-02T08:11:32Z

src/normalizers/genericnormalizer.js

+ *
+ * @implements module:paste-from-office/normalizer~Normalizer
+ */
+export default class GenericNormalizer {


Does it happen only on Safari? If so I'd still ad some env checking to be sure that this normalizer doesn't fire up too often. @mlewand WDYT? OTOH this normalizer will always fix pasted lists (even from other sources) so it may be beneficial.

I see two routes:

Make it run only for Safari and name it more specific, like: SafariListNormalizer

Make it general list fixer - the I'd name it NestedListNormalizer and maybe add a check if pasted content has broken list (AFAICS ul></ul, /p></li and /li><ul could be checked - with ul|ol variants check) - but the check idea is something to give another though if it makes sense.

Co-Authored-By: Maciej <[email protected]>

…ile.

jodator

I've added small changes to the list.js file:

some re-organization (exported methods should be on top).
last list example was misaligned with the other two

Mateusz Samsel added 8 commits July 30, 2019 09:31

Add support for pasting lists from google docs.

173e44d

Add unit test for google docslist item filters.

7c4f7dc

Add data for autoamtic tests for pasted lists from google docs.

1111bea

Provide option to obtain model's data.

11cbe07

Fix typo in folder name.

348f3b6

Correct test files. Add entry point for tests, add test to dataset.

419ff3d

Add unit test for mixed lists.

3eda19c

Fix small typos.

4a566b2

msamsel requested a review from Mgsy July 30, 2019 10:02

mlewand requested a review from jodator July 30, 2019 12:27

Introduce generic nromalizer, to patch content which is wrong but not…

5bf77e7

… properly recognized byt other normalizers. What can happen for partial copy-paste from Safari.

Mgsy added the status:review- label Jul 30, 2019

Mateusz Samsel added 3 commits July 31, 2019 14:36

Add support and provide normalization for repeatedly nested lists.

f6ee361

Ad unit test for fixed filter.

2fb7ddb

Add new automatic test for repeatedly nested lists.

e8e12c9

msamsel removed the status:review- label Jul 31, 2019

jodator mentioned this pull request Jul 31, 2019

Can't paste indented bullets ckeditor/ckeditor5#1930

Open

jodator closed this Jul 31, 2019

jodator reopened this Jul 31, 2019

Mateusz Samsel added 2 commits August 1, 2019 10:09

Merge branch 'master' into t/69

6c9f2e9

Support mort cases with unwrapping lists.

975992e

Mateusz Samsel added 10 commits August 1, 2019 14:10

Move generic nromalizer as last one which will be always active.

3806a55

Move google docs list filter to list.js file.

16ca74f

Remove getModelData from manual test.

4aa476a

Remove unecessary if. There always should be at least one active norm…

70495e8

…alizer.

Rename unwrapParagraph and moveNestedLists... function.

68d155e

Move description to jsdoc.

b8eaff4

Utilize walker insted of recurence to fix lists indentation.

861ba50

Replace recurrence in unwrapParagraph with treewalker.

a6fa897

Simplify comment.

40c88a2

Correct names of generic normalizer in unit tests.

c9b208f

msamsel mentioned this pull request Aug 1, 2019

Introduce Paste Tools and Paste from GDocs ckeditor/ckeditor4#3257

Merged

2 tasks

msamsel requested a review from jodator August 2, 2019 07:13

jodator suggested changes Aug 2, 2019

View reviewed changes

jodator added the status:review- label Aug 2, 2019

msamsel and others added 4 commits August 2, 2019 12:00

Apply suggestions from code review

85c8764

Co-Authored-By: Maciej <[email protected]>

Utilize new API of upcast writer.

4a1dd43

Add comment to code and improve docs description.

383b387

Correct variables names and docs entries.

1a95ece

msamsel mentioned this pull request Aug 2, 2019

Add unwrapElement method to UpcastWriter ckeditor/ckeditor5-engine#1772

Merged

Remove generic normalizer.

676730c

msamsel requested a review from jodator August 2, 2019 14:00

msamsel removed the status:review- label Aug 2, 2019

jodator added 4 commits August 5, 2019 14:49

Fix examples alignment in the docs of fixListIndentation() function.

d2da715

Use isList() helper function in isNewListNeeded.

5513c92

Move unwrapParagraphInListItem & fixListIndentation to the top of a f…

3d467da

…ile.

Remove comment.

0a56b5e

jodator approved these changes Aug 5, 2019

View reviewed changes

jodator merged commit 6ad2a62 into master Aug 5, 2019

jodator deleted the t/69 branch August 5, 2019 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide support for pasting lists from google docs #72

Provide support for pasting lists from google docs #72

msamsel commented Jul 30, 2019 •

edited by pomek

Loading

coveralls commented Jul 30, 2019 •

edited

Loading

msamsel commented Jul 30, 2019

Mgsy commented Jul 30, 2019 •

edited

Loading

msamsel commented Jul 30, 2019

Mgsy commented Jul 30, 2019 •

edited

Loading

msamsel commented Jul 30, 2019 •

edited

Loading

msamsel commented Jul 31, 2019

jodator commented Jul 31, 2019 •

edited

Loading

msamsel commented Jul 31, 2019

jodator commented Aug 1, 2019

jodator left a comment

jodator Aug 2, 2019

jodator left a comment •

edited

Loading

Provide support for pasting lists from google docs #72

Provide support for pasting lists from google docs #72

Conversation

msamsel commented Jul 30, 2019 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

coveralls commented Jul 30, 2019 • edited Loading

Pull Request Test Coverage Report for Build 271

💛 - Coveralls

msamsel commented Jul 30, 2019

Mgsy commented Jul 30, 2019 • edited Loading

msamsel commented Jul 30, 2019

Mgsy commented Jul 30, 2019 • edited Loading

msamsel commented Jul 30, 2019 • edited Loading

msamsel commented Jul 31, 2019

jodator commented Jul 31, 2019 • edited Loading

msamsel commented Jul 31, 2019

jodator commented Aug 1, 2019

jodator left a comment

Choose a reason for hiding this comment

jodator Aug 2, 2019

Choose a reason for hiding this comment

jodator left a comment • edited Loading

Choose a reason for hiding this comment

msamsel commented Jul 30, 2019 •

edited by pomek

Loading

coveralls commented Jul 30, 2019 •

edited

Loading

Mgsy commented Jul 30, 2019 •

edited

Loading

Mgsy commented Jul 30, 2019 •

edited

Loading

msamsel commented Jul 30, 2019 •

edited

Loading

jodator commented Jul 31, 2019 •

edited

Loading

jodator left a comment •

edited

Loading