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.

Sign up for GitHub

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

Support col[span] and pts in paste from excel #14720

Merged
merged 16 commits into from
Aug 14, 2023
Merged

Support col[span] and pts in paste from excel #14720

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5c36a10
Add support for col[span] and custom units in the col width.
Dumluregn Aug 1, 2023
2e94961
Cleanup.
Dumluregn Aug 1, 2023
bdc5d79
Rename model property.
Dumluregn Aug 2, 2023
4ea383a
Extract helper.
Dumluregn Aug 2, 2023
661e1da
Refactor.
Dumluregn Aug 2, 2023
ebe6013
Add unit tests.
Dumluregn Aug 2, 2023
9197a7e
Update docs.
Dumluregn Aug 2, 2023
41ea609
Add issue references [skip ci].
Dumluregn Aug 2, 2023
09c3645
Merge branch 'master' into ck/14521-support-cell-widths-in-paste-from…
Dumluregn Aug 2, 2023
b1fd583
Handle the problem with empty columns.
Dumluregn Aug 3, 2023
ae9cbdf
Remove stray console.log().
Dumluregn Aug 4, 2023
4e7c664
Add unit test for clipboard integration.
Dumluregn Aug 4, 2023
2b22b6f
Add comment and improve precision.
Dumluregn Aug 4, 2023
e321628
Merge remote-tracking branch 'origin/master' into ck/14521-support-ce…
mlewand Aug 9, 2023
5334cbe
Merge branch 'master' into ck/14521-support-cell-widths-in-paste-from…
Dumluregn Aug 14, 2023
07dfbcb
Apply code review notes.
Dumluregn Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/ckeditor5-table/src/tableclipboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@ export default class TableClipboard extends Plugin {
return;
}

console.log( pastedTable );
Dumluregn marked this conversation as resolved.
Show resolved Hide resolved

// Override default model.insertContent() handling at this point.
evt.stop();

Expand Down
3 changes: 2 additions & 1 deletion packages/ckeditor5-table/src/tablecolumnresize/converters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ export function upcastColgroupElement( tableUtilsPlugin: TableUtils ): ( dispatc
}

const columnElements = getTableColumnElements( tableColumnGroup );
let columnWidths = getTableColumnsWidths( tableColumnGroup );
const columnsCount = tableUtilsPlugin.getColumns( modelTable );
let columnWidths = getTableColumnsWidths( tableColumnGroup, conversionApi.writer );

// Fill the array with 'auto' values if the number of columns is higher than number of declared values.
columnWidths = Array.from( { length: columnsCount }, ( _, index ) => columnWidths[ index ] || 'auto' );

