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

Provide a solution for how to downcast a table to table>caption #10892

Closed
Reinmar opened this issue Nov 23, 2021 · 6 comments · Fixed by #11240
Closed

Provide a solution for how to downcast a table to table>caption #10892

Reinmar opened this issue Nov 23, 2021 · 6 comments · Fixed by #11240
Assignees
Labels
package:table squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@Reinmar
Copy link
Member

Reinmar commented Nov 23, 2021

By default we use <figure class=table> with <figcaption> which was a design choice made at the beginning mostly because:

  • it's consistent with other features contained in <figure>
  • and that, in turn, is a result of <figure> being more generic

However, in some cases, e.g. in Drupal, <table> + <caption> is the expected markup.

I don't think we should have it as a configuration option, because the table feature exists for a long time and this was requested for the second time to my knowledge. But we can provide (even here) a code sample on how to change the output of the editor.

Other remarks:

  • The goal is to mimic CKE4's output.
  • Can we base this on the reconversion work or should we start from master?
    • If we'll go with the reconversion approach, then we need to start from that branch obviously.
    • Using reconversion as a base means that we must release reconversion in February's release at the latest.
  • Post-fixer or overriding table's converter?
    • Post-fixer would be ugly. But it'd be safest. Also: we don't support data view post-fixers yet.
    • Overriding table's converter:
      • Would be consistent with how they override images.
      • Would need to be based on the reconversion branch.
  • The new markup needs to be compatible with:
    • GHS (table's and caption's attributes)
    • Table properties
      • On master table's width is set on <figure>. After this customization, <figure> disappears, so the style needs to be moved to <table>.
  • Due to the amount of work, complexity, and integration options, let's deliver this as a plugin, with tests. But let's not cover this in the guides, at least for now.
@Reinmar Reinmar added type:task This issue reports a chore (non-production change) and other types of "todos". squad:table squad:core Issue to be handled by the Core team. labels Nov 23, 2021
@Reinmar Reinmar changed the title Provide a code sample on how to downcast a table to table>caption Provide a solution for how to downcast a table to table>caption Dec 19, 2021
@oleq oleq assigned oleq and unassigned oleq Feb 7, 2022
@dawidurbanski
Copy link
Contributor

dawidurbanski commented Feb 8, 2022

This ticket might also require to look at this one: #9360

Although strictly speaking it's not a blocker to complete the current one, but it makes it obvious that there is something wrong with the simplest of payloads. For example:

<table>
	<caption>Monthly savings</caption>
	<tr>
		<th>Month</th>
		<th>Savings</th>
	</tr>
	<tr>
		<td>January</td>
		<td>$100</td>
	</tr>
</table>

Will be upcasted to:

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

Which later will be downcasted in the data pipeline to:

<table>
	<caption>Monthly savings</caption>
	<tbody>
		<tr>
			<th>Month</th>
			<th>Savings</th>
		</tr>
		<tr>
			<th>January</th>
			<th>$100</th>
		</tr>
	</tbody>
</table>

Couple notes:

  1. You may notice a <tbody> tag being added in the process. This is normal. It's being added by the browser. If you place any HTML table markup without tbody, every browser will add it for you. The same results you get in CKEditor 4.
  2. Model element got unexpected headingColumns attribute.
  3. All table cells are marked as headings (because of the previous point).

Also worth to mention that loading the following markup will result in the same thing:

<table>
	<caption>Monthly savings</caption>
	<tbody>
		<tr>
			<th>Month</th>
			<th>Savings</th>
		</tr>
		<tr>
			<td>January</td>
			<td>$100</td>
		</tr>
	</tbody>
</table>

This on the other hand will be good:

<table>
	<caption>Monthly savings</caption>
	<thead>
		<tr>
			<th>Month</th>
			<th>Savings</th>
		</tr>
	</thead>
	<tbody>
		<tr>
			<td>January</td>
			<td>$100</td>
		</tr>
	</tbody>
</table>

It's the exact same problem as described in #9360.

The problem source is located in this part of the code:

https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-table/src/converters/upcasttable.js#L213-L243

Basically what is happening is:

  1. Check if there is a <thead> element.
  2. Go into the <thead> and count all the rows inside. This will be the headingRows attribute value.
  3. Then go to the <tbody> element and find all <th> table cells. If you find one, bump the number of heading columns.

Since we have no <thead> it will go to <tbody> and use it only for the purpose of detecting columns.

In our case we have two <th> table cells, so we end up with headingColumns="2".

@Reinmar
Copy link
Member Author

Reinmar commented Feb 9, 2022

Some plugin naming ideas:

  • BareTableOutput
  • NoFigureTableOutput
  • FigurelessTableOutput
  • LegacyTableOutput

@dawidurbanski
Copy link
Contributor

I went with PlainTableOutput

@maxbarnas
Copy link
Contributor

maxbarnas commented Feb 10, 2022

@dawidurbanski and I checked the behavior of GHS together with both regular and plain table support and we found no issues whatsoever. Both the allowed inline elements and their attributes were handled correctly, along with attributes on <table> and <caption>.

As such the compatibility issues with GHS should be considered solved.

@Reinmar Reinmar added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Feb 11, 2022
@wimleers
Copy link

PlainTableOutput sounds reasonable — but I'd love to understand why this is considered "plain" or "bare". Plain/bare in contrast to what? The absence of a wrapping <figure> I suspect?

@Reinmar
Copy link
Member Author

Reinmar commented Feb 11, 2022

but I'd love to understand why this is considered "plain" or "bare". Plain/bare in contrast to what? The absence of a wrapping <figure> I suspect?

Yes. The default for us (and "native") is figure>table, hence a plugin overriding this has to indicate that it strips that figure.

Reinmar added a commit that referenced this issue Mar 2, 2022
Feature (table): Introduced the `PlainTableOutput` plugin to override the default `figure>caption` markup in the data pipeline (it outputs the table as `table>caption`). Closes: #10892.

Tests (table): Added tests for the `TableCaptionUI` plugin.
@CKEditorBot CKEditorBot added this to the iteration 52 milestone Mar 2, 2022
@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 2, 2022
@pomek pomek modified the milestones: iteration 52, iteration 51 Mar 2, 2022
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. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants