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

Support block content inside tables #3199

Closed
Reinmar opened this issue Jun 14, 2018 · 20 comments · Fixed by ckeditor/ckeditor5-table#97
Closed

Support block content inside tables #3199

Reinmar opened this issue Jun 14, 2018 · 20 comments · Fixed by ckeditor/ckeditor5-table#97
Assignees
Labels
package:table type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jun 14, 2018

The MVP of the table feature doesn't contain a support for block content inside table cells. So, the first version will only support inline content (no images, no lists, no hard line breaks etc.)

Support for block content is our next priority after releasing the MVP. The challenge here is the UX. By default, we insert a table with inline content only:

<table>
    <tr> 
       <td>Foo</td>
       <td>Boo</td>
   </tr>
</table>

When the user presses Enter in one of the cells, what should happen? We need to insert paragraphs there:

<table>
    <tr> 
       <td><p>Foo</p><p>Bar</p></td>
       <td>Boo</td>
   </tr>
</table>

Is it ok that only one cell has block content and the other has inline content? This may cause vertical misalignment due to styling (paragraphs have margins) and general mess. So perhaps once you create block content in one of the cells, we should turn the entire table into "block table"?

There are also implementation issues. How to reflect that in the schema? A cell may contain inline content. Or it may contain block content. These are two different types of cells. Two different elements. Also, for features like headings, lists, etc. to be enabled, a block must be allowed in the current selection position. So, without paragraphs in the model all these features will be disabled. So, in fact, we always need paragraphs in the model.

And so on and so on :)

@jodator
Copy link
Contributor

jodator commented Jul 24, 2018

Quick test of:

schema.extend( '$block', { allowIn: 'tableCell' } );

Kinda works (I didn't check for bugs long enough to find any):
selection_022

As you can see both types of content are allowed in tableCell so maybe it will work on single tableCell element 🤔.

Some observations:

  • TAB - ie for lists (AFAICS only last item can be intended with TAB.
  • selection is a bit buggy IMHO - after trying to select next list item with shift+right the selection post-fixer selects whole table.
  • (dunno if bug 😛 ): table in table - will probably allow indifinte nesting but I don't insert <p>'s in tableCell on enter yet... (probably a similar check in the schema is need as in blockquote)
    selection_023
  • you can remove paragraph from a table cell - so back to $text out of the box ;)

@jodator
Copy link
Contributor

jodator commented Jul 24, 2018

Mah... infinite table nesting FTW!

selection_024

@pjasiun
Copy link

pjasiun commented Jul 24, 2018

Does it work with collaboration?

@jodator
Copy link
Contributor

jodator commented Jul 24, 2018

@pjasiun kinda works (I didn't check for editing in the same time of a table though):

peek 2018-07-24 16-14

@Reinmar
Copy link
Member Author

Reinmar commented Jul 24, 2018

:D

BTW, I'm ok with disallowing tables in tables. I even think that it should be the default. Block content makes sense in table cells, but whole tables do not. Let's start with such a simplification and see.

@pjasiun
Copy link

pjasiun commented Jul 24, 2018

BTW, I'm ok with disallowing tables in tables. I even think that it should be the default. Block content makes sense in table cells, but whole tables do not. Let's start with such a simplification and see.

Me too. But it is cool that it is possible :)

@jodator
Copy link
Contributor

jodator commented Jul 25, 2018

OK time for some decisions to make as I've almost got this working:

peek 2018-07-25

@Reinmar / @pjasiun / @oleq : Things to consider:

  1. Does current behavior makes sense: wrap the table cell in <paragraph> and then execute enter command (for me not executing enter is not OK as it might lead to WTFs if "nothing changes" after one enter press.
  2. Wrapping all table cells contents in <paragraph>s - YES / NO?
  3. Should we disable this behavior when paragraph model is not available (kinda makes sense)?
  4. Does current implementation in which we allow $text and $block inside tableCell is OK or do we need tow different elements like in @Reinmar's issue description?
  5. The tab in list in table cell: how to handle list indentation and moving to the next cell? Should be block caret movement in lists or should we block indentation? Right not (shift+)tab does both: indent/outdent + move caret :)
  6. Meging cells:
    a. mixed content allowed: probably the cell with $text only should be wrapped in <paragraph> before merging
    b. only one type of content allowed: $text + $text as now, <blockA> + <blockB> no merging even if same type of node (paragraph, list, etc)?

ps.: Right now I have this thing working with some minor bugs with tab/shift+tab but this probably will be resolved quickly.

Edit: Added point 5 about tab in list in table cell.
Edit 2: Added point 6 about merging cells...

@jodator
Copy link
Contributor

jodator commented Jul 25, 2018

After some F2F talks:

  1. We're going with always having <paragraph> inside <tableCell> in the model for empty cells and text-only cells. However it will be rendered differently for editng/data:
    • in the editing view: single <paragraph> will be rendered as <span> (if necessary), all other block (if single) or more then one <paragraph> will be rendered as in other places.
    • in the data pipeline: single <paragraph> will not be reneder (so it's $text will be placed directly into the <td> element.
      This implies that in the view table will have mixed cells with and without <p> elements.
  2. The enter will work as expected due to <paragraph> always present in the model.
  3. The Table feature will not work without Paragraph properly but as for now we do not care as much - editor will throw error eventually.
  4. Merging cells - will work as adding block elements so it should fix Consider spacing between the content when merging cells #3191.
  5. The tab should work for the lowest element so tab in <listItem> will indent list.

@jodator
Copy link
Contributor

jodator commented Jul 25, 2018

@Reinmar & @pjasiun another thing...

What if <paragraph> has attribute like alignment="right" - I suppose that we should output the <p> even if such scenario:

<table>
    <tableRow>
        <tableCell>
            <paragraph textAlign="left">foo</pargraph>
        </tableCell>
    </tableRow>
</table>

so the output will be:

<table>
    <tbody>
    <tr>
        <td>
            <p style="text-align:right;">foo</p>
        </td>
    </tr>
    </tbody>
</table>

@Reinmar
Copy link
Member Author

Reinmar commented Jul 25, 2018

Yes... and no. I forgot about this but we discussed it on the first meeting. The problem here is that when you have a single block of content in a table cell then clicking the alignment buttons may also apply to the entire table cell... In fact, such option might also be useful even if there are multiple blocks. Unfortunately, as most non-semantical information, it can be applied pretty much everywhere and I can easily imagine people wanting to do all these things.

Let's extract this to a separate ticket because it may be a long and tiring discussion.

@jodator
Copy link
Contributor

jodator commented Jul 30, 2018

@oleq: maybe something to consider is how to style block contents inside table. With current styles we have something like this:

selection_026

The most annoying are centered lists but other styles might some tuning also.

@oleq
Copy link
Member

oleq commented Aug 3, 2018

I guess we should fix the styles. OTOH:

  • I'm not sure whether we should revert them to their original, left-aligned look&feel as they would stand out compared to the default (centered) table text,
    • OTTH I think it may be OK to align all block content to the left by default :P It's unlikely a table will mix an inline and block content and even if that's true, it's up to the developers to make sure this edge case goes well with their website,
  • I'm not sure whether they should be fixed in the table content styles or feature (list, heading) content styles. The first option feels better because it will not bloat the editor styles when the tables feature is not loaded,

@jodator
Copy link
Contributor

jodator commented Aug 3, 2018

It's unlikely a table will mix an inline and block content

I think that a table with one table cell with block contend and single paragraphs (inline content?) will already mix:

selection_030

I'm not sure whether they should be fixed in the table content styles or feature (list, heading) content styles.

A perfect solution would be conditionally loaded styles for integration purposes like theme/table-paragraph.css or theme/table-list.css but we do not have such mechanism :) So I'm for the first option - table.css should do this.

@jodator
Copy link
Contributor

jodator commented Aug 8, 2018

@oleq As ckeditor/ckeditor5-table#97 is almost ready: how to change table's content styles: should we revert to left alignment for table cells? Mixed? Follow up?

@Reinmar
Copy link
Member Author

Reinmar commented Aug 14, 2018

Ping, @oleq

@oleq
Copy link
Member

oleq commented Aug 16, 2018

@oleq As ckeditor/ckeditor5-table#97 is almost ready: how to change table's content styles: should we revert to left alignment for table cells? Mixed? Follow up?

TBH I'm not sure I understand this question.

But I'd go with either:

  • table.css which makes the block content look like there was no table around (e.g. list items aligned to left instead of center). Unless we come up with some very smart table styles management system, leaving the whole "problem" to developers feels like a safe decision.
  • table cell content aligned to the left by default. Then, there's no problem in the first place 😛 It's also the styling most of the WYSIWYG (and native too) editors implement out-of-the-box.

I guess there's no good reason for CKE5 to stand out so I'd rather go with the later. It's simpler too and doesn't need maintanence as new content types arrive down the road. TBH, I don't quite remember why we went with centered table content in the first place (anyone?).

@Reinmar
Copy link
Member Author

Reinmar commented Aug 16, 2018

table cell content aligned to the left by default. Then, there's no problem in the first place 😛 It's also the styling most of the WYSIWYG (and native too) editors implement out-of-the-box.

👍

@Reinmar
Copy link
Member Author

Reinmar commented Aug 16, 2018

We may perhaps consider whether headings should be centred or not. They will rarely contain block content and I think that it's frequent to centre them, but I don't know if it's worth potential problems and introducing such a difference in comparison to normal cells.

@jodator
Copy link
Contributor

jodator commented Sep 3, 2018

@Reinmar & @oleq to sum up and finish ckeditor/ckeditor5-table#97:

The styles of block contents inside a table will be as they are in other parts of the editor:

  • Paragraph: left aligned
  • Heading: left aligned
  • ListItem: left aligned

@jodator jodator self-assigned this Sep 3, 2018
oskarwrobel referenced this issue in ckeditor/ckeditor5-table Sep 6, 2018
Feature: Support block content inside table. Closes #56.

BREAKING CHANGE: Removed `table/commands/utils~getParentTable()` method. Use `table/commands/utils~findAncestor()` instead.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-table Oct 9, 2019
@mlewand mlewand added this to the iteration 20 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:table labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:table type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants