Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Columns width for tables pasted from MS Excel are not conveyed properly #14521

Closed
mlewand opened this issue Jul 3, 2023 · 4 comments · Fixed by #14720
Closed

Columns width for tables pasted from MS Excel are not conveyed properly #14521

mlewand opened this issue Jul 3, 2023 · 4 comments · Fixed by #14720
Assignees
Labels
package:table squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@mlewand
Copy link
Contributor

mlewand commented Jul 3, 2023

📝 Provide detailed reproduction steps (if any)

  1. Download Excel (3).xlsx file & open it in Excel.
  2. Select last three columns, like so:

  1. Paste it into https://ckeditor.com/docs/ckeditor5/latest/examples/builds-custom/full-featured-editor.html

✔️ Expected result

Cell widths are preserved (cell with "color on text and row" is wider than the other two).

Example, see 34.2.0 full featured editor example:

Note it doesn't retain styles, but that's not the scope of this issue. We care only about widths here.

❌ Actual result

Depending on version results were bad over time.

With column resize enabled it started to set wrong column widths, see table column resize demo in 35.0.1:

38.1.0 due to #14516  it looks even worse:

📃 Other details

  • Browser: Any
  • OS: Any
  • First affected CKEditor version: 35.0.1
  • Installed CKEditor plugins: TableColumnResize

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@mlewand mlewand added type:bug This issue reports a buggy (incorrect) behavior. package:table type:regression This issue reports a bug that was not present in the previous releases. labels Jul 3, 2023
@mlewand
Copy link
Contributor Author

mlewand commented Jul 3, 2023

It's safe to assume that column resize implementation is causing these problems.

Cols have invalid widths

For some reason each col has same width:

Individual cells do have proper widths

The widths here are looking fine, both format in points (style attr) and in px (width attr).

@Dumluregn Dumluregn added the squad:features Issue to be handled by the Features team. label Jul 31, 2023
@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Jul 31, 2023
@Dumluregn Dumluregn self-assigned this Aug 1, 2023
@Dumluregn
Copy link
Contributor

Dumluregn commented Aug 2, 2023

What happens here

Pasting the table from the first comment:

produces the following markup:

<table align="left" border="0" cellpadding="0" cellspacing="0" style="border-collapse:collapse;width:267pt" width="357">
    <colgroup>
        <col span="2" style="width:53pt" width="71">
        </col>
        <col style="mso-width-alt:6869;mso-width-source:userset;width:161pt" width="215">
        </col>
    </colgroup>
    <tbody>
        <tr height="35" style="height:26.0pt">
            <td class="xl66" height="35" style="height:26.0pt;width:53pt" width="71">size</td>
            <td class="xl65" style="border-left-style:none;width:53pt" width="71">all</td>
            <td class="xl69" rowspan="2" style="width:161pt" width="215">color on text <font class="font11">and row
                </font>
            </td>
        </tr>
        <tr height="39" style="height:29.0pt">
            <td class="xl67" colspan="2" height="39" style="border-right:.5pt solid black;height:29.0pt">
                <font class="font5">a</font>
                <font class="font6">ll </font>
                <font class="font7">m</font>
                <font class="font8">i</font>
                <font class="font9">x</font>
                <font class="font10">ed</font>
            </td>
        </tr>
    </tbody>
</table>

The width attribute is set correctly on <td> elements. So why doesn't it work?

1. <colgroup> has priority over inline widths

If the <colgroup> element is specified for the table, setting the inline widths will be ignored by the editor (such an integration is another story). The <colgroup> element was introduced specifically for table column resize feature in 35.0.1. So in the earlier versions, the <colgroup> was trimmed from this markup and it worked properly due to inline widths set. But <colgroup> with <col>s is also set in this markup, so why it doesn't work as expected?

2. pts are not converted in col[style[width]]

As can be seen here, in the col[style] upcaster all non-percentage width values (pts, pxs etc) are converted to auto. That means they are later automatically calculated based on the remaining %. So if all of our widths are in pt, they are all turned into auto and set to be equal to each other, which results in what can be seen below:

Turning on the pt unit handling results in an improvement, but still not exactly what we'd expect:

What happened? We have three columns, but in the markup there are only two col elements. The last column receives the minimal width.

3. col[span] is not supported

The first <col> element in the markup has span attribute: <col span="2" [...]>. In tables pasted from Excel, if two or more adjacent columns have the same width, the <col> elements are merged together using the span attribute. And that's what happened in this example - the first two columns have the same width, so they have one <col> element. The editor doesn't handle it, so it's dropped during the upcast and tries to automatically calculate the missing widths (which fails).

The fix

We need to:

  1. Provide support for pt unit in defining col element width.
  2. Translate the col[span] attribute to multiple tableColumns during the upcast.

We'll not fix the inline widths integration with table column resize in this issue. It's a separate, quite a large topic I suppose.

Edit after #14521 (comment)

There's one more thing to take into account. Let's take a look at the table from this example: its second column (according to the Excel grid) is an "empty" one - there are no cells that are anchored in it. While pasting the content into the editor, such columns are being removed. However, the <colgroup> element remains unaffected - no elements are removed from it - so it ends up with too many column slots and in consequence produces the offset at the end.

There's a postfixer in the table column resize that's responsible for handling situations like column removal. In this case it doesn't work though, since the table reshaping happens before inserting the modified table into the editor - so differ doesn't see this change and can't handle it. That's why cleaning up the <colgroup> element needs to be done "close" to the column removal.

It's worth noticing that since this filtering is only enabled in the TableClipboard plugin, the problem doesn't occur for data upcast, so [colspan] from won't be removed while upcasting the following table:

<figure class="table">
    <table>
        <tbody>
            <tr>
                <td colspan="2">&nbsp;</td>
                <td>&nbsp;</td>
            </tr>
            <tr>
                <td colspan="2">&nbsp;</td>
                <td>&nbsp;</td>
            </tr>
        </tbody>
    </table>
</figure>

(which means btw that executing the remove column in the first column makes no visual difference for the user - you can check it in any demo).

@dufipl

This comment was marked as resolved.

@charlttsie

This comment was marked as resolved.

mlewand added a commit that referenced this issue Aug 14, 2023
…in-paste-from-excel

Fix (table,paste-from-office): Tables pasted from MS Excel will have proper column widths. Closes #14521. Closes #14516.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Aug 14, 2023
@CKEditorBot CKEditorBot added this to the iteration 66 milestone Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:table squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants