Opened 2 years ago

Closed 22 months ago

Last modified 22 months 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 Ś 2 years ago.
16682listWithMargin.docx (14.0 KB) - added by Tade0 22 months ago.
List with indent changed in the Layout tab in Word

Download all attachments as: .zip

Change History (22)

Changed 2 years ago by Jakub Ś

Attachment: Test1.docx added

comment:1 Changed 2 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1

comment:3 Changed 23 months ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:4 Changed 23 months 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.

This issue might only get fixed as Edge team will implement clipboard data access.

Fixture pushed to t/16682 .

Last edited 23 months ago by Marek Lewandowski (previous) (diff)

comment:5 Changed 23 months ago by Marek Lewandowski

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

comment:6 Changed 23 months 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 23 months ago by Tade0

Owner: set to Tade0
Status: confirmedreview

Added config, fixed some minor issues.

Changes pushed to branch:t/16682.

comment:8 Changed 22 months 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 22 months ago by Tade0 (previous) (diff)

comment:9 Changed 22 months ago by Tade0

Status: assignedreview

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

comment:10 Changed 22 months 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 22 months ago by Marek Lewandowski (previous) (diff)

comment:11 Changed 22 months ago by Tade0

Status: review_failedreview

Corretcted errors, added some methods and tests.

Changes pushed to branch:t/16682.

comment:12 Changed 22 months 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 22 months 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 22 months ago by Marek Lewandowski

Owner: changed from Tade0 to Marek Lewandowski
Status: reviewassigned

comment:15 Changed 22 months ago by Marek Lewandowski

Status: assignedreview

Changed 22 months ago by Tade0

Attachment: 16682listWithMargin.docx added

List with indent changed in the Layout tab in Word

comment:16 Changed 22 months 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 22 months 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 22 months 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 22 months ago by Marek Lewandowski (previous) (diff)

comment:19 Changed 22 months 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 22 months ago by Marek Lewandowski

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