#4469: Implemented the view RawElement #7621

oleq · 2020-07-14T07:47:19Z

Suggested merge commit message (convention)

Feature (engine): Implemented the view RawElement. Implemented the DowncastWriter#createRawElement() method. Closes #4469.

Fix (media-embed): The editor placeholder should disappear after inserting media into an empty editor. Closes #1684.

Docs (ckeditor5): Used the RawElement instead of UIElement in the "Using a React component in a block widget" guide (see #1684).

Internal (media-embed): Removed the getFillerOffset() hack from the createMediaFigureElement() helper that is no longer needed since the media content is rendered using RawElements (see #1684).

BREAKING CHANGE (engine): The DomConverter#getParentUIElement() method was renamed to DomConverter#getHostViewElement() because now it supports both UIElement and RawElement (see #4469).

Additional information

TBH the conversion (mapping) to a RawElement should be possible but I don't know where to write tests for it because I couldn't find similar tests for other view element types. Any ideas?

…iew utils.

…ollides with media embed.

…widget" guide.

Reinmar · 2020-07-16T08:56:37Z

packages/ckeditor5-engine/src/view/domconverter.js

@@ -887,7 +896,8 @@ export default class DomConverter {
 	 *
 	 * The following places are considered as incorrect for selection boundaries:
 	 * * before or in the middle of the inline filler sequence,
-	 * * inside the DOM element which represents {@link module:engine/view/uielement~UIElement a view ui element}.
+	 * * inside the DOM element which represents {@link module:engine/view/uielement~UIElement a view UI element}.
+	 * * inside the DOM element which represents {@link module:engine/view/rawelement~RawElement a view raw element}.


Something went wrong here.

It renders as:

inside the DOM element which represents a view UI element. inside the DOM element which represents a view raw element

Duplicated "inside...". Should be joined with "and" or something.

Reinmar · 2020-07-16T08:59:03Z

packages/ckeditor5-engine/src/view/downcastwriter.js

+	 * Custom render function can be provided as third parameter:
+	 *
+	 *		writer.createRawElement( 'span', null, function( domDocument ) {
+	 *			const domElement = this.toDomElement( domDocument );


IMO, in the callback you should get the DOM element as in case of RawElement, unlike like in UIElement, you do not control the host element. It's being controlled by the view.

Reinmar · 2020-07-16T09:14:36Z

packages/ckeditor5-engine/src/view/downcastwriter.js

@@ -272,6 +273,36 @@ export default class DowncastWriter {
 		return uiElement;
 	}

+	/**
+	 * Creates a new {@link module:engine/view/rawelement~RawElement}.


Does it make sense to create raw elements without their "internals" (passing the callback)?

Also, I'd focus on having here a good cross-ref between UIElement and RawElement, explaining also briefly what's the difference, etc. These API methods have a high chance of being visited and it's hard to figure out what it is based on this method.

What about this? I can see that the callback is still optional.

I'll remove the first two examples and leave the rest as is to make it less confusing.

I can see it's a copy-paste from UI element... But it makes more sense there as sometimes UIElements can be used without any content as their purpose is different.

Oh, I remember now that you mentioned that it'd look bad if the last param wasn't optional since attrs are. So, that's fine. I'll just remove those examples.

packages/ckeditor5-engine/src/view/downcastwriter.js

packages/ckeditor5-engine/src/view/rawelement.js

Reinmar · 2020-07-16T09:18:28Z

packages/ckeditor5-engine/src/view/rawelement.js

+			 * @error view-rawelement-cannot-add
+			 */
+			throw new CKEditorError(
+				'view-rawelement-cannot-add: Cannot add child nodes to RawElement instance.',


to a RawElement instance

Reinmar · 2020-07-16T09:18:37Z

packages/ckeditor5-engine/src/view/rawelement.js

+	_insertChild( index, nodes ) {
+		if ( nodes && ( nodes instanceof Node || Array.from( nodes ).length > 0 ) ) {
+			/**
+			 * Cannot add children to a {@link module:engine/view/rawelement~RawElement}.


missing "instance" or remove "a" :D

Reinmar · 2020-07-16T09:21:07Z

packages/ckeditor5-engine/tests/view/renderer.js

+
+				expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( normalizeHtml(
+					'<p>Foo<span><span id="id2"><b>RAW2</b></span></span> Bar</p>' ) );
+			} );


I'm missing a test of updating a raw element's attributes via the view.

Reinmar · 2020-07-16T09:26:01Z

I left a couple of comments. The two biggest things are:

The form of the raw element's render callback.
A missing test for using toWidget( rawElement ) and then whether e.g. the classes of this element are nicely updated when selection changes – as that's one of the things that wasn't possible before (you had to have a wrapper). Similar test can be written for the data pipeline – for checking that you can indeed implement something like "raw HTML" embedding Custom Html Widget #5566 (comment). These tests could land in the widget package as some integration tests, although the data pipeline test would not, in fact, be in any way related to widgets (as only the editing pipeline uses it).

Reinmar · 2020-07-16T09:26:04Z

I left a couple of comments. The two biggest things are:

The form of the raw element's render callback.
A missing test for using toWidget( rawElement ) and then whether e.g. the classes of this element are nicely updated when selection changes – as that's one of the things that wasn't possible before (you had to have a wrapper). Similar test can be written for the data pipeline – for checking that you can indeed implement something like "raw HTML" embedding Custom Html Widget #5566 (comment). These tests could land in the widget package as some integration tests, although the data pipeline test would not, in fact, be in any way related to widgets (as only the editing pipeline uses it).

Reinmar

As commented.

…ment rendering strategy.

…ing strategy.

…ndering strategy.

…a React component in a block widget" guide.

…" guide.

…on-ContainerElement instance.

…Element gets a default render() method if not specified.

…) docs.

docs/framework/guides/architecture/editing-engine.md

oleq added 12 commits July 13, 2020 10:20

Created a RawElement class.

f6d139d

Added support for RawElement in getData(), stringify(), and parse() v…

2deb00b

…iew utils.

Added RawElement support in the MutationObserver.

e694001

Made it possible to use the RawElement in the DomConverter.

eec1c98

Integrated the Renderer class with RawElement.

c1e0eba

Integrated the DowncastWriter with the RawElement.

3ddcc77

Made the Media Embed feature use RawElement instead of UIElement.

1932a7a

Tests: Added a test that verifies the placeholder feature no longer c…

50fbc80

…ollides with media embed.

Docs: Used the RawElement in the "Using a React component in a block …

b56be5d

…widget" guide.

Docs: Mentioned RawElement in the "Editing engine" guide.

c467a3b

Tests: Added a comment in media embed integration test.

a5208bf

Docs: Minor docs correction.

8271513

oleq requested a review from Reinmar July 14, 2020 07:53

oleq added 2 commits July 14, 2020 10:26

Tests: Added tests for RawElement class.

b3f21a1

Tests: Code refactoring.

bbe1ffc

Reinmar reviewed Jul 16, 2020

View reviewed changes

packages/ckeditor5-engine/src/view/downcastwriter.js Outdated Show resolved Hide resolved

Reinmar reviewed Jul 16, 2020

View reviewed changes

packages/ckeditor5-engine/src/view/rawelement.js Outdated Show resolved Hide resolved

Reinmar reviewed Jul 16, 2020

View reviewed changes

Reinmar requested changes Jul 16, 2020

View reviewed changes

oleq added 4 commits July 17, 2020 11:43

Merge branch 'master' into i/4469-view-raw-element

5aaffb6

Simplified RawElement#render(). Added docs to the class.

1fa577f

Tests: Updated RawElement tests after #render() simplification.

5e9cefb

Simplified the DowncastWriter#createRawElement() method.

bc1a607

oleq added 9 commits July 17, 2020 16:01

Made sure editing view creates the RawElement. Updated docs.

d7483a1

Tests: Updated view stringify test helper after changes to the RawEle…

60476ef

…ment rendering strategy.

Tests: Updated view renderer tests after changes to RawElement render…

777a1b3

…ing strategy.

Tests: Updated mutation observer tests after changes to RawElement re…

f298535

…ndering strategy.

Used the new DowncastWriter#createRawElement API in the MediaRegistry.

1d771af

Docs: Used the new DowncastWriter#createRawElement API in the "Using …

ff1d7e2

…a React component in a block widget" guide.

Docs: Improved the description of raw elements in the "Editing engine…

9969d1e

…" guide.

toWidget() should throw when attempting to create a widget out of a n…

ea42b0d

…on-ContainerElement instance.

Tests: Added a DowncastWriter#createRawElement() test to check if Raw…

4885381

…Element gets a default render() method if not specified.

oleq requested a review from Reinmar July 21, 2020 09:24

oleq and others added 4 commits July 22, 2020 09:45

Merge branch 'master' into i/4469-view-raw-element

2b91300

Docs: Updated DowncastWriter#createUIElement() and #createRawElement(…

a13b288

…) docs.

Merge branch 'master' into i/4469-view-raw-element

9b23bf7

Removed confusing examples.

b224e97

Reinmar approved these changes Jul 22, 2020

View reviewed changes

Reinmar merged commit bff38e3 into master Jul 22, 2020

Reinmar deleted the i/4469-view-raw-element branch July 22, 2020 20:44

godai78 reviewed Jul 31, 2020

View reviewed changes

docs/framework/guides/architecture/editing-engine.md Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4469: Implemented the view RawElement #7621

#4469: Implemented the view RawElement #7621

oleq commented Jul 14, 2020 •

edited

Loading

Reinmar Jul 16, 2020

Reinmar Jul 17, 2020 •

edited

Loading

Reinmar Jul 16, 2020

Reinmar Jul 16, 2020

Reinmar Jul 22, 2020

Reinmar Jul 22, 2020

Reinmar Jul 22, 2020

Reinmar Jul 16, 2020

Reinmar Jul 16, 2020

Reinmar Jul 16, 2020

Reinmar commented Jul 16, 2020

Reinmar commented Jul 16, 2020

Reinmar left a comment

#4469: Implemented the view RawElement #7621

#4469: Implemented the view RawElement #7621

Conversation

oleq commented Jul 14, 2020 • edited Loading

Suggested merge commit message (convention)

Additional information

Choose a reason for hiding this comment

Reinmar Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Jul 16, 2020

Reinmar commented Jul 16, 2020

Reinmar left a comment

Choose a reason for hiding this comment

oleq commented Jul 14, 2020 •

edited

Loading

Reinmar Jul 17, 2020 •

edited

Loading