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

Better detection of heading rows #3172

Closed
jodator opened this issue May 24, 2018 · 8 comments · Fixed by #13600
Closed

Better detection of heading rows #3172

jodator opened this issue May 24, 2018 · 8 comments · Fixed by #13600
Assignees
Labels
package:table squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@jodator
Copy link
Contributor

jodator commented May 24, 2018

Right now when upcasting table row it is assumed that headingRows attribute is calculated only from number of rows of first <thead> section (following <thead> sections are treated as normal rows).

The other case might be when a <table> element does not have <thead> nor <tbody> sections or a <tbody> section with "pseudo table head".

The "pseudo table head" is a row which contains only <th> elements.

We might detect such rows as heading rows because right now they will be counted as heading columns which will result in wrong table:

For instance such table:

<table>
	<tr>
		<th>a</th>
		<th>b</th>
	</tr>
	<tr>
		<th>c</th>
		<td>d</td>
	</tr>
</table>

will be converted to a model representation:

<table headingColumns="2">
    <tableRow>...</tableRow>
    <tableRow>...</tableRow>
</table>

but should be:

<table headingColumns="1" headingRows="1">
    <tableRow>...</tableRow>
    <tableRow>...</tableRow>
</table>
@Reinmar Reinmar changed the title Better detection of heading rows. Better detection of heading rows May 25, 2018
@Reinmar
Copy link
Member

Reinmar commented May 25, 2018

This will be important for pasting. And for handling some legacy data (although, of course, CKEditor 4 does produce <thead> element so I rather mean a content produced by other editors).

@jodator
Copy link
Contributor Author

jodator commented Jul 31, 2018

Another case - non-semantic heading section in <tbody>:

<table>
  <caption>Concerts</caption>
  <tbody>
  <tr>
    <th>Date</th>
    <th>Event</th>
    <th>Venue</th>
  </tr>
  <tr>
    <td>12 Feb</td>
    <td>Waltz with Strauss</td>
    <td>Main Hall</td>
  </tr>
  <tr>
    <td>24 Mar</td>
    <td>The Obelisks</td>
    <td>West Wing</td>
  </tr>
  <tr>
    <td>14 Apr</td>
    <td>The What</td>
    <td>Main Hall</td>
  </tr>
  </tbody>
</table>

Edit: reported also in ckeditor/ckeditor5-table#109.

@jodator
Copy link
Contributor Author

jodator commented Oct 18, 2018

@Reinmar We might consider a simple fix for this. A case when there is a row with only <th> elements in it looks like heading row and should not be taken into consideration when detecting number of heading columns.

It keeps coming back from time to time: ckeditor/ckeditor5-table#109, ckeditor/ckeditor5-table#140.

@Reinmar
Copy link
Member

Reinmar commented Oct 18, 2018

A case when there is a row with only <th> elements in it looks like heading row and should not be taken into consideration when detecting number of heading columns.

👍

In general, I think we need two separate mechanisms:

  1. For detecting header rows. If thead is present, then the number of rowa in that thead. If not, then we should count all trs that have only th children.

  2. For detecting header cols. That'd require checking how many ths are there at the beginning of each rows. The minimal number is the number of heading cols. For example:

    tr: th, th, td, td
    tr: th, td, th, th
    tr: th, th, td, td
    

    That'd be 1 because in the second row there's only one th at the beginning.

If you agree that this algorithm makes sense, then we could try to go this way. I don't mean that we have to rewrite what we have right now, but we should keep it in mind when making decisions about fixing some issues.

@jodator
Copy link
Contributor Author

jodator commented Oct 18, 2018

2\. For detecting header cols.

This is how this works now:
https://github.com/ckeditor/ckeditor5-table/blob/33d25d2f48f740c3d6cf2c83b7e1ba4a0fbdcbb2/src/converters/upcasttable.js#L226-L235

We only lacks the mechanism of not counting columns in heading row.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-table Oct 9, 2019
@mlewand mlewand added this to the nice-to-have milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:table labels Oct 9, 2019
@mlewand mlewand added the support:2 An issue reported by a commercially licensed client. label Jan 12, 2021
@mlewand
Copy link
Contributor

mlewand commented Jan 12, 2021

Also got it reported from one of our users today.

@Reinmar
Copy link
Member

Reinmar commented Mar 1, 2023

@Reinmar
Copy link
Member

Reinmar commented Mar 1, 2023

Notes:

For this input:

<table>
	<tr>
		<th>a</th>
		<th>b</th>
	</tr>
	<tr>
		<th>c</th>
		<td>d</td>
	</tr>
</table>

This is the expected output:

<figure class=table>
	<table>
		<thead>
			<tr>
				<th>a</th>
				<th>b</th>
			</tr>
		</thead>
		<tbody>
			<tr>
				<th>c</th>
				<td>d</td>
			</tr>
		</tbody>
	</table>
</figure>

@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Mar 1, 2023
@Reinmar Reinmar added the squad:core Issue to be handled by the Core team. label Mar 1, 2023
@CKEditorBot CKEditorBot added 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 Mar 2, 2023
@mremiszewski mremiszewski self-assigned this Mar 2, 2023
arkflpc added a commit that referenced this issue Mar 7, 2023
…able-heading-rows-and-columns

Fix (table): Change detection on heading rows and columns on table upcast. Closes #3172.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Mar 7, 2023
@CKEditorBot CKEditorBot added this to the iteration 61 milestone Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:table squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants