Преглед на файлове

Fixed the path matching algorithm to always require right end matching.

Piotrek Koszuliński преди 9 години
родител
ревизия
5bbe9d8101

+ 39 - 32
packages/ckeditor5-engine/src/model/schema.js

@@ -501,7 +501,7 @@ export class SchemaItem {
 	}
 
 	/**
-	 * Checks whether this item has any registered path of given type that matches provided path.
+	 * Checks whether this item has any registered path of given type that matches the provided path.
 	 *
 	 * @protected
 	 * @param {String} type Paths' type. Possible values are `allow` or `disallow`.
@@ -510,38 +510,10 @@ export class SchemaItem {
 	 * @returns {Boolean} `true` if item has any registered matching path, `false` otherwise.
 	 */
 	_hasMatchingPath( type, checkPath, attribute ) {
-		const itemPaths = this._getPaths( type, attribute );
-
-		// We check every path registered (possibly with given attribute) in the item.
-		for ( let itemPath of itemPaths ) {
-			// Pointer to last found item from `itemPath`.
-			let i = 0;
-
-			// Now we have to check every item name from the path to check.
-			for ( let checkName of checkPath ) {
-				// Don't check items that are not registered in schema.
-				if ( !this._schema.hasItem( checkName ) ) {
-					continue;
-				}
+		const allowedPaths = this._getPaths( type, attribute );
 
-				// Every item name is expanded to all names of items that item is extending.
-				// So, if on item path, there is an item that is extended by item from checked path, it will
-				// also be treated as matching.
-				const chain = this._schema._extensionChains.get( checkName );
-
-				// Since our paths have to match in given order, we always check against first item from item path.
-				// So, if item path is: B D E
-				// And checked path is: A B C D E
-				// It will be matching (A won't match, B will match, C won't match, D and E will match)
-				if ( chain.indexOf( itemPath[ i ] ) > -1 ) {
-					// Move pointer as we found element under index `i`.
-					i++;
-				}
-			}
-
-			// If `itemPath` has no items it means that we removed all of them, so we matched all of them.
-			// This means that we found a matching path.
-			if ( i === itemPath.length ) {
+		for ( const allowedPath of allowedPaths ) {
+			if ( matchPaths( this._schema, checkPath, allowedPath ) ) {
 				return true;
 			}
 		}
@@ -581,3 +553,38 @@ export class SchemaItem {
  * @typedef {String|Array.<String|module:engine/model/element~Element>|module:engine/model/position~Position}
  * module:engine/model/schema~SchemaPath
  */
+
+// Checks whether the given checkPath and allowedPath right ends match.
+//
+// checkPath: C, D
+// allowedPath: A, B, C, D
+// result: OK
+//
+// checkPath: A, B, C
+// allowedPath: A, B, C, D
+// result: NOK
+//
+// Note – when matching paths, element extension chains (inheritance) are taken into consideration.
+//
+// @param {Schema} schema
+// @param {Array.<String>} checkPath
+// @param {Array.<String>} allowedPath
+function matchPaths( schema, checkPath, allowedPath ) {
+	const allowedPathReversed = allowedPath.slice().reverse();
+	const checkPathReversed = checkPath.slice().reverse();
+
+	const length = Math.min( allowedPathReversed.length, checkPathReversed.length );
+	let index = 0;
+
+	while ( index < length ) {
+		const extChain = schema._extensionChains.get( checkPathReversed[ index ] );
+
+		if ( extChain.includes( allowedPathReversed[ index ] ) ) {
+			index++;
+		} else {
+			return false;
+		}
+	}
+
+	return true;
+}

+ 69 - 0
packages/ckeditor5-engine/tests/model/schema/schema.js

@@ -336,6 +336,75 @@ describe( 'check', () => {
 			expect( schema.check( { name: 'new', inside: new Position( root, [ 0 ] ) } ) ).to.be.false;
 		} );
 	} );
+
+	describe( 'bug #732', () => {
+		// Ticket case.
+		it( 'should return false if given element is allowed in the root but not deeper', () => {
+			schema.registerItem( 'paragraph', '$block' );
+
+			expect( schema.check( { name: 'paragraph', inside: [ '$root', 'paragraph' ] } ) ).to.be.false;
+		} );
+
+		// Two additional, real life cases accompanying the ticket case.
+		it( 'should return true if checking whether text is allowed in $root > paragraph', () => {
+			schema.registerItem( 'paragraph', '$block' );
+
+			expect( schema.check( { name: '$text', inside: [ '$root', 'paragraph' ] } ) ).to.be.true;
+		} );
+
+		it( 'should return true if checking whether text is allowed in paragraph', () => {
+			schema.registerItem( 'paragraph', '$block' );
+
+			expect( schema.check( { name: '$text', inside: [ 'paragraph' ] } ) ).to.be.true;
+		} );
+
+		// Veryfing the matching algorithm.
+		// The right ends of the element to check and "inside" paths must match.
+		describe( 'right ends of paths must match', () => {
+			beforeEach( () => {
+				schema.registerItem( 'a' );
+				schema.registerItem( 'b' );
+				schema.registerItem( 'c' );
+				schema.registerItem( 'd' );
+				schema.registerItem( 'e' );
+
+				schema.allow( { name: 'a', inside: [ 'b', 'c', 'd' ] } );
+				schema.allow( { name: 'e', inside: [ 'a' ] } );
+			} );
+
+			// Simple chains created by a single allow() call.
+
+			it( 'a inside b, c', () => {
+				expect( schema.check( { name: 'a', inside: [ 'b', 'c' ] } ) ).to.be.false;
+			} );
+
+			it( 'a inside b', () => {
+				expect( schema.check( { name: 'a', inside: [ 'b' ] } ) ).to.be.false;
+			} );
+
+			it( 'a inside b, c, d', () => {
+				expect( schema.check( { name: 'a', inside: [ 'b', 'c', 'd' ] } ) ).to.be.true;
+			} );
+
+			it( 'a inside c, d', () => {
+				expect( schema.check( { name: 'a', inside: [ 'c', 'd' ] } ) ).to.be.true;
+			} );
+
+			it( 'a inside d', () => {
+				expect( schema.check( { name: 'a', inside: [ 'd' ] } ) ).to.be.true;
+			} );
+
+			// "Allowed in" chains created by two separate allow() calls (`e inside a` and `a inside b,c,d`).
+
+			it( 'e inside a, d', () => {
+				expect( schema.check( { name: 'e', inside: [ 'd', 'a' ] } ) ).to.be.true;
+			} );
+
+			it( 'e inside b, c, d', () => {
+				expect( schema.check( { name: 'e', inside: [ 'b', 'c', 'd' ] } ) ).to.be.false;
+			} );
+		} );
+	} );
 } );
 
 describe( 'itemExtends', () => {

+ 3 - 5
packages/ckeditor5-engine/tests/model/schema/schemaitem.js

@@ -112,23 +112,21 @@ describe( 'SchemaItem', () => {
 
 			expect( item._hasMatchingPath( 'allow', [ 'div', 'header' ] ) ).to.be.true;
 			expect( item._hasMatchingPath( 'allow', [ 'html', 'div', 'header' ] ) ).to.be.true;
-			expect( item._hasMatchingPath( 'allow', [ 'div', 'header', 'span' ] ) ).to.be.true;
-			expect( item._hasMatchingPath( 'allow', [ 'html', 'div', 'p', 'header', 'span' ] ) ).to.be.true;
 		} );
 
 		it( 'should return false if there are no allowed paths that match query path', () => {
 			item.allow( [ 'div', 'p' ] );
 
-			expect( item._hasMatchingPath( 'allow', [ 'p' ] ) ).to.be.false;
 			expect( item._hasMatchingPath( 'allow', [ 'div' ] ) ).to.be.false;
 			expect( item._hasMatchingPath( 'allow', [ 'p', 'div' ] ) ).to.be.false;
+			expect( item._hasMatchingPath( 'allow', [ 'div', 'p', 'span' ] ) ).to.be.false;
 		} );
 
 		it( 'should return true if there is at least one disallowed path that matches query path', () => {
 			item.allow( [ 'div', 'header' ] );
 			item.disallow( [ 'p', 'header' ] );
 
-			expect( item._hasMatchingPath( 'disallow', [ 'html', 'div', 'p', 'header', 'span' ] ) ).to.be.true;
+			expect( item._hasMatchingPath( 'disallow', [ 'html', 'div', 'p', 'header' ] ) ).to.be.true;
 		} );
 
 		it( 'should use only paths that are registered for given attribute', () => {
@@ -142,7 +140,7 @@ describe( 'SchemaItem', () => {
 			expect( item._hasMatchingPath( 'allow', [ 'html', 'div' ], 'bold' ) ).to.be.true;
 
 			expect( item._hasMatchingPath( 'disallow', [ 'html', 'div', 'header' ] ) ).to.be.false;
-			expect( item._hasMatchingPath( 'disallow', [ 'html', 'div', 'p', 'header', 'span' ], 'bold' ) ).to.be.true;
+			expect( item._hasMatchingPath( 'disallow', [ 'html', 'div', 'p', 'header' ], 'bold' ) ).to.be.true;
 		} );
 	} );