Skip to content

Commit

Permalink
Fix incorrect upcast scan table heuristics responsible for creating h…
Browse files Browse the repository at this point in the history 
…eading rows. (#17640)

Fix (table): Prevent table corruption when setting editor data with th cells following colspan rows. Closes #17556, #17404
  • Loading branch information
@Mati365
Mati365 authored Dec 16, 2024
1 parent 417c261 commit fa2e169
Show file tree
Hide file tree
Showing 2 changed files with 125 additions and 6 deletions.
21 changes: 19 additions & 2 deletions packages/ckeditor5-table/src/converters/upcasttable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,14 +241,26 @@ function scanTable( viewTable: ViewElement ) {
( el: ViewNode ): el is ViewElement & { name: 'tr' } => el.is( 'element', 'tr' )
);

// Keep tracking of the previous row columns count to improve detection of heading rows.
let maxPrevColumns = null;

for ( const tr of trs ) {
const trColumns = Array
.from( tr.getChildren() )
.filter( el => el.is( 'element', 'td' ) || el.is( 'element', 'th' ) );

// This <tr> is a child of a first <thead> element.
if (
( firstTheadElement && tableChild === firstTheadElement ) ||
(
tableChild.name === 'tbody' &&
Array.from( tr.getChildren() ).length &&
Array.from( tr.getChildren() ).every( e => e.is( 'element', 'th' ) )
trColumns.length > 0 &&
// These conditions handles the case when the first column is a <th> element and it's the only column in the row.
// This case is problematic because it's not clear if this row should be a heading row or not, as it may be result
// of the cell span from the previous row.
// Issue: https://github.com/ckeditor/ckeditor5/issues/17556
( maxPrevColumns === null || trColumns.length === maxPrevColumns ) &&
trColumns.every( e => e.is( 'element', 'th' ) )
)
) {
headingRows++;
Expand All @@ -263,6 +275,11 @@ function scanTable( viewTable: ViewElement ) {
headingColumns = headingCols;
}
}

// We use the maximum number of columns to avoid false positives when detecting
// multiple rows with single column within `rowspan`. Without it the last row of `rowspan=3`
// would be detected as a heading row because it has only one column (identical to the previous row).
maxPrevColumns = Math.max( maxPrevColumns || 0, trColumns.length );
}
}

Expand Down
110 changes: 106 additions & 4 deletions packages/ckeditor5-table/tests/converters/upcasttable.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@ describe( 'upcastTable()', () => {
} );

describe( 'headingRows', () => {
it( 'should be able to detect heding row in 2x2 table', () => {
it( 'should be able to detect heading row in 2x2 table', () => {
editor.setData(
'<table>' +
'<tr>' +
Expand Down Expand Up @@ -590,7 +590,7 @@ describe( 'upcastTable()', () => {
);
} );

it( 'should be able to detect heding row in table with caption', () => {
it( 'should be able to detect heading row in table with caption', () => {
editor.setData(
'<table>' +
'<caption>Concerts</caption>' +
Expand Down Expand Up @@ -645,7 +645,7 @@ describe( 'upcastTable()', () => {
);
} );

it( 'should be able to detect heding row in 2x1 table', () => {
it( 'should be able to detect heading row in 2x1 table', () => {
editor.setData(
'<table>' +
'<tbody>' +
Expand All @@ -671,7 +671,7 @@ describe( 'upcastTable()', () => {
);
} );

it( 'should be able to detect heding row that has colspan', () => {
it( 'should be able to detect heading row that has colspan', () => {
editor.setData(
'<table>' +
'<tbody>' +
Expand Down Expand Up @@ -700,6 +700,108 @@ describe( 'upcastTable()', () => {
'</table>'
);
} );

it( 'should not treat the row containing only th as a heading row if it follows rowspan=2', () => {
editor.setData(
'<table>' +
'<tbody>' +
'<tr>' +
'<th>00</th>' +
'<td>01</td>' +
'<td>02</td>' +
'</tr>' +
'<tr>' +
'<th>10</th>' +
'<td colspan="2" rowspan="2">11</td>' +
'</tr>' +
'<tr>' +
'<th>20</th>' +
'</tr>' +
'<tr>' +
'<th>30</th>' +
'<td>31</td>' +
'<td>32</td>' +
'</tr>' +
'</tbody>' +
'</table>'
);

expectModel(
'<table headingColumns="1">' +
'<tableRow>' +
'<tableCell><paragraph>00</paragraph></tableCell>' +
'<tableCell><paragraph>01</paragraph></tableCell>' +
'<tableCell><paragraph>02</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>10</paragraph></tableCell>' +
'<tableCell colspan="2" rowspan="2"><paragraph>11</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>20</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>30</paragraph></tableCell>' +
'<tableCell><paragraph>31</paragraph></tableCell>' +
'<tableCell><paragraph>32</paragraph></tableCell>' +
'</tableRow>' +
'</table>'
);
} );

it( 'should not treat the row containing only th as a heading row if it is last row of rowspan=3', () => {
editor.setData(
'<table>' +
'<tbody>' +
'<tr>' +
'<th>00</th>' +
'<td>01</td>' +
'<td>02</td>' +
'</tr>' +
'<tr>' +
'<th>10</th>' +
'<td colspan="2" rowspan="3">11</td>' +
'</tr>' +
'<tr>' +
'<th>20</th>' +
'</tr>' +
'<tr>' +
'<th>30</th>' +
'</tr>' +
'<tr>' +
'<th>40</th>' +
'<td>41</td>' +
'<td>42</td>' +
'</tr>' +
'</tbody>' +
'</table>'
);

expectModel(
'<table headingColumns="1">' +
'<tableRow>' +
'<tableCell><paragraph>00</paragraph></tableCell>' +
'<tableCell><paragraph>01</paragraph></tableCell>' +
'<tableCell><paragraph>02</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>10</paragraph></tableCell>' +
'<tableCell colspan="2" rowspan="3"><paragraph>11</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>20</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>30</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>40</paragraph></tableCell>' +
'<tableCell><paragraph>41</paragraph></tableCell>' +
'<tableCell><paragraph>42</paragraph></tableCell>' +
'</tableRow>' +
'</table>'
);
} );
} );

describe( 'block contents', () => {
Expand Down

0 comments on commit fa2e169

Please sign in to comment.