Schema addChildCheck/addAttributeCheck filtering

[map-r]

Suggested merge commit message (convention)

Internal (engine): Added option for custom check callbacks to run only for a specified context node or children. Closes #15834.

---

Additional information

[scofalik]

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.

[scofalik]

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

[map-r]

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

[map-r]

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

[scofalik]

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().

[map-r]

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

[scofalik]

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.

[scofalik]

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.

[map-r]

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.

[map-r]

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.

[scofalik]

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.

[map-r]

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]

@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.