if ( columnWidths.length != columnElements.length || columnWidths.includes( 'auto' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not triggering column width normalization for such case:

<table>
    <colgroup>
        <col style="width: 30pt;">
        <col style="width: 70pt;">
    </colgroup>
    <tbody>
        <tr>
            <td>a</td>
            <td>b</td>
        </tr>
    </tbody>
</table>

And the final table ends up like this:

<figure class="table">
    <table class="ck-table-resized">
        <colgroup>
            <col style="width:30%;">
            <col style="width:70%;">
        </colgroup>
        <tbody>
        <tr>
            <td>a</td>
            <td>b</td>
        </tr>
        </tbody>
    </table>
</figure>

so pt values are treated as they would be %, the table gets full page width and the original sizes are not preserved.

The column width is normalized in post-fixer (but should be normalized before post-fixers are triggered).

Expand Down
17 changes: 15 additions & 2 deletions packages/ckeditor5-table/src/tablecolumnresize/tablecolumnresizeediting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ export default class TableColumnResizeEditing extends Plugin {

this.editor.model.schema.register( 'tableColumn', {
allowIn: 'tableColumnGroup',
allowAttributes: [ 'columnWidth' ],
allowAttributes: [ 'columnWidth', 'colSpan' ],
isLimit: true
} );
}
Expand Down Expand Up @@ -418,14 +418,27 @@ export default class TableColumnResizeEditing extends Plugin {
value: ( viewElement: ViewElement ) => {
const viewColWidth = viewElement.getStyle( 'width' );

if ( !viewColWidth || !viewColWidth.endsWith( '%' ) ) {
// 'pt' is the default unit for table column width pasted from MS Office.
Dumluregn marked this conversation as resolved.
Show resolved Hide resolved
if ( !viewColWidth || ( !viewColWidth.endsWith( '%' ) && !viewColWidth.endsWith( 'pt' ) ) ) {
return 'auto';
}

return viewColWidth;
}
}
} );

// The `col[span]` attribute is present in tables pasted from MS Excel. We use it to set the temporary `colSpan` model attribute,
// which is consumed during the `colgroup` element upcast.
// See https://github.com/ckeditor/ckeditor5/issues/14521#issuecomment-1662102889 for more details.
conversion.for( 'upcast' ).attributeToAttribute( {
view: {
name: 'col',
key: 'span'
},
model: 'colSpan'
} );

conversion.for( 'downcast' ).attributeToAttribute( {
model: {
name: 'tableColumn',
Expand Down
45 changes: 37 additions & 8 deletions packages/ckeditor5-table/src/tablecolumnresize/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,14 +230,11 @@ export function sumArray( array: Array<number | string> ): number {
* changed proportionally so that they all sum back to 100%. If there are columns without specified width, the amount remaining
* after assigning the known widths will be distributed equally between them.
*
* Currently, only widths provided as percentage values are supported.
*
* @param columnWidths An array of column widths.
* @returns An array of column widths guaranteed to sum up to 100%.
*/
export function normalizeColumnWidths( columnWidths: Array<string> ): Array<string> {
const widths: Array<number | 'auto'> = columnWidths.map( width => {
// Possible values are 'auto' or string ending with '%'
if ( width === 'auto' ) {
return width;
}
Expand Down Expand Up @@ -376,23 +373,55 @@ export function getColumnGroupElement( element: Element ): Element {
}

/**
* Returns an array of 'tableColumn' elements.
* Returns an array of 'tableColumn' elements. It may be empty if there's no `tableColumnGroup` element.
*
* @internal
* @param element A 'table' or 'tableColumnGroup' element.
* @returns An array of 'tableColumn' elements.
*/
export function getTableColumnElements( element: Element ): Array<Element> {
return Array.from( getColumnGroupElement( element ).getChildren() as IterableIterator<Element> );
const columnGroupElement = getColumnGroupElement( element );

if ( !columnGroupElement ) {
return [];
Dumluregn marked this conversation as resolved.
Show resolved Hide resolved
}

return Array.from( columnGroupElement.getChildren() as IterableIterator<Element> );
}

/**
* Returns an array of table column widths.
* Returns an array of table column widths. If the `writer` is provided, it will also translate the `colSpan` model attribute.
Dumluregn marked this conversation as resolved.
Show resolved Hide resolved
*
* @internal
* @param element A 'table' or 'tableColumnGroup' element.
* @param [writer] A writer instance.
* @returns An array of table column widths.
*/
export function getTableColumnsWidths( element: Element ): Array<string> {
return getTableColumnElements( element ).map( column => column.getAttribute( 'columnWidth' ) as string );
export function getTableColumnsWidths( element: Element, writer?: Writer ): Array<string> {
const tableColumnElements = getTableColumnElements( element );

if ( !writer ) {
return tableColumnElements.map( column => column.getAttribute( 'columnWidth' ) as string );
}

return tableColumnElements.reduce( ( acc: Array<string>, element ) => {
const columnWidth = element.getAttribute( 'columnWidth' ) as string;
const colSpan = element.getAttribute( 'colSpan' ) as number | undefined;

if ( !colSpan ) {
acc.push( columnWidth );
return acc;
}

// Translate the `colSpan` model attribute on to the proper number of column widths
// and remove it from the element.
// See https://github.com/ckeditor/ckeditor5/issues/14521#issuecomment-1662102889 for more details.
for ( let i = 0; i < colSpan; i++ ) {
acc.push( columnWidth );
}

writer.removeAttribute( 'colSpan', element );

return acc;
}, [] );
}
22 changes: 22 additions & 0 deletions packages/ckeditor5-table/src/tableutils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type {
import TableWalker, { type TableWalkerOptions } from './tablewalker';
import { createEmptyTableCell, updateNumericAttribute } from './utils/common';
import { removeEmptyColumns, removeEmptyRows } from './utils/structure';
import { getTableColumnElements } from './tablecolumnresize/utils';

type Cell = { cell: Element; rowspan: number };
type CellsToMove = Map<number, Cell>;
Expand Down Expand Up @@ -471,6 +472,7 @@ export default class TableUtils extends Plugin {

model.change( writer => {
adjustHeadingColumns( table, { first, last }, writer );
const tableColumns = getTableColumnElements( table );

for ( let removedColumnIndex = last; removedColumnIndex >= first; removedColumnIndex-- ) {
for ( const { cell, column, cellWidth } of [ ...new TableWalker( table ) ] ) {
Expand All @@ -482,6 +484,26 @@ export default class TableUtils extends Plugin {
writer.remove( cell );
}
}

if ( tableColumns[ removedColumnIndex ] ) {
let adjacentColumn;
Dumluregn marked this conversation as resolved.
Show resolved Hide resolved

// If the removed column is the first one then we need to add its width to the next column.
// Otherwise we add it to the previous column.
if ( removedColumnIndex === 0 ) {
adjacentColumn = tableColumns[ 1 ];
} else {
adjacentColumn = tableColumns[ removedColumnIndex - 1 ];
}

const removedColumnWidth = parseInt( tableColumns[ removedColumnIndex ].getAttribute( 'columnWidth' ) as string );
const adjacentColumnWidth = parseInt( adjacentColumn.getAttribute( 'columnWidth' ) as string );

writer.remove( tableColumns[ removedColumnIndex ] );

// Add the removed column width (in %) to the adjacent column.
writer.setAttribute( 'columnWidth', removedColumnWidth + adjacentColumnWidth + '%', adjacentColumn );
}
}

// Remove empty rows that could appear after removing columns.
Expand Down
187 changes: 187 additions & 0 deletions packages/ckeditor5-table/tests/tablecolumnresize/tablecolumnresizeediting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,90 @@ describe( 'TableColumnResizeEditing', () => {
);
} );

it( 'should handle <col> with pt width', () => {
editor.setData(
`<figure class="table">
<table>
<colgroup>
<col style="width:30pt;">
<col style="width:30pt;">
<col style="width:60pt">
</colgroup>
<tbody>
<tr>
<td>11</td>
<td>12</td>
<td>13</td>
</tr>
</tbody>
</table>
</figure>`
);

expect( getModelData( model, { withoutSelection: true } ) ).to.equal(
'<table>' +
'<tableRow>' +
'<tableCell>' +
'<paragraph>11</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>12</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>13</paragraph>' +
'</tableCell>' +
'</tableRow>' +
'<tableColumnGroup>' +
'<tableColumn columnWidth="25%"></tableColumn>' +
'<tableColumn columnWidth="25%"></tableColumn>' +
'<tableColumn columnWidth="50%"></tableColumn>' +
'</tableColumnGroup>' +
'</table>'
);
} );

it( 'should handle <col> with pt width summing to less than 100', () => {
editor.setData(
`<figure class="table">
<table>
<colgroup>
<col style="width:15pt;">
<col style="width:15pt;">
<col style="width:30pt">
</colgroup>
<tbody>
<tr>
<td>11</td>
<td>12</td>
<td>13</td>
</tr>
</tbody>
</table>
</figure>`
);

expect( getModelData( model, { withoutSelection: true } ) ).to.equal(
'<table>' +
'<tableRow>' +
'<tableCell>' +
'<paragraph>11</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>12</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>13</paragraph>' +
'</tableCell>' +
'</tableRow>' +
'<tableColumnGroup>' +
'<tableColumn columnWidth="25%"></tableColumn>' +
'<tableColumn columnWidth="25%"></tableColumn>' +
'<tableColumn columnWidth="50%"></tableColumn>' +
'</tableColumnGroup>' +
'</table>'
);
} );

it( 'should adjust the missing column widths proportionally', () => {
editor.setData(
`<figure class="table">
Expand Down Expand Up @@ -482,6 +566,109 @@ describe( 'TableColumnResizeEditing', () => {
'</table>'
);
} );

it( 'should consume the `col[span]` attribute', () => {
Dumluregn marked this conversation as resolved.
Show resolved Hide resolved
editor.setData(
`<figure class="table">
<table>
<colgroup>
<col style="width:10%;" span="2">
<col style="width:50%;">
<col style="width:10%;" span="3">
</colgroup>
<tbody>
<tr>
<td>11</td>
<td>12</td>
<td>13</td>
<td>14</td>
<td>15</td>
<td>16</td>
</tr>
</tbody>
</table>
</figure>`
);

expect( getModelData( model, { withoutSelection: true } ) ).to.equal(
'<table>' +
'<tableRow>' +
'<tableCell>' +
'<paragraph>11</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>12</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>13</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>14</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>15</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>16</paragraph>' +
'</tableCell>' +
'</tableRow>' +
'<tableColumnGroup>' +
'<tableColumn columnWidth="10%"></tableColumn>' +
'<tableColumn columnWidth="10%"></tableColumn>' +
'<tableColumn columnWidth="50%"></tableColumn>' +
'<tableColumn columnWidth="10%"></tableColumn>' +
'<tableColumn columnWidth="10%"></tableColumn>' +
'<tableColumn columnWidth="10%"></tableColumn>' +
'</tableColumnGroup>' +
'</table>'
);
} );

it( 'should handle the `col[span]` attribute and missing cols', () => {
editor.setData(
`<figure class="table">
<table>
<colgroup>
<col style="width:10%;" span="2">
<col style="width:50%;">
</colgroup>
<tbody>
<tr>
<td>11</td>
<td>12</td>
<td>13</td>
<td>14</td>
</tr>
</tbody>
</table>
</figure>`
);

expect( getModelData( model, { withoutSelection: true } ) ).to.equal(
'<table>' +
'<tableRow>' +
'<tableCell>' +
'<paragraph>11</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>12</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>13</paragraph>' +
'</tableCell>' +
'<tableCell>' +
'<paragraph>14</paragraph>' +
'</tableCell>' +
'</tableRow>' +
'<tableColumnGroup>' +
'<tableColumn columnWidth="10%"></tableColumn>' +
'<tableColumn columnWidth="10%"></tableColumn>' +
'<tableColumn columnWidth="50%"></tableColumn>' +
'<tableColumn columnWidth="30%"></tableColumn>' +
'</tableColumnGroup>' +
'</table>'
);
} );
} );
} );

Expand Down
2 changes: 1 addition & 1 deletion packages/ckeditor5-table/tests/tablecolumnresize/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ describe( 'TableColumnResize utils', () => {
} );
} );

describe( 'getColumnWidths()', () => {
describe( 'getTableColumnsWidths()', () => {
it( 'should return tableColumnGroup count when there are columns', () => {
setModelData( model, modelTable( [ [ '01', '02' ] ], { columnWidths: '50%,50%' } ) );

Expand Down