Improved addAtrributeCheck approach as per review.

ckeditor · Mar 12, 2024 · 8f457fc · 8f457fc
1 parent 1e4f2e0
commit 8f457fc
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 41 deletions.
diff --git a/packages/ckeditor5-engine/src/model/schema.ts b/packages/ckeditor5-engine/src/model/schema.ts
@@ -66,30 +66,6 @@ export default class Schema extends ObservableMixin() {
 			args[ 0 ] = new SchemaContext( args[ 0 ] );
 			args[ 1 ] = this.getDefinition( args[ 1 ] );
 		}, { priority: 'highest' } );
-
-		this.on<SchemaCheckAttributeEvent>( 'checkAttribute', ( evt, [ ctx, attributeName ] ) => {
-			const generalChecks = this._customAttributeChecks.get( this._genericCheckSymbol ) || [];
-			const customChecksForContext = this._customAttributeChecks.get( ctx.last.name ) || [];
-
-			let retValue;
-
-			for ( const nextCheck of [ ...generalChecks, ...customChecksForContext ] ) {
-				const checkResult = nextCheck( ctx, attributeName );
-
-				// Only apply newer return value if it's boolean.
-				retValue = typeof checkResult === 'boolean' ? checkResult : retValue;
-
-				if ( retValue === false ) {
-					// Break out of loop if any check returns false (forbidding is prioritized).
-					break;
-				}
-			}
-
-			if ( typeof retValue === 'boolean' ) {
-				evt.stop();
-				evt.return = retValue;
-			}
-		}, { priority: 'high' } );
 	}
 
 	/**
@@ -469,6 +445,27 @@ export default class Schema extends ObservableMixin() {
 			return false;
 		}
 
+		const generalChecks = this._customAttributeChecks.get( this._genericCheckSymbol ) || [];
+		const customChecksForContext = this._customAttributeChecks.get( ( context as SchemaContext ).last.name ) || [];
+
+		let retValue;
+
+		for ( const nextCheck of [ ...generalChecks, ...customChecksForContext ] ) {
+			const checkResult = nextCheck( context as SchemaContext, attributeName );
+
+			// Only apply newer return value if it's boolean.
+			retValue = typeof checkResult === 'boolean' ? checkResult : retValue;
+
+			if ( retValue === false ) {
+				// Break out of loop if any check returns false (forbidding is prioritized).
+				break;
+			}
+		}
+
+		if ( typeof retValue === 'boolean' ) {
+			return retValue;
+		}
+
 		return def.allowAttributes.includes( attributeName );
 	}
 

diff --git a/packages/ckeditor5-engine/tests/model/schema.js b/packages/ckeditor5-engine/tests/model/schema.js
@@ -755,7 +755,7 @@ describe( 'Schema', () => {
 			} );
 
 			schema.on( 'checkChild', () => {
-				order.push( 'checkChild:high-after' );
+				order.push( 'checkChild:high' );
 			}, { priority: 'high' } );
 
 			sinon.stub( schema, '_checkContextMatch' ).callsFake( () => {
@@ -764,12 +764,12 @@ describe( 'Schema', () => {
 
 			schema.checkChild( root1, r1p1 );
 
-			expect( order.join() ).to.equal( 'checkChild:high-after,addChildCheck,declarativeCheck' );
+			expect( order.join() ).to.equal( 'checkChild:high,addChildCheck,declarativeCheck' );
 
 			sinon.restore();
 		} );
 
-		it( 'stops the event and overrides the return value when callback returned true', () => {
+		it( 'overrides the return value when callback returned true', () => {
 			schema.register( '$text' );
 
 			expect( schema.checkChild( root1, '$text' ) ).to.be.false;
@@ -781,7 +781,7 @@ describe( 'Schema', () => {
 			expect( schema.checkChild( root1, '$text' ) ).to.be.true;
 		} );
 
-		it( 'stops the event and overrides the return value when callback returned false', () => {
+		it( 'overrides the return value when callback returned false', () => {
 			expect( schema.checkChild( root1, r1p1 ) ).to.be.true;
 
 			schema.addChildCheck( () => {
@@ -853,47 +853,39 @@ describe( 'Schema', () => {
 			} );
 		} );
 
-		it( 'adds a high-priority listener', () => {
+		it( 'adds a callback that runs after listeners', () => {
 			const order = [];
 
 			schema.addAttributeCheck( () => {
 				order.push( 'addAttributeCheck' );
 			} );
 
 			schema.on( 'checkAttribute', () => {
-				order.push( 'checkAttribute:high-after' );
+				order.push( 'checkAttribute:high' );
 			}, { priority: 'high' } );
 
 			schema.checkAttribute( r1p1, 'foo' );
 
-			expect( order.join() ).to.equal( 'addAttributeCheck,checkAttribute:high-after' );
+			expect( order.join() ).to.equal( 'checkAttribute:high,addAttributeCheck' );
 		} );
 
-		it( 'stops the event and overrides the return value when callback returned true', () => {
+		it( 'overrides the return value when callback returned true', () => {
 			expect( schema.checkAttribute( r1p1, 'bar' ) ).to.be.false;
 
 			schema.addAttributeCheck( () => {
 				return true;
 			} );
 
-			schema.on( 'checkAttribute', () => {
-				throw new Error( 'the event should be stopped' );
-			}, { priority: 'high' } );
-
 			expect( schema.checkAttribute( r1p1, 'bar' ) ).to.be.true;
 		} );
 
-		it( 'stops the event and overrides the return value when callback returned false', () => {
+		it( 'overrides the return value when callback returned false', () => {
 			expect( schema.checkAttribute( r1p1, 'foo' ) ).to.be.true;
 
 			schema.addAttributeCheck( () => {
 				return false;
 			} );
 
-			schema.on( 'checkAttribute', () => {
-				throw new Error( 'the event should be stopped' );
-			}, { priority: 'high' } );
-
 			expect( schema.checkAttribute( r1p1, 'foo' ) ).to.be.false;
 		} );