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.

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

Make sure we warn in cases when conversion events were stopped #10377

Closed
niegowski opened this issue Aug 18, 2021 · 1 comment · Fixed by #11211
Closed

Make sure we warn in cases when conversion events were stopped #10377

niegowski opened this issue Aug 18, 2021 · 1 comment · Fixed by #11211
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. package:engine squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@niegowski
Copy link
Contributor

niegowski commented Aug 18, 2021

This change is necessary after #10376 when we changed the flow of events. Now, stopping a conversion event may lead to children/attributes of that node not being converted.

Old notes from @niegowski

First, we thought about blocking stopping events, but then, after numerous discussions around this we realized it's not the right direction. Check the first comment under this ticket.

In order to be able to properly fall back to child nodes conversion, we must ensure that the event is not stopped before the handler at lowest priority is called. In the conversion process, we are using the consumables concept that allows more granular handling of what is still there to be processed. In the API docs, we are strictly saying, that those events should be stopped, but there was a change in the code that removed that to allow converters that work as side effect triggers.

Since we are mentioning this outdated behavior in the API docs I would suggest making events fired by DowncastDispatcher and UpcastDispatcher not stoppable to avoid confusion.

This would be a good improvement for #10376 to make sure that the conversion will still work after the changes.

@niegowski niegowski added type:task This issue reports a chore (non-production change) and other types of "todos". domain:dx This issue reports a developer experience problem or possible improvement. squad:compat labels Aug 18, 2021
@Reinmar Reinmar added squad:core Issue to be handled by the Core team. and removed squad:compat labels Sep 27, 2021
@Reinmar
Copy link
Member

Reinmar commented Sep 27, 2021

Change of plans. Blocking stopping the events is not feasible because we stop them in at least one case (table converters). Instead, try resolving the old #3818.

Notes from the arch guild meeting

  • Maybe we could resolve the old issue where we wanted to warn if you didn't consume some model change (didn't convert it)?
    • This would catch the above-discussed situation on a bit later stage. Still, we can document what might've happened (three options: forgot to write a converter, stopped the event, incorrect converter that forgot to consume).
    • We can't assume that attributes are consumed and converted. Upload status, GHS, etc. prove that. Attributes may affect only the data.
    • But structure changes are crucial.
    • This: There should be error when there is no converter to the model element #3818 💀
    • Are there any technical challenges here?
    • Can we make the warning useful (do we have all information there)?
    • Can we quickly prototype it?
      • Should be doable inside the Dispatcher
      • Let's check how many warnings (in automated tests and all-features manual sample) we'll get
    • Update (14.09): It worked fine in the PoC
      • Some things were not consumed, but nothing critical.
      • Compat will continue this.

Refinement

@Reinmar Reinmar changed the title Event's fired by conversion dispatchers should not be stoppable Make sure we warn in cases when conversion events were stopped Sep 27, 2021
@niegowski niegowski self-assigned this Sep 27, 2021
@Reinmar Reinmar added this to the iteration 48 milestone Sep 27, 2021
Reinmar added a commit that referenced this issue Oct 4, 2021
Feature (engine): The `DowncastDispatcher` will throw an error if any of the model items were not consumed while converting. Closes #10377.

Tests (clipboard): Added missing downcast conversion.

Internal (heading): Added missing consuming converter.

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

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`](https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#conversion-model-consumable-not-consumed) error documentation for more information.
@Reinmar Reinmar closed this as completed Oct 4, 2021
dawidurbanski added a commit that referenced this issue Feb 9, 2022
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.
@Reinmar Reinmar modified the milestones: iteration 48, iteration 51 Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. package:engine squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants