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

Change the order/flow of events fired by the DowncastDispatcher #10376

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

Change the order/flow of events fired by the DowncastDispatcher #10376

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

Provide a description of the task

Currently DowncastDispatcher is firing events for the inserted range by walking on the range with TreeWalker so for the provided structure:

<x foo="bar">
	<y bar="123" baz="abc">
		foobar
	</y>
</x>

It fires events like a flat sequence:

insert:x
attribute:foo:x
insert:y
attribute:bar:y
attribute:baz:y
insert:$text

but if any of the elements' converter uses slots (and with this triggers conversion of nested element) the flow becomes mixed:

insert:x
	insert:y
	attribute:bar:y
	attribute:baz:y
	insert:$text
attribute:foo:x

The x element is triggering its children's conversion by using slots. This makes the order of events different, the flow becomes partly recursive (for the insert:x). This could be a complication hard to understand for developers. Also with a flat sequence of events, there is no way to listen on the whole element with children got converted (that could be useful for side effects that could listen on a low priority to notice when that conversion is completed).

In the UpcastDispatcher the flow is different, it's always recursive - there is the lowest priority listener that triggers nested nodes conversion (and also many converters can trigger it if appropriate).

I propose to align the DowncastDispatcher to the same recursive flow as it's used in the UpcastDispatcher so the sequence of events would look like this:

insert:x
	insert:y
		insert:$text
	attribute:bar:y
	attribute:baz:y
attribute:foo:x

or maybe even to:

insert:x
	attribute:foo:x
	insert:y
		attribute:bar:y
		attribute:baz:y
		insert:$text

by introducing nested trigger for attributes conversion (sth similar to conversionApi.convertInsert()). This way the order of events will be exactly the same but conversion of attributes and child nodes would be nested (on the call stack) inside the parent insert event handler.

@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
@niegowski niegowski changed the title Change the order of events fired by the DowncastDispatcher Change the order/flow of events fired by the DowncastDispatcher Aug 18, 2021
@Reinmar
Copy link
Member

Reinmar commented Sep 16, 2021

Notes from a f2f meeting:

  • We agreed that the change makes sense, which is a kind of success, I think.
  • There are some risks, though:

Existing features (also 3rd-party) that hook in downcast events on different priorities could break if we touch anything.

Approach A: Some features may assume that parent's (x) attributes have been converted before converting a child (y). If we go with the following order, this will break:

insert:x
	insert:y
		insert:$text
	attribute:bar:y
	attribute:baz:y
attribute:foo:x

Approach B: The following looks safer than 👆 because conversion for y will know about attributes of x. There's some downside to this: listeners on low priority for x may break because children would be converted before they get a chance to act.

insert:x
	attribute:foo:x
	insert:y
		attribute:bar:y
		attribute:baz:y
		insert:$text
  • This approach is similar to upcast, which could be OK because we're streamlining both processes.
  • Let's go with Approach B.
  • Should we fire events for elX's attrs and children if they were consumed already by elX's standard converter?
    • It'd be weird if these events weren't fired if some feature like sideBox consumed its children prematurely. This would mean that they are never fired. Hence, things like "side-effects" could not be implemented.
    • But: DowncastDispatcher doesn't fire them today.
    • But but: The current implementation may not make sense and this entier ticket is about making sense of it.

@Reinmar Reinmar added this to the iteration 47 milestone Sep 16, 2021
Reinmar added a commit that referenced this issue Sep 24, 2021
…event-flow

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.

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

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

Other (list): Downcast conversion should consume downcasted attributes.
@Reinmar Reinmar closed this as completed Sep 24, 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 47, iteration 51 Feb 11, 2022
@Reinmar Reinmar added squad:core Issue to be handled by the Core team. package:engine labels 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