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.

Sign up for GitHub

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

Jump to bottom

Secure slotFor() use for cases where only elements are used #11163

Closed
Reinmar opened this issue Jan 26, 2022 · 4 comments · Fixed by #11211
Closed

Secure slotFor() use for cases where only elements are used #11163

Reinmar opened this issue Jan 26, 2022 · 4 comments · Fixed by #11211
Assignees
@oleq
Labels
package:engine squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.
Milestone
iteration 51 (v33...

Comments

@Reinmar
Copy link
Member

Reinmar commented Jan 26, 2022

Extracted from #11149.

For now, we need to go for:

  • D: Make slots handle only elements, not text nodes.
    • Reasonable for now.
    • Throw when a text node was encountered by slotFor().

Source: #11149 (comment)

Scope: The slot mechanism should throw an error when encountering a text node.
@Reinmar Reinmar added type:improvement This issue reports a possible enhancement of an existing feature. package:engine squad:core Issue to be handled by the Core team. labels Jan 26, 2022
@Reinmar Reinmar mentioned this issue Jan 26, 2022
Closed
@Reinmar
Copy link
Member Author

Reinmar commented Jan 31, 2022 

        model: 'myElement',

IF POSSIBLE in 10 minutes (if we have access to the schema): We may check whether the schema allows $text in myElement. If so, throw immediately.

If not possible, check&throw when the slot is being filled with nodes.

@oleq oleq self-assigned this Feb 7, 2022
@oleq oleq mentioned this issue Feb 7, 2022
Merged
niegowski added a commit that referenced this issue Feb 7, 2022
@niegowski
Merge pull request #11229 from ckeditor/ck/11163-disallowed-text-in-e…
03fa4d0 
…lement-to-structure

Internal (engine): `elementToStructure()` should throw when invoked for an element that allows `$text`. Closes #11163.
@niegowski
Copy link
Contributor

Closed in #11229.

@oleq oleq reopened this Feb 7, 2022
@oleq
Copy link
Member

oleq commented Feb 7, 2022

The #11229 has been merged and then reverted due to failing tests of the Widget plugin. Turns out, there were some tests that assumed that text can be allowed in an element that gets converted via elementToStructure(). Let's figure out whether the use case was valid and how many integrations would be affected if we went with an error in this case

ckeditor5/packages/ckeditor5-widget/tests/widget.js

Lines 232 to 239 in 92595b2

expect( getViewData( view ) ).to.equal(
'[<div class="ck-widget ck-widget_selected" ' +
'contenteditable="false">' +
'foo bar' +
'<b></b>' +
'<div class="ck ck-reset_all ck-widget__type-around"></div>' +
'</div>]'
);

This was referenced Feb 8, 2022
Merged
Closed
@oleq oleq mentioned this issue Feb 8, 2022
Merged
oleq added a commit that referenced this issue Feb 8, 2022
@oleq
Merge pull request #11239 from ckeditor/revert-11233-revert-11229-ck/…
2a237b7 
…11163-disallowed-text-in-element-to-structure

Internal (engine): `elementToStructure()` should throw when invoked for an element that allows `$text`. Closes #11163.
@oleq
Copy link
Member

oleq commented Feb 8, 2022

An update on the revert of the revert 🙃: #11239 (comment)

@oleq oleq closed this as completed Feb 8, 2022
@Reinmar Reinmar added this to the iteration 51 milestone Feb 9, 2022
dawidurbanski added a commit that referenced this issue Feb 9, 2022
@dawidurbanski
Merge pull request #11211 from ckeditor/ck/10294/reconversion
9f17c59 
Feature (engine): The DowncastWriter#createContainerElement() should accept a list of children so bigger view structures can be created in one call. Closes #10714.

Feature (engine): The elementToElement downcast helper will log a console warning if multiple elements have been created. Closes #10610.

Feature (engine): The DowncastDispatcher will throw an error if any of the model items were not consumed while converting. Closes #10377.

Feature (engine): Introducing convertItem(), convertChildren() and convertAttributes() in the downcast conversion API interface.

Feature (engine): Added support for reconversion in DowncastHelpers#elementToElement() downcast helper. Closes #10359.

Feature (engine): Added DowncastHelpers#elementToStructure() downcast helper with reconversion support. Closes #10358.

Feature (engine): It's now possible to trigger a nested conversion while downcasting an element.

Docs (engine): Overhauled the conversion documentation and introduced the documentation for conversion helpers. Closes #10294.

Internal (engine): elementToStructure() should throw when invoked for an element that allows $text. Closes #11163.

Internal (engine): Replaced conversionApi.slotFor() in elementToStructure with writer.createSlot(). Closes #11179.

Internal (engine): Reconversion helpers can coexist with Differ#refreshItem() children.

Other (engine): Implemented the EditingController#reconvertMarker() method to be used instead of Writer#updateMarker() for marker reconversion purposes. Implemented the EditingController#reconvertItem() method to replace Differ#refreshItem(). Closes #10659.

Other (engine): The attribute and child nodes conversion events are fired by the lowest priority handler for the insert event instead of by DowncastDispatcher itself. Closes #10376.

Other (engine): Events are fired by the DowncastDispatcher even if they were previously consumed. It's the conversion handler's responsibility to check if it can be consumed or if it was already consumed by other converters.

Other (engine): The DowncastDispatcher#convert() method introduced as a replacement to previously used convertInsert(). The new method handles not only nodes conversion but also markers.

Internal (horizontal-line): The horizontalLine element downcast conversion has been changed from elementToElement to elementToStructure.

Internal (html-support): HTML elements downcast conversion have been changed from elementToElement to elementToStructure.

Internal (image): The imageBlock and imageInline elements downcast conversion have been changed from elementToElement to elementToStructure.

Internal (link): Link inlineWidget element downcast conversion has been changed from elementToElement to elementToStructure.

Internal (media-embed): The media element downcast conversion has been changed from elementToElement to elementToStructure.

Internal (page-break): The pageBreak element downcast conversion has been changed from elementToElement to elementToStructure.

Internal (widget): The blockWidget and inlineWidget elements downcast conversion have been changed from elementToElement to elementToStructure.

Internal (heading): Added missing consuming converter.

Internal (heading): Code adjusted to changes in DowncastDispatcher API.

Internal (html-embed): Code adjusted to the removal of triggerBy conversion option.

Other (list): The ckeditor5-list package was restructured into subdirectories. Closes #10811.

Other (list): Downcast conversion should consume downcasted attributes.

Other (table): Table downcast conversion migrated to elementToStructure() downcast helper. Closes #10502.

Tests (clipboard): Added missing downcast conversion.

Tests (image): Tests updated to properly handle non-consumed errors fired by the DowncastDispatcher.

Tests (alignment, autoformat, block-quote, engine, html-support, image, indent, table, word-count): Tests updated after list package restructuring.

Tests (html-support): Fixed test for checking if an attribute was consumed while converting.

Docs (ckeditor5): Links in the migration guide point to the new conversion documentation.

MAJOR BREAKING CHANGE (list): The ListEditing, ListUI, ListStyleEditing, ListStyleUI, TodoListEditing, TodoListUI plugins were moved to the dedicated subdirectories (list, liststyle, todolist).

MAJOR BREAKING CHANGE (engine): Removed the Differ#refreshItem() method from the public API. Replaced by EditingController#reconvertItem() (see #10659).

MAJOR BREAKING CHANGE (engine): The DowncastDispatcher will throw an error if any of the model items were not consumed while converting. Read the conversion-model-consumable-not-consumed error documentation for more information.

MAJOR BREAKING CHANGE (engine): The DowncastDispatcher#conversionApi property is no longer available. The instances of DowncastConversionApi being created at the start of conversion.

MAJOR BREAKING CHANGE (engine): The support for the triggerBy option for downcast helpers is removed and replaced with the new elementToStructure() options.

MINOR BREAKING CHANGE (media-embed): The createMediaFigureElement helper function first argument has been changed from writer object to conversionApi object.

MINOR BREAKING CHANGE (table): The downcast converters of the table feature has been rewritten with the use of elementToStructure() and the re-conversion mechanism. See #10502.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@oleq oleq

Labels
package:engine squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Milestone
iteration 51 (v33.0.0)
Development

Successfully merging a pull request may close this issue.

New reconversion system ckeditor/ckeditor5
3 participants
@Reinmar @oleq @niegowski