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

Introduce RawElement (aka ShadowElement) #4469

Closed
Reinmar opened this issue Jan 23, 2019 · 14 comments · Fixed by #7621
Closed

Introduce RawElement (aka ShadowElement) #4469

Reinmar opened this issue Jan 23, 2019 · 14 comments · Fixed by #7621
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. package:engine type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@Reinmar
Copy link
Member

Reinmar commented Jan 23, 2019

See this comment: https://github.com/ckeditor/ckeditor5-engine/issues/1643#issuecomment-456814970

The name UIElement is misleading because this element can also be used to e.g. output raw content to the data.

We also need to rename createUIElement() and perhaps some other helpers.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 23, 2019

BTW, I know there were some other ideas for this name – CustomElement, HTMLElement... Maybe RawHTMLElement? Do you have other ideas?

@jodator
Copy link
Contributor

jodator commented Jan 23, 2019

As others I also think that RawElement isn't the element name as for me it has strong anotation to the "raw content" mentioned above. After some thought I came to a conclusion that such component will:

  • not visible to the model - it is a visual element that exists in the editing element but is not part of data
  • interactable - as above user can interact with it (possibly changing the model)

As such my wild propositions are:

  1. ShadowElement ? (like shadow DOM)
  2. Black box - you can interact with it but you shouldn't know the internals
  3. PresentationalElement - too long probably
  4. MetaElement - as from Greek or as in metadata

@jodator
Copy link
Contributor

jodator commented Jan 23, 2019

VirtualElement

@scofalik
Copy link
Contributor

scofalik commented Jan 23, 2019

Mmm.. not really.

First of all, you didn't give any thought to just adding a new element. UIElement and this new element have different semantical meaning and different use cases.

UIElements, indeed, aren't linked to the model. They don't exist in the model. They have special handling in some algorithms. The only common thing they have with the new element type is that you can render any HTML in them without worrying about conversion and it is treated as one atomic block.

However, the new element can be linked to the model and it probably should be able to be widgetized.

I am for creating a new type of element and name it CustomHTMLElement. Let's not be fancy about it and give it a name exact to this element's role.

@jodator
Copy link
Contributor

jodator commented Jan 23, 2019

However, the new element can be linked to the model and it probably should be able to be widgetized.

Maybe I'm missing something but I think that the widget's element and what is linked to the model would be a parent to CustomHTMLElement. Like drag handler for table (its is UIElement in widget) or some crazy form for changing model's attributes (rendered as <div> with CustomUIElement inside).

Edit: but I'm fine with CustomHTMLElement name.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 23, 2019

Aaaand, after a long F2F discussion we came to the following conclusions:

  1. We need an additional type of element (that we called RawElement) which would be a combination of UIElement (you control its rendering) and EmptyElement (it's understood as a non-dividable entity from outside). This may help in representing cases which we right now represent by a combination of WidgetizedContainerElement > UIElement.
  2. The new type will also help in cases where we need to output some raw HTML to the data.
  3. This means that we won't need to use UIElement in the data pipeline. Hence, renaming is unnecessary.
  4. Strictly speaking, you can leave without RawElement too (workaround with UIElement) hence we don't need to do anything here unless we'll see that we need to use this workaround too often.

@Reinmar Reinmar changed the title Rename UIElement to RawElement Introduce RawElement (aka ShadowElement) Jan 23, 2019
@Reinmar
Copy link
Member Author

Reinmar commented Jan 23, 2019

  1. ShadowElement ? (like shadow DOM

❤️

@jodator
Copy link
Contributor

jodator commented Jul 24, 2019

Now I see the use-case for RawElement (ShadowElement or CustomHTMLElement): the Media embed. Now it is using UIElement but this produces side-effect - the content appears to placeholder feature to be empty while it is not.

After reading this again I get the distinction between UIElement and the RawElement here - the RawElement maps directly to model element and it acts similarly as EmptyElement but has custom HTML rendered inside (Just like UIElment does).

@Reinmar
Copy link
Member Author

Reinmar commented Jul 24, 2019

the RawElement maps directly to model element

That's not necessarily true. You can have WidgetizedContainerElement (figure) > RawElement (div). In this case the outer container will be mapped. RawElement will not be mapped. Or – does not have to be mapped.

But the distinction makes sense:

  • UIElement – transparent from the view's perspective. Treated as "no content". UIElement cannot be mapped to anything in the model because everything in the model must map to something "visible" in the view.
  • RawElement – like EmptyElement but with custom rendering. Considered a "content" in the view. Thus, can be mapped to something in the model.

Questions:

  • Can RawElement be widgetized? Currently, there's a limitation that we had to do this: WidgetizedContainerElement > UIElement because UIElement cannot be mapped to anything in the model so it cannot represent a widget. So, can RawElement be widgetized? The answer is – only if your custom render() function controls only its children, not the element itself. In UIElement you render the element itself as you wish. That would not work with toWidget(). Do you think that RawElement's custom rendering could be limited only to its children?
  • Couldn't we implement "custom rendering" as a feature of EmptyElement? That would, perhaps, spare us some work because there might be less code to update. From outside, there will be no difference between EmptyElement and RawElement so this kinda makes sense. However, this idea is fishy from a semantic perspective because empty means empty. But if it would save us a lot of time, I think we could actually consider it.

@jodator
Copy link
Contributor

jodator commented Jul 24, 2019

* function controls only its children, not the element itself.

I think of the RawElement just like this.

* Couldn't we implement "custom rendering" as a feature of ``

I kinda lean towards this actually. The EmptyElement is empty from a View POV - it does not have any children but it can be represented in the editing with children.

Still I'm undecided :/

@pjasiun
Copy link

pjasiun commented Jul 24, 2019

👍 for separate RawElement
👍 for only custom content and attributes handled by the view.

@jodator jodator self-assigned this Aug 5, 2019
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the backlog milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:engine labels Oct 9, 2019
@jodator
Copy link
Contributor

jodator commented Oct 15, 2019

A simple use-case for RawElement - passing non-editable regions of the content: #5566 (comment).

@Reinmar Reinmar added domain:dx This issue reports a developer experience problem or possible improvement. squad:red labels May 29, 2020
@Reinmar
Copy link
Member Author

Reinmar commented May 29, 2020

I stumbled upon this again. Similar case to what @jodator posted above.

@jodator jodator removed their assignment Jun 22, 2020
@jodator
Copy link
Contributor

jodator commented Jun 22, 2020

Let's not forget about this fix: #1684.

@Reinmar Reinmar removed this from the backlog milestone Jul 7, 2020
@Reinmar Reinmar added this to the iteration 34 milestone Jul 7, 2020
Reinmar added a commit that referenced this issue Jul 22, 2020
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).
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 type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants