Add disallow rules in the Schema #15916
Conversation
… `SchemaItemDefinition#disallowIn`, `#disallowChildren` and `disallowAttributes`.
WalkthroughThe recent updates across various modules introduce the ability to specify disallowed children and attributes in the schema definitions of the editing engine. This enhancement aims to provide finer control over document structure by allowing exclusions in element behavior, crucial for complex document integrations. Changes
Assessment against linked issues
The changes align well with the objectives of enhancing schema capabilities to include disallowing specific children and attributes, crucial for creating more structured and controlled document environments. Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (2)
Additional Context UsedLanguageTool (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Out of diff range and nitpick comments (22)
docs/tutorials/crash-course/model-and-schema.md (1)
Line range hint
96-96: Consider adding a comma after "On the right side" for better readability.- On the right side you should see `highlight:true` under the `Allowed Attributes` section. + On the right side, you should see `highlight:true` under the `Allowed Attributes` section.docs/framework/architecture/editing-engine.md (5)
Line range hint
29-31: Clarify the repeated words in the description of the model.- Like in the DOM, the model structure is contained within a {@link module:engine/model/document~Document document} that contains {@link module:engine/model/document~Document#roots root elements} (the model, as well as the view, may have multiple roots). The document also holds its {@link module:engine/model/history~History history of its changes}. + Like in the DOM, the model structure is contained within a {@link module:engine/model/document~Document document}, which includes {@link module:engine/model/document~Document#roots root elements} (the model, as well as the view, may have multiple roots). The document also maintains a record of its changes through the {@link module:engine/model/history~History}.
Line range hint
61-61: Ensure consistency in the explanation of nested change blocks.- All changes done within a single `change()` block are combined into one undo step (they are added to a single {@link module:engine/model/batch~Batch batch}). When nesting `change()` blocks, all changes are added to the outermost `change()` block's batch. + All changes made within a single `change()` block are combined into one undo step, grouped into a single {@link module:engine/model/batch~Batch batch}. When nesting `change()` blocks, all changes are consolidated into the batch of the outermost `change()` block.
Line range hint
116-116: Clarify the conditional phrase regarding the effect on typed text.- OK, but how to let CKEditor 5 know that I want the selection to "be bold" in the case described above? This is important information because it affects whether or not the typed text will be bold, too. + OK, but how does one inform CKEditor 5 that the selection should "be bold" in the described scenario? This is crucial as it determines whether the typed text will also be bold.
Line range hint
152-152: Ensure clarity in the description of ranges and selections.- A {@link module:engine/model/range~Range} contains two positions: {@link module:engine/model/range~Range#start start} and {@link module:engine/model/range~Range#end end} ones. + A {@link module:engine/model/range~Range} is defined by two positions: the {@link module:engine/model/range~Range#start start} and the {@link module:engine/model/range~Range#end end}.
Line range hint
364-364: Clarify the description of the editing downcasting process.- {@link framework/deep-dive/conversion/downcast#downcast-pipelines **Editing downcasting**} is a bit different process than the other two. + {@link framework/deep-dive/conversion/downcast#downcast-pipelines **Editing downcasting**} differs from the other two processes.packages/ckeditor5-engine/docs/framework/deep-dive/schema.md (15)
Line range hint
13-13: Consider revising the phrase "in regards of" to "in regard to" for grammatical correctness.- atomicity in regards of external actions + atomicity in regard to external actions
Line range hint
412-412: Consider adding a preposition to correct the sentence structure.- Taken these characteristics, the image caption should be defined as a limit element + Considering these characteristics, the image caption should be defined as a limit element
Line range hint
428-428: Insert a comma after "like in the example above" for better readability.- For an image caption like in the example above it does not make much sense to select the caption box + For an image caption like in the example above, it does not make much sense to select the caption box
Line range hint
430-430: Insert a comma after "Most likely" for better readability.- Most likely users should be able to select the entire image + Most likely, users should be able to select the entire image
Line range hint
453-453: Consider revising "made out of" to a more concise alternative like "composed of".- content is usually made out of blocks like paragraphs + content is usually composed of blocks like paragraphs
Line range hint
497-497: Insert a comma after "and this".- find their way into the editor data and this is what makes them content elements + find their way into the editor data, and this is what makes them content elements
Line range hint
585-585: Consider revising "despite the fact that" to a more concise alternative like "although".- despite the fact that the block quote and paragraph features know nothing about each other + although the block quote and paragraph features know nothing about each other
Line range hint
587-587: Remove the comma before "because" as the clause is essential to the meaning.- with the `allowContentOf: '$root'` rule), because `<$container>` is also allowed in `<$root>` + with the `allowContentOf: '$root'` rule) because `<$container>` is also allowed in `<$root>`
Line range hint
680-680: Correct the double determiner issue by choosing either "the" or "a".- which is the a base method used to check whether some element is allowed + which is a base method used to check whether some element is allowed
Line range hint
718-718: Insert a comma after "Normally" for better clarity.- Normally it would wrap the two selected blocks (`<y>` and `<z>`) with a `<blockQuote>` element + Normally, it would wrap the two selected blocks (`<y>` and `<z>`) with a `<blockQuote>` element
Line range hint
736-736: Insert a comma after "code".- specifically for your scenario by your code which makes their implementation much easier + specifically for your scenario by your code, which makes their implementation much easier
Line range hint
747-747: Consider revising the wordy expression to be more direct.- The reason for this is that when you implement a command or any other feature + This is because when you implement a command or any other feature
Line range hint
748-748: Insert a comma after "feature".- when you implement a command or any other feature you may need to perform multiple operations + when you implement a command or any other feature, you may need to perform multiple operations
Line range hint
748-748: Consider revising "in the meantime" to a more concise alternative like "meanwhile".- The state in the meantime (between these atomic operations) may be incorrect + The state meanwhile (between these atomic operations) may be incorrect
Line range hint
767-767: Insert a comma after "Thanks to that".- Thanks to that if you tried to load incorrect data to the editor + Thanks to that, if you tried to load incorrect data to the editorpackages/ckeditor5-engine/src/model/schema.ts (1)
929-986: Optimize the schema compilation process for performance.The schema compilation process in the
_compilemethod involves multiple iterations over potentially large sets of schema definitions. Consider optimizing this process by reducing the number of iterations and exploring more efficient data structures or algorithms. This could significantly improve the performance, especially for large schemas.
| it( 'copies definitions objects', () => { | ||
| const definitions = {}; | ||
|
|
||
| schema.register( 'foo', definitions ); | ||
|
|
||
| definitions.isBlock = true; | ||
|
|
||
| expect( schema.getDefinitions().foo ).to.not.have.property( 'isBlock' ); | ||
| expect( schema.getDefinitions().foo.isBlock ).to.be.false; |
There was a problem hiding this comment.
Ensure that the definitions object is properly cloned to avoid unintended side effects.
- const definitions = {};
+ const definitions = { ...{} };Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| it( 'copies definitions objects', () => { | |
| const definitions = {}; | |
| schema.register( 'foo', definitions ); | |
| definitions.isBlock = true; | |
| expect( schema.getDefinitions().foo ).to.not.have.property( 'isBlock' ); | |
| expect( schema.getDefinitions().foo.isBlock ).to.be.false; | |
| it( 'copies definitions objects', () => { | |
| const definitions = { ...{} }; | |
| schema.register( 'foo', definitions ); | |
| definitions.isBlock = true; | |
| expect( schema.getDefinitions().foo.isBlock ).to.be.false; |
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (9)
packages/ckeditor5-engine/docs/framework/deep-dive/schema.md (9)
Line range hint
13-13: Consider revising "in regards of" to "with regard to" for better clarity.- atomicity in regards of external actions + atomicity with regard to external actions
Line range hint
585-585: Consider revising "despite the fact that" to "although" for conciseness.- despite the fact that the block quote and paragraph features + although the block quote and paragraph features
Line range hint
587-587: Remove the comma before "because" as it is not necessary.- rule), because `<$container>` is also allowed in `<$root>` + rule) because `<$container>` is also allowed in `<$root>`
Line range hint
680-680: Remove the extra determiner "the" for grammatical correctness.- which is the a base method used to check + which is the base method used to check
Line range hint
718-718: Add a comma after "Normally" for better readability.- Normally it would wrap the two selected blocks + Normally, it would wrap the two selected blocks
Line range hint
747-747: Consider revising "The reason for this is that" to "This is because" for conciseness.- The reason for this is that when you implement a command or any other feature + This is because when you implement a command or any other feature
Line range hint
748-748: Consider revising "in the meantime" to "meanwhile" for conciseness.- The state in the meantime (between these atomic operations) + The state meanwhile (between these atomic operations)
Line range hint
25-25: Replace hard tabs with spaces for consistency.- <code> + <code>Also applies to: 35-35, 43-45, 53-54, 62-64, 71-71, 82-83, 145-148, 149-150, 151-152, 153-154, 155-156, 157-158, 159-160, 161-162, 163-164, 165-166, 167-168, 169-170, 171-172, 173-174, 175-176, 177-178, 179-180, 181-182, 183-184, 185-186, 187-188, 189-190, 191-192, 193-194, 195-196, 197-198, 199-200, 201-202, 203-204, 205-206, 207-208, 209-210, 211-212, 213-214, 215-216, 217-218, 219-220, 221-222, 223-224, 225-226, 227-228, 229-230, 231-232, 233-234, 235-236, 237-238, 239-240, 241-242, 243-244, 245-246, 247-248, 249-250, 251-252, 253-254, 255-256, 257-258, 259-260, 261-262, 263-264, 265-266, 267-268, 269-270, 271-272, 273-274, 275-276, 277-278, 279-280, 281-282, 283-284, 285-286, 287-288, 289-290, 291-292, 293-294, 295-296, 297-298, 299-300, 301-302, 303-304, 305-306, 307-308, 309-310, 311-312, 313-314, 315-316, 317-318, 319-320, 321-322, 323-324, 325-326, 327-328, 329-330, 331-332, 333-334, 335-336, 337-338, 339-340, 341-342, 343-344, 345-346, 347-348, 349-350, 351-352, 353-354, 355-356, 357-358, 359-360, 361-362, 363-364, 365-366, 367-368, 369-370, 371-372, 373-374, 375-376, 377-378, 379-380, 381-382, 383-384, 385-386, 387-388, 389-390, 391-392, 393-394, 398-398, 399-400, 416-416, 423-423, 434-434, 440-440, 444-444, 446-448, 460-460, 462-462, 474-474, 483-483, 490-490, 501-501, 510-510, 519-519, 523-523, 527-528, 532-534, 538-541, 545-547, 555-555, 563-566, 581-581, 590-590, 599-599, 600-600, 601-601, 602-602, 603-603, 604-604, 605-605, 606-606, 607-607, 608-608, 616-616, 617-617, 618-618, 619-619, 620-620, 621-621, 622-622, 623-623, 624-624, 625-625, 626-626, 627-627, 628-628, 629-629, 630-630, 631-631, 632-632, 633-633, 634-634, 635-635, 636-636, 637-637, 638-638, 639-639, 640-640, 641-641, 649-649, 650-650, 651-651, 652-652, 653-653, 654-654, 655-655, 656-656, 657-657, 658-658, 659-659, 660-660, 661-661, 662-662, 663-663, 664-664, 665-665, 666-666, 667-667, 668-668, 669-669, 670-670, 671-671, 672-672, 673-673, 674-674, 688-688, 689-689, 691-691, 692-692, 693-693, 694-694, 695-695, 712-712, 713-713, 714-714, 722-722, 723-723, 724-724, 725-725, 726-726, 770-770, 775-775, 780-780, 784-785, 789-789, 793-793, 797-797, 803-804, 808-809, 813-817
Line range hint
436-436: Add blank lines before and after fenced code blocks for better readability.436a437 + 446a447 + 462a463 +
| ## Disallowing structures | ||
|
|
||
| The schema, in addition to allowing certain structures, can also be used to ensure some structures are explicitly disallowed. This can be achieved with the use of disallow rules. |
There was a problem hiding this comment.
Add a blank line before the heading for better readability.
74a75
+Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ## Disallowing structures | |
| The schema, in addition to allowing certain structures, can also be used to ensure some structures are explicitly disallowed. This can be achieved with the use of disallow rules. |
Disallowing structures
The schema, in addition to allowing certain structures, can also be used to ensure some structures are explicitly disallowed. This can be achieved with the use of disallow rules.
</details>
<!-- suggestion_end -->
<!-- This is an auto-generated comment by CodeRabbit -->
| @@ -71,6 +71,70 @@ Both the `{@link module:engine/model/schema~SchemaItemDefinition#allowIn}` and ` | |||
| You can read more about the format of the item definition in the {@link module:engine/model/schema~SchemaItemDefinition} API guide. | |||
| </info-box> | |||
|
|
|||
| ## Disallowing structures | |||
|
|
|||
| The schema, in addition to allowing certain structures, can also be used to ensure some structures are explicitly disallowed. This can be achieved with the use of disallow rules. | |||
There was a problem hiding this comment.
Consider revising "made out of" to "composed of" for conciseness.
- Generally speaking, content is usually made out of blocks
+ Generally speaking, content is usually composed of blocksCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| The schema, in addition to allowing certain structures, can also be used to ensure some structures are explicitly disallowed. This can be achieved with the use of disallow rules. | |
| The schema, in addition to allowing certain structures, can also be used to ensure some structures are explicitly disallowed. This can be achieved with the use of disallow rules. | |
| Generally speaking, content is usually composed of blocks |
| schema.extend( 'baseChild', { disallowIn: 'extendedParent' } ); | ||
| ``` | ||
|
|
||
| This changes how schema rules are resolved. `baseChild` will still be disallowed in `extendedParent` as before. But now, `extendedChild` will be disallowed in `extendedParent` as well. That's because it will inherit this rule from `baseChild`, and there is no other rule that would allow `extendedChild` in `extendedParent`. |
There was a problem hiding this comment.
Remove the trailing space for consistency.
-
+ Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| This changes how schema rules are resolved. `baseChild` will still be disallowed in `extendedParent` as before. But now, `extendedChild` will be disallowed in `extendedParent` as well. That's because it will inherit this rule from `baseChild`, and there is no other rule that would allow `extendedChild` in `extendedParent`. | |
| This changes how schema rules are resolved. `baseChild` will still be disallowed in `extendedParent` as before. But now, `extendedChild` will be disallowed in `extendedParent` as well. That's because it will inherit this rule from `baseChild`, and there is no other rule that would allow `extendedChild` in `extendedParent`. |
Suggested merge commit message (convention)
Feature (engine):
Schemanow supports disallowing items. IntroducedSchemaItemDefinition#disallowIn,#disallowChildrenand#disallowAttributes. Closes #15835.