Opened 4 years ago

Closed 3 years ago

#12707 closed Bug (fixed)

Table CAPTION output below THEAD

Reported by: hel Owned by: kkrzton
Priority: Normal Milestone: CKEditor 4.5.8
Component: Core : Tables Version: 3.5.3
Keywords: Cc:

Description

The table properties editor looks to be placing the CAPTION element below the THEAD element.

The following ticket from a while back sounds like the same issue but this was noticed on our version 4.4.4 and can also be recreated on the ckeditor/demo version (4.4.6?).

https://dev.ckeditor.com/ticket/4809

The problem can be recreated in the demo ckeditor by:

1) Clicking the Table toolbar button
2) Selecting Headers "First Row"
3) Specifying a Caption
4) Clicking Ok to create the table
5) Click the Source toolbar button to view the generated HTML (screen shot attached).

This generates the following HTML output:

<table border="1" cellpadding="1" cellspacing="1" style="width:500px">
	<thead>
		<tr>
			<th scope="col">&nbsp;</th>
			<th scope="col">&nbsp;</th>
		</tr>
	</thead>
	<caption>Caption</caption>
	<tbody>
		<tr>
			<td>&nbsp;</td>
			<td>&nbsp;</td>
		</tr>
		<tr>
			<td>&nbsp;</td>
			<td>&nbsp;</td>
		</tr>
	</tbody>
</table>

<p>&nbsp;</p>

Having the CAPTION below the THEAD does not pass w3c validation:

Line 10, Column 10: document type does not allow element "caption" here

	<caption>Caption</caption>

Change History (12)

comment:1 Changed 4 years ago by hel

Keywords: firefox ie added

comment:2 Changed 4 years ago by hel

I believe (but could be wrong!) that this bug is to do with the following code circa line 640 in core/htmldataprocessor.js (defaultHtmlFilterRulesForAll.elements.table function):

// Clone the array as it would become empty during the sort call.
var children = element.children.slice( 0 );

This was introduced at commit #9312: Fixed table with multiple <tbody> output in wrong order:

https://github.com/ckeditor/ckeditor-dev/commit/a50a33300d71f7201c27d89ebccbbe5f1205a8a3

If I revert this (in my local version) to the following, the ordering comes out correctly:

var children = element.children;

What I'm not sure about is why that clone was added.

Thanks

comment:3 Changed 4 years ago by Jakub Ś

Keywords: firefox ie table caption thead removed
Status: newconfirmed
Version: 4.4.63.5.3

@helsom you are right - this issue can be reproduced from CKEditor 3.6.5 and #9312 seems to be the culprit here.

comment:4 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7

comment:5 Changed 3 years ago by kkrzton

Owner: set to kkrzton
Status: confirmedassigned

comment:6 Changed 3 years ago by kkrzton

Considering these four functions for handling tables:

table.createCaption();
table.createTHead();
table.createTBody();
table.createTFoot();

It depends on a browser how elements (children of table) are ordered.

In chrome no matter functions execution order the elements order is always the same:

<caption></caption><thead></thead><tfoot></tfoot><tbody></tbody>

In FF elements order reflect function execution order except thead which is always pushed as a first element.

In IE well... tbody and tfoot are not moved and thead and caption are prepended so it depends on execution order also.

Example here: jsfiddle table elements order test.

comment:7 Changed 3 years ago by kkrzton

Status: assignedreview

Changes in t/12707

The proposed fix moves caption element to the first place in table nodes list if necessary.

Last edited 3 years ago by kkrzton (previous) (diff)

comment:8 Changed 3 years ago by Marek Lewandowski

Status: reviewreview_failed

That's a good research on how createCaption, createTHead, createTBody, createTFootare playing together.

What we could do is to simply use standard append method instead. As far as I saw it does not reorder the tags. I've rebased the branch and pushed few commits - please check if changes make sense for you.

  • please move unit tests to tests/plugins/table/caption.js
  • I'm missing manual test (make sure to include proper tags @bender-tags: 4.5.7, tc, 12707)

comment:9 Changed 3 years ago by kkrzton

Status: review_failedassigned

comment:10 Changed 3 years ago by kkrzton

Status: assignedreview

Changes in t/12707.

Your solution is indeed simpler so I'm fine with it. I moved unit tests to tests/plugins/table/caption.js and added 3 manual tests with proper tags.

Last edited 3 years ago by kkrzton (previous) (diff)

comment:11 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:12 Changed 3 years ago by Marek Lewandowski

Resolution: fixed
Status: reviewclosed

Well done, fixed with git:6ec5dd5 to master.

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