Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#16682 closed Bug (fixed)

[Edge] List gets pasted as a set of paragraphs

Reported by: Jakub Ś Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.6.2
Component: General Version: 4.6.0
Keywords: Edge Cc:

Description

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.

Attachments (2)

Test1.docx (14.3 KB) - added by Jakub Ś 3 years ago.
16682listWithMargin.docx (14.0 KB) - added by Tade0 3 years ago.
List with indent changed in the Layout tab in Word

Download all attachments as: .zip

Change History (22)

Changed 3 years ago by Jakub Ś

Attachment: Test1.docx added

comment:1 Changed 3 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1

comment:3 Changed 3 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:4 Changed 3 years ago by 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 + &nbsp;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 .

Version 0, edited 3 years ago by Marek Lewandowski (next)

comment:5 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1
Owner: Marek Lewandowski deleted
Status: assignedconfirmed

comment:6 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by Tade0

Owner: set to Tade0
Status: confirmedreview

Added config, fixed some minor issues.

Changes pushed to branch:t/16682.

comment:8 Changed 3 years ago by Tade0

Status: reviewassigned

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

Last edited 3 years ago by Tade0 (previous) (diff)

comment:9 Changed 3 years ago by Tade0

Status: assignedreview

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

comment:10 Changed 3 years ago by Marek Lewandowski

Status: reviewreview_failed

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

Pushed some fixes to the branch.

General

Code

Code style:

Docs

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

Last edited 3 years ago by Marek Lewandowski (previous) (diff)

comment:11 Changed 3 years ago by Tade0

Status: review_failedreview

Corretcted errors, added some methods and tests.

Changes pushed to branch:t/16682.

comment:12 Changed 3 years ago by Marek Lewandowski

Summary: List gets pasted as a set of paragraphs in Edge[Edge] List gets pasted as a set of paragraphs

comment:13 Changed 3 years ago by 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.

comment:14 Changed 3 years ago by Marek Lewandowski

Owner: changed from Tade0 to Marek Lewandowski
Status: reviewassigned

comment:15 Changed 3 years ago by Marek Lewandowski

Status: assignedreview

Changed 3 years ago by Tade0

Attachment: 16682listWithMargin.docx added

List with indent changed in the Layout tab in Word

comment:16 Changed 3 years ago by Tade0

Status: reviewreview_failed

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

comment:17 Changed 3 years ago by 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.
  1. Issues with CKEDITOR.tools.array.unique(these are distractions really as this function is not used):
    • it has O(N2)complexity - with a comparator function this could(and should) be reduced to O(Nlog2N).
    • uses forEach instead of reduce.

comment:18 Changed 3 years ago by Marek Lewandowski

Status: review_failedreview

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

Last edited 3 years ago by Marek Lewandowski (previous) (diff)

comment:19 Changed 3 years ago by Marek Lewandowski

Resolution: fixed
Status: reviewclosed

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.

comment:20 Changed 3 years ago by Marek Lewandowski

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