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

Should tableCell be defined as object in schema? #6432

Closed
scofalik opened this issue Mar 13, 2020 · 5 comments · Fixed by #7770
Closed

Should tableCell be defined as object in schema? #6432

scofalik opened this issue Mar 13, 2020 · 5 comments · Fixed by #7770
Assignees
Labels
package:engine package:table type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

scofalik commented Mar 13, 2020

📝 Provide a description

I am not sure if tableCell element should be "an object" (isObject: true in schema). Looking at description: https://ckeditor.com/docs/ckeditor5/latest/api/module\_engine\_model\_schema-SchemaItemDefinition.html an object is:

"self-contained" and should be treated as a whole. Examples of object elements: image, table, video, etc. Note: An object is also a limit, so isLimit() returns true for object elements automatically.

So I am not sure if it applies. I'd add to that, that for me, "an object" is something that carries an information on it's own. For example a video, or image. It can be empty, with no other content/data and it still is meaningful.

Current situation messes up some algorithms.

First, getNearestSelectionRange() selects a cell, while I think that even in this situation:

<table>
    <tableRow>
        <tableCell><paragraph></paragraph></tableCell>[]
    </tableRow>
</table>

The selection range should be

<table>
    <tableRow>
        <tableCell><paragraph>[]</paragraph></tableCell>
    </tableRow>
</table>

Not

<table>
    <tableRow>
        [<tableCell><paragraph></paragraph></tableCell>]
    </tableRow>
</table>

(even with table selection turned on).

Second, Model#hasContent() will return true when an empty cell is selected (like in the example above).

I think it should be a isLimit + isBlock.

@scofalik
Copy link
Contributor Author

scofalik commented Mar 13, 2020

For example, this is what I have to do if I want to be sure that the range will be created in something that is "visible" in the editor. A real piece of content, if you will.

_hasContent( range ) {
	for ( const item of range.getItems() ) {
		if ( _itemIsContent( item ) ) {
			return true;
		}
	}

	return false;
}

_itemIsContent( item ) {
	return item.is( 'textProxy' ) || this.editor.model.schema.isObject( item ) && !item.is( 'tableCell' );
}

Similar would happen if I wanted to trim the range just to the actual content.

@Reinmar Reinmar added this to the backlog milestone Mar 24, 2020
@Reinmar
Copy link
Member

Reinmar commented Mar 25, 2020

IMO, the current definition is close to being correct. Let me go through it:

  • isObject this is needed to make the cell selectable from outside. It also makes a table cell "count" as a content which is an unfortunate side effect. If anything, I'd look for a different property to mark selectable things, but a new one would need to be introduced. isSelectable? All objects would be selectable by default too.
  • isLimit is fine of course. All objects are limits too.
  • isBlock that would be incorrect. Blocks are paragraphs, headings, etc. They have block formatting enabled (text transformation) and can be converted one to another (toggling between block styles). Also, containers such as blockQuote are not marked as blocks and tableCell is a container in fact (it contains paragraphs and other block-level elements).

So, if the problems boil down to two things:

  • make cells not seen as content
  • make cells not be selected automatically by getNearestSelectionRange()

Then we need to differentiate cells from other selectable objects. In other words – cell is selectable like other objects, but it's not an object. Hence, we need a new property in the schema and use that property in the aforementioned algorithms and selection postfixer.

@scofalik
Copy link
Contributor Author

👍 

You are right. Plus, table cell cannot be divided by enter key.

@Reinmar
Copy link
Member

Reinmar commented Mar 25, 2020

Plus, table cell cannot be divided by enter key.

That's ensured by isLimit.

@Reinmar Reinmar modified the milestones: backlog, iteration 31 Mar 25, 2020
@Reinmar Reinmar added package:engine type:improvement This issue reports a possible enhancement of an existing feature. labels Mar 25, 2020
@jodator jodator modified the milestones: iteration 31, iteration 32 Apr 16, 2020
@Reinmar Reinmar modified the milestones: iteration 32, next May 6, 2020
@Reinmar Reinmar modified the milestones: next, iteration 34 Jun 23, 2020
@Reinmar
Copy link
Member

Reinmar commented Jul 16, 2020

Let's wait for more ideas in #7631. If we won't have ground breaking proposals there regarding isObject, then the only decision that we'll have to make in this ticket will be whether we are also taking the occasion to implement isContent or not. Both options will be fine, so it's a matter of your intuition @oleq.

@Reinmar Reinmar modified the milestones: iteration 34, iteration 35 Jul 27, 2020
Reinmar added a commit that referenced this issue Aug 5, 2020
Feature (engine): Introduced `SchemaItemDefinition#isSelectable` and `SchemaItemDefinition#isContent` properties. Closes #6432.

Other (table): The `tableCell` model element brought by the `TableEditing` plugin is no longer an object (`SchemaItemDefinition#isObject`) in the `Schema` but a selectable (`SchemaItemDefinition#isSelectable`) (see #6432).

Internal (ui): Aligned the `BalloonToolbar` plugin behavior to the new  `SchemaItemDefinition#isSelectable` property (see #6432).

Docs (engine): Extended the "Schema" deep dive guide with the new properties and methods (see #6432, #7631).

Tests (image): Aligned tests to the fact that the `tableCell` model element is no longer an object but a selectable in the schema (see #6432).

Tests (horizontal-line): Aligned tests to the fact that the `tableCell` model element is no longer an object but a selectable in the schema (see #6432).

MINOR BREAKING CHANGE (table): The `tableCell` model element brought by the `TableEditing` plugin is no longer an object (`SchemaItemDefinition#isObject`) in the `Schema` but a selectable (`SchemaItemDefinition#isSelectable`). Please update your integration code accordingly (see #6432).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine package:table type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
4 participants