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

Schema addChildCheck/addAttributeCheck filtering #15959

Merged
merged 3 commits into from
Jul 30, 2024
Merged

Schema addChildCheck/addAttributeCheck filtering #15959

merged 3 commits into from
Jul 30, 2024

Conversation

map-r
Copy link
Contributor

@map-r map-r commented Mar 1, 2024

Suggested merge commit message (convention)

Feature (engine): Schema#addChildCheck() and Schema#addAttributeCheck() can now register a callback for specific item or attribute, which should improve performance when using custom callback checks. Callback checks should be added only for specific item or attribute if possible. See the API reference. Closes #15834.

Fix (engine): Schema#checkChild() will now correctly check custom callback checks for each item in the context.

Other (engine): Introduced Schema#trimLast().

Internal: Used specific callback checks in some of the features.

Docs (engine): Improved schema deep dive guide and added missing section related to attributes custom callback checks.

MINOR BREAKING CHANGE: Schema callbacks added through addChildCheck() will no longer add event listeners with high priority and will no longer stop checkChild event. Instead, these callbacks are now handled on normal priority, as a part of the default checkChild() call. This also means that listeners added to checkChild event on high priority will fire before any callbacks added by checkChild(). Earlier they would fire in registration order. This may impact you if you implemented custom schema callback using both addChildCheck() and direct listener to checkChild event. All above is also true for addAttributeCheck() and checkAttribute event and callbacks.


Additional information

Copy link
Contributor

@scofalik scofalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments.

One decision to make here is whether we want to stick to the original plan (my advice to keep things simple) or if we want to extend the original plan a bit, following what @map-r did in this PR.

In short (read my comments to understand more):

  1. The original was to have addChildCheck( callback, elementName ) and addAttributeCheck( callback, attributeName ) so that these callbacks are fired only for given element name (checkChild( context, elementName )) or attribute (checkAttribute( context, attributeName )).
  2. However they could be extended to cover more cases.
  3. It would be something like addChildCheck( callback, elementOrParentName ) and addAttributeCheck( callback, attributeOrElementName ).
  4. With these extensions, it seems we would cover most (all?) of the cases we have right now. Callback added by both methods would be additionally fired if context.last.name matches the parameter.
  5. I'd advise closing the other PR (introducing disallowing in schema) first, and then rewrite some of the callbacks to static declarations. And then see how many more callbacks we need to cover.
  6. In general, I'd err on keeping this simple. If there's one or two global callbacks, then we are fine. We can always extend later. OTOH, note, that if you go with extensions how Adam proposed, you need to make extra checks in the callbacks (which I commented on in this review -- I thought they are not needed but they actually are). Extending later would be a breaking change

@DawidKossowski @map-r @niegowski I leave this to you.

Comment on lines 150 to 152
if ( childDefinition.name === '$marker' ) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If now we fire this callback just for $marker checks, can't it be just () => true? 🤔.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we run it for both $marker as item in context and as a child element

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thus, it might fire for wrong thing (when $marker is in context and something else is child)

packages/ckeditor5-code-block/src/codeblockediting.ts Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/image/imageinlineediting.ts Outdated Show resolved Hide resolved
Comment on lines 96 to 98
if ( typeof retValue === 'boolean' ) {
evt.stop();
evt.return = retValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we move it into the if above (in the loop)?

Ah, okay, I get it. This should get a comment why we are doing it exactly like that. Add a comment here (above this `if`) and also above `if` from line `114` (comment there that if any of the callbacks disallowed the attribute then it is disallowed and it cannot be overwritten, hence we can stop processing).

One thing that is related to the other PR is that it would be good to document that the custom callbacks will get precedence over the rules declared in schema.register(). After we introduce disabling things in schema.register() it may be confusing, that, for example, I disabled something through schema.register() but it is still enabled (because another custom callback allowed for it). I think we should mention it in checkChild(), checkAttribute(), addChildCheck(), and addAttributeCheck().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above comment as of today is still to be resolved, but probably in sync with that other PR Szymon mentioned

public addAttributeCheck( callback: SchemaAttributeCheckCallback ): void {
this.on<SchemaCheckAttributeEvent>( 'checkAttribute', ( evt, [ ctx, attributeName ] ) => {
const retValue = callback( ctx, attributeName );
public addAttributeCheck( callback: SchemaAttributeCheckCallback, forNode?: string ): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misunderstanding here on your part. It should be declared for attribute, not for node. From the issue:

First, we should add additional parameter to addChildCheck() and addAttributeCheck() that will specify for which element/attribute this callback is declared.

We want to call this callbacks only when "bold" or "linkHref" are checked, not when given elements are checked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I think I understand why you went this way. We have two types of callbacks when it comes to current addAttributeCheck.

First -- we have custom callbacks for attribute X. They would be solved if you implemented it accordingly to my idea (set callback for attribute name, not element name).

But then, we have attribute-related callbacks for element Y. For example, disallow all attributes in $text inside title-content. But in this case, such callback should be actually called for all attributes, so it makes sense that it would stay as a generic callback. It's the same as adding one callback for all attributes.

OTOH, if a callback stays generic it will be also called for non-related items. Let's take example rule:

schema.addAttributeCheck( ( context, attributeName ) => {
	if ( context.endsWith( 'codeBlock $text' ) ) {
		return false;
	}
} );

Now it would be fired for, e.g. schema.checkAttribute( [ 'image' ], 'imageSrc' ), which doesn't make much sense. If we specify $text here, and only get callbacks for context.last.name and attributeName, then we could limit the number of callbacks even more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, the solution is to either do the attr check only and disregard specific nodes, making some checks generic, or to make option to use either context or attribute to filter the checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess making two properties to filter in both childCheck and attributeCheck seems reasonable. I can leave it like this to be improved, or try to improve it by this two-type filtering, tbd.

packages/ckeditor5-engine/src/model/schema.ts Outdated Show resolved Hide resolved

const generalChecks = this._customChildChecks.get( this._genericCheckSymbol ) || [];
const checksForChild = this._customChildChecks.get( childDef.name ) || [];
const checksForContext = this._customChildChecks.get( ctx.last.name ) || [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very hesitant about this. It expands how this mechanism work in a way that I haven thought about, so I am not sure if this is correct.

The point of the issue was to turn this situation:

  1. We call schema.childCheck( context, 'elementName' );
  2. As a result, we call 15 custom callbacks
  3. Then we call declarative check (if needed).

Into this situation:

  1. We call schema.childCheck( context, 'elementName' );
  2. As a result, we call 2 custom generic callbacks.
  3. Then we call 1 specific callback for 'elementName' (if needed).
  4. Then we call declarative check (if needed).

With your solution, we call a custom callback both when we want to check if 'elementName' can be inserted somewhere and where something is being inserted into 'elementName'.

It may turn one or two global callbacks into specific but works in a bit unexpected way. Unless you have convincing arguments, for now, I'd be against it (because I don't want to spend too much time thinking about this topic) but we can keep it in mind as a possible improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything we would need at this point is to make a filter object, which either has context or a child as a property, with the name of element being the value. Then we will only check what we want only once, as calling this twice is what you say you have problem with.

@map-r
Copy link
Contributor Author

map-r commented Mar 13, 2024

@DawidKossowski Outro for you and of course for @scofalik:

I've fixed and resolved some of the comments. The major one was to move the callback triggering into an actual checkChild and checkAttribute function - I admit , I took it naturally that it should probably be kept where it was before, i.e. in the prioritized listeners. But Szymon actually was right that he meant something else, simpler, and it does the work so far, too (didn't yet come across any biggest issue with that, but that yet may wait to be discovered).

The things left out for you to pick up are:

  • decision what to do with the type of filtering, whether it should be done via context and children both, or only by children (rest would be generic), I proposed a solution to use an object-based parameter that we will extract correct parameters to filter for any one of these properties, giving us better control over when to trigger the check.
  • an useful thing to add in the documentation about precedences of custom callbacks also over the disallow* and allow* rules.

@scofalik scofalik force-pushed the ck/15834 branch 2 times, most recently from 920b5fc to 69695dc Compare June 10, 2024 11:18
@scofalik
Copy link
Contributor

Summary:

  1. Schema#addChildCheck() and Schema#addAttributeCheck() have a new optional parameter that allows to register the callback for a specific item or specific attribute.
  2. Earlier, when you registered a custom callback, it was fired for every checkChild() and checkAttribute() call. With multiple features and multiple custom callbacks, it could lead to performance problems (lagging when typing).
  3. The way how it works is very simple:
    1. If you registered addChildCheck( cb, 'foo' ) it will be called when checkChild( ctx, 'foo' ) is called.
      1. Mind, that checkChild( ctx ) also checks the whole ctx. If 'foo' is inside ctx the callback will be fired too.
    2. If you registered addAttributeCheck( cb, 'bold' ) it will be called when checkAttribute( ctx, 'bold' ) is called.
      1. Mind, that you register attribute check for a model attribute not a model item.
  4. Generic callbacks are still available.
    1. Of course, generic callbacks are not recommended as they can lead to performance problems. However, sometimes, they can't be avoided.
  5. checkChild() and checkAttribute() are still decorated so you can add listeners to the events. This is not recommended as it can also lead to performance problems.
  6. The order of performing checks is as follows:
    1. All callbacks added as events listeners with highest and high priority.
    2. All generic callbacks. Among them, order as added in code.
    3. All specific callbacks. Among them, order as added in code.
    4. Declarative checks.

I also added this summary to the issue so it can be easy to found.

@scofalik scofalik requested review from niegowski and removed request for DawidKossowski June 10, 2024 11:33
packages/ckeditor5-engine/src/model/schema.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/schema.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/schema.ts Show resolved Hide resolved
packages/ckeditor5-engine/src/model/schema.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/schema.ts Show resolved Hide resolved
packages/ckeditor5-engine/src/model/schema.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/schema.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/schema.ts Outdated Show resolved Hide resolved
Comment on lines -819 to -821
schema.on( 'checkChild', () => {
throw new Error( 'the event should be stopped' );
}, { priority: 'high' } );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be marked as a breaking change? Same as for the attribute check?
What if someone provided a child check callback or a custom listener who relied on the event being stopped before?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to analyze this and you are right, and I added a minor breaking change entry.

However, if I get it correctly, the scope for possible problem is quite narrow here.

If somebody was using only direct event listeners (i.e. schema.on( 'checkChild' )) then it seems to me that nothing changed for them. They were probably adding these events on high priority and were stopping them (otherwise, these callback would not make much sense, I think).

If somebody was using only callbacks added through addChildCheck() or addAttributeCheck() then nothing changes here as well -- they are still working in "first wins" fashion, even though the event is not stopped.

So if I get it right, the only problem may happen if somebody was defining rules both using the event listener and using the methods, because they might have first define addChildCheck( callback ) and then add listener with high priority, and in such case, if callback returned a value, the listener would not fire. But now, not only the listener will fire, it will fire before the callback.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you think about the entry. I first described it for "child checks" and then at the end added that it is true also for "attribute checks". When I was describing both at once, it became almost unreadable.

…eck()` can now register a callback for specific item or attribute, which should improve performance when using custom callback checks. Callback checks should be added only for specific item or attribute if possible.

Fix (engine): `Schema#checkChild()` will now correctly check custom callback checks for each item in the context.

Internal: Used specific callback checks in some of the features.

Docs (engine): Improved schema deep dive guide and added missing section related to attributes custom callback checks.
@scofalik scofalik requested a review from niegowski July 29, 2024 17:09
scofalik
scofalik previously approved these changes Jul 30, 2024
@niegowski niegowski merged commit c4878b7 into master Jul 30, 2024
7 checks passed
@niegowski niegowski deleted the ck/15834 branch July 30, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema#addChildCheck leads to performance problems
3 participants