Skip to content

Fix incorrect move of table row containing only th elements to thead if previous row was not header row (v47.x) #19448

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

Merged
Mati365 merged 1 commit into from
Nov 26, 2025
+180 −10
Merged

Fix incorrect move of table row containing only th elements to thead if previous row was not header row (v47.x) #19448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions .changelog/20251126125431_ck_19431.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
type: Fix
scope:
- ckeditor5-table
closes:
- https://github.com/ckeditor/ckeditor5/issues/19431
---

Add experimental fix for incorrect table rows marking and moving as header row when preceding rows are not header rows.

This change needs to be enabled by setting `experimentalFlags.tableCellTypeSupport` to `true`. In the next major release, this flag will be removed and the fix will be enabled by default.
32 changes: 23 additions & 9 deletions packages/ckeditor5-table/src/converters/upcasttable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import type { ModelElement, UpcastDispatcher, UpcastElementEvent, ViewElement, ViewNode } from 'ckeditor5/src/engine.js';
import type { Editor } from 'ckeditor5/src/core.js';

import { createEmptyTableCell } from '../utils/common.js';
import { getViewTableFromWrapper } from '../utils/structure.js';
Expand Down Expand Up @@ -81,7 +82,9 @@ export function upcastTableFigure() {
* @returns Conversion helper.
* @internal
*/
export function upcastTable() {
export function upcastTable( editor: Editor ) {
Copy link
Contributor

@pszczesniak pszczesniak Nov 26, 2025

Choose a reason for hiding this comment

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

Maybe would be good to pass only the config flag value with default set to false instead of editor instance?

This function Is re-exported as an internal function but maybe we can add a fallback for the parameter.

Copy link
Contributor

@pszczesniak pszczesniak Nov 26, 2025

Choose a reason for hiding this comment

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

Ok, it will be removed in v48.

const ignoreHeaderRowMoveIfNormalRowsBefore = !!editor.config.get( 'experimentalFlags.tableCellTypeSupport' );

return ( dispatcher: UpcastDispatcher ): void => {
dispatcher.on<UpcastElementEvent>( 'element:table', ( evt, data, conversionApi ) => {
const viewTable = data.viewItem;
Expand All @@ -91,7 +94,7 @@ export function upcastTable() {
return;
}

const { rows, headingRows, headingColumns } = scanTable( viewTable );
const { rows, headingRows, headingColumns } = scanTable( viewTable, ignoreHeaderRowMoveIfNormalRowsBefore );

// Only set attributes if values is greater then 0.
const attributes: { headingColumns?: number; headingRows?: number } = {};
Expand Down Expand Up @@ -200,10 +203,16 @@ export function ensureParagraphInTableCell( elementName: string ) {
* headingRows - The number of rows that go as table headers.
* headingColumns - The maximum number of row headings.
* rows - Sorted `<tr>` elements as they should go into the model - ie. if `<thead>` is inserted after `<tbody>` in the view.
*
* @param viewTable The view table element.
* @param ignoreHeaderRowMoveIfNormalRowsBefore If set to `true`, then heading rows detection will stop
* once a normal row is found between heading rows. If set to `false`, then heading rows will be detected
* even if normal rows are between them.
* @returns The table metadata.
*/
function scanTable( viewTable: ViewElement ) {
let headingRows = 0;
function scanTable( viewTable: ViewElement, ignoreHeaderRowMoveIfNormalRowsBefore: boolean ) {
let headingColumns: number | undefined = undefined;
let shouldAccumulateHeadingRows: boolean | null = null;

// The `<tbody>` and `<thead>` sections in the DOM do not have to be in order `<thead>` -> `<tbody>` and there might be more than one
// of them.
Expand Down Expand Up @@ -232,8 +241,9 @@ function scanTable( viewTable: ViewElement ) {
}

// Save the first `<thead>` in the table as table header - all other ones will be converted to table body rows.
if ( tableChild.name === 'thead' && !firstTheadElement ) {
firstTheadElement = tableChild;
if ( tableChild.name === 'thead' ) {
shouldAccumulateHeadingRows = null;
firstTheadElement ||= tableChild;
}

// There might be some extra empty text nodes between the `<tr>`s.
Expand Down Expand Up @@ -261,13 +271,17 @@ function scanTable( viewTable: ViewElement ) {
// 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' ) )
trColumns.every( e => e.is( 'element', 'th' ) ) &&
// If there is at least one "normal" table row between heading rows, then stop accumulating heading rows.
// However, if flag `ignoreHeaderRowMoveIfNormalRowsBefore` is set to `false`, then ignore this rule.
( !ignoreHeaderRowMoveIfNormalRowsBefore || shouldAccumulateHeadingRows === null || shouldAccumulateHeadingRows )
)
) {
headingRows++;
headRows.push( tr );
shouldAccumulateHeadingRows = true;
} else {
bodyRows.push( tr );
shouldAccumulateHeadingRows = false;
}

// We use the maximum number of columns to avoid false positives when detecting
Expand Down Expand Up @@ -299,7 +313,7 @@ function scanTable( viewTable: ViewElement ) {
}

return {
headingRows,
headingRows: headRows.length,
headingColumns: headingColumns || 0,
rows: [ ...headRows, ...bodyRows ]
};
Expand Down
2 changes: 1 addition & 1 deletion packages/ckeditor5-table/src/tableediting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export class TableEditing extends Plugin {
conversion.for( 'upcast' ).add( upcastTableFigure() );

// Table conversion.
conversion.for( 'upcast' ).add( upcastTable() );
conversion.for( 'upcast' ).add( upcastTable( editor ) );

conversion.for( 'editingDowncast' ).elementToStructure( {
model: {
Expand Down
145 changes: 145 additions & 0 deletions packages/ckeditor5-table/tests/converters/upcasttable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,151 @@ describe( 'upcastTable()', () => {
'</table>'
);
} );

describe( 'experimental skipping of movement of heading rows', () => {
beforeEach( async () => {
await editor.destroy();

editor = await ClassicTestEditor.create( '', {
plugins: [ TableEditing, Paragraph, ImageBlockEditing, Widget ],
experimentalFlags: {
tableCellTypeSupport: true
}
} );

model = editor.model;

// Since this part of test tests only view->model conversion editing pipeline is not necessary
// so defining model->view converters won't be necessary.
editor.editing.destroy();
} );

it( 'should properly calculate heading columns when result is more than zero', () => {
editor.setData(
'<table>' +
'<tbody>' +
// This row starts with 2 th (3 total).
'<tr><th>31</th><th>32</th><td>33</td><th>34</th></tr>' +
// This row starts with 2 th (2 total).
'<tr><th>41</th><th>42</th><td>43</td><td>44</td></tr>' +
// This row starts with 1 th (1 total). This one has min number of heading columns: 1.
'<tr><th>51</th><td>52</td><td>53</td><td>54</td></tr>' +
// This row starts with 4 th (4 total).
'<tr><th>11</th><th>12</th><th>13</th><th>14</th></tr>' +
'</tbody>' +
'<thead>' +
// This row has 4 ths but it is a thead.
'<tr><th>21</th><th>22</th><th>23</th><th>24</th></tr>' +
'</thead>' +
'</table>'
);

expectModel(
'<table headingColumns="1" headingRows="1">' +
'<tableRow>' +
'<tableCell><paragraph>21</paragraph></tableCell>' +
'<tableCell><paragraph>22</paragraph></tableCell>' +
'<tableCell><paragraph>23</paragraph></tableCell>' +
'<tableCell><paragraph>24</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>31</paragraph></tableCell>' +
'<tableCell><paragraph>32</paragraph></tableCell>' +
'<tableCell><paragraph>33</paragraph></tableCell>' +
'<tableCell><paragraph>34</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>41</paragraph></tableCell>' +
'<tableCell><paragraph>42</paragraph></tableCell>' +
'<tableCell><paragraph>43</paragraph></tableCell>' +
'<tableCell><paragraph>44</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>51</paragraph></tableCell>' +
'<tableCell><paragraph>52</paragraph></tableCell>' +
'<tableCell><paragraph>53</paragraph></tableCell>' +
'<tableCell><paragraph>54</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>11</paragraph></tableCell>' +
'<tableCell><paragraph>12</paragraph></tableCell>' +
'<tableCell><paragraph>13</paragraph></tableCell>' +
'<tableCell><paragraph>14</paragraph></tableCell>' +
'</tableRow>' +
'</table>'
);
} );

it( 'should properly group heading rows in table containing multiple tbody', () => {
editor.setData( `
<figure class="table">
<table>
<thead>
<tr>
<th>Col Header 1</th>
<th>Col Header 2</th>
<th>Col Header 3</th>
</tr>
<tr>
<th colspan="3">Rowgroup Header 1</th>
</tr>
</thead>
<tbody>
<tr>
<th>Row Header 1</th>
<td>Data 1,1</td>
<td>Data 2,1</td>
</tr>
<tr>
<th colspan="3">Rowgroup Header 2</th>
</tr>
<tr>
<th>Row Header 2</th>
<td>Data 1,2</td>
<td>Data 2,2</td>
</tr>
<tr>
<th>Row Header 3</th>
<td>Data 1,3</td>
<td>Data 2,3</td>
</tr>
</tbody>
</table>
</figure>
` );

expectModel(
'<table headingColumns="1" headingRows="2">' +
'<tableRow>' +
'<tableCell><paragraph>Col Header 1</paragraph></tableCell>' +
'<tableCell><paragraph>Col Header 2</paragraph></tableCell>' +
'<tableCell><paragraph>Col Header 3</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell colspan="3"><paragraph>Rowgroup Header 1</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>Row Header 1</paragraph></tableCell>' +
'<tableCell><paragraph>Data 1,1</paragraph></tableCell>' +
'<tableCell><paragraph>Data 2,1</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell colspan="3"><paragraph>Rowgroup Header 2</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>Row Header 2</paragraph></tableCell>' +
'<tableCell><paragraph>Data 1,2</paragraph></tableCell>' +
'<tableCell><paragraph>Data 2,2</paragraph></tableCell>' +
'</tableRow>' +
'<tableRow>' +
'<tableCell><paragraph>Row Header 3</paragraph></tableCell>' +
'<tableCell><paragraph>Data 1,3</paragraph></tableCell>' +
'<tableCell><paragraph>Data 2,3</paragraph></tableCell>' +
'</tableRow>' +
'</table>'
);
} );
} );
} );

describe( 'headingRows', () => {
Expand Down