ElementCollection should mount children's dom elements instead of Element itself

[revyh]

Hi!

For now Element inserts itself into the parent element. I think it should be quite opposite - this code should be in the ElementCollection.

First, it will give a better distribution of responsibilities. No longer need to access the private fields of the parent element in Element::mount.

Secondly, it will facilitate extension of ElementCollection. For example, the integration of some carousel library in CE. Most likely it would require a new element - CarouselElement which is a thin wrapper, adapter over the carousel library's api and represents a collection of slides (and therefore inherited from ElementCollection). In particular, the addition of the slide's DOM elements in the document should take place through the library's api and thus be part of CarouselElement. It can be easily implemented through the overloading of attach method on CarouselElement. But with the current realization you still need to redefine Element::mount (and maybe some other elements), so it only creates DOM element and doesn't attaches it to the document if parent is CarouselElement. Naturally this is not the best approach.

Thirdly, it will simplify the implementation of the Fixture. Instead, to check whether the parent is Fixture, you can simply inherit the Fixture from ElementCollection and override the mount.

The same applies to unmount.

I do not know how many changes and backward incompatibilities it can cause. At least, in CT mount should not be called on the focused element itself, but on its container.

What do you think about it?

[anthonyjb]

Hi @revyh,

Thanks for taking the time to review the code and make a thoughtful comment to open a discussion about your suggested change 👍

First let me say I think this is worthy of discussion and whilst I haven't directly experienced an issue with the current approach while implementing element classes I don't want it to be difficult for others to extend so I'm keen to hear how best I can ease this process.

My reading from your comment is that instead of mount, each element will have a method (perhaps called buildEditDOM) which returns the DOM element and that a parent element collection takes responsibility for physically mounting the DOM in it's mount method?

Note on fixtures The fixture API is due to change, the access of the private _domElement attribute for this is rushed and needs to be rethought. It should either be set using a specific method (e.g. Fixture.updateDOMElement(domElement), or more likely I will simply have the fixture's domElement() method return the childs domElement() method and tidy up areas of CE that don't use the method but the property (e.g _domElement) directly.

Note on private properties and distribution of responsibilities Without the hacky fixture code there isn't access to any private properties, however to decouple we could simply pass a DOM element into the mount(domElement) method at which point the child wouldn't need to know anything about the parent at all to mount itself - however this isn't practical because the parent -> child relationship here is a dependency (that is the child is dependent on having a parent) - the type of child or parent doesn't matter but the interface and relationship do.

Where the responsibility for mounting the element to the DOM is a fair question? The way the tree of elements is set up everything inherits from the Element class including ElementCollection so whilst element collections support children they can also be a child and both classes support broadly the same interface. Lists for example, which can have any number of tiers, a list can be the child of a list item itself (so a list item is both a child and a parent). This isn't a reason for the DOM mounting to be done in mount rather than the child collections mount but it provides some background as to why the responsibility is broken up so that each element is responsible for mounting itself into the DOM rather than this being exclusively the responsibility of collections.

I know you're working on an element carousel example at the moment and we've discussed to some extent how best to approach this (whilst the question wasn't directly related to this issue). Can you provide a mock example of how you see this working in that scenario so I have something to play with, I find a good example is often the best way to decide on something. What you're doing with the carousel seems very similar to how a list might be constructed so I'm interested in how this approach improves things compared with a list.

I think there would be some issues with backwards compatibility with such a change but not so many that it would be hard to switch should we implement the change (we had a similar issue when switching to the new event system).

[revyh]

First let me say I think this is worthy of discussion

Glad to see that you are ready for the discussion. 👍

parent element collection takes responsibility for physically mounting the DOM in it's mount method

Yes, it seems you get it right. But I supposed to override mount, instead of creating a new method. On the other hand, mount which actually doesn't mounts anything in the document looks strange.

Note on fixtures
Note on private properties and distribution of responsibilities Without the hacky fixture code there isn't access to any private properties

AFAIK, in this case parent._domElement.replaceChild() and parent.domElement().ReplaceChild() are not so different. Correct me if I'm wrong, but CE Element class in fact is a wrapper over the dom element. Therefore, all the changes in the private field _domElement should be carried out only by methods of the Element wrapper. Getter domElement should only be used to retrieve additional information about dom element, not to change it.

whilst element collections support children they can also be a child and both classes support broadly the same interface

Good point. Actually, I thought about it before and I see no reasons why you need to separate elements from collections. Class Node quite possible to unite with NodeCollection and Element with ElementCollection. Again, if dom classes Node andElement can have children, why can not their CE wrappers Node and Element? Another example is React, which also doesn't separate elements and collections. You can also add a method (isLeaf for example) for easily understanding are there any children in Element or not.

So, If we assume that there is no separation beetween Element and ElementCollection, the approach to mounting dom elements may be next. Element should define a method (say createDOMElement)

# just an illustration
createDOMElement: () ->
  @_domElement = document.createElement(@_tagName)
  @_setAttributes()
  @_addDOMEventListeners()

  for child in @children()
    @ _domElement.appendChild Child.createDOMElement()

  @_domElement

All subclasses can specify it more accurately. This method only recursively creates dom tree, but doesn't mount it in document. To physically mount it should be sufficient mount andunmount methods only in Region.

Alternatively, instead of a single createDOMElement you can make two: createEditableDOMElement and createImmutableDOMElement. The first is used when CT in edit mode, and the second - after saving. createImmutableDOMElement can help to solve my other problem.

I find a good example is often the best way to decide on something

Absolutely agree. I'll try to provide gist example tomorrow.

[anthonyjb]

Thanks for the response, I look forward to the gist.

Just some quick thoughts,

parent._domElement.replaceChild() and parent.domElement().ReplaceChild()

I think this is a matter of interpretation. The point of asking external classes to use domElement() over _domElement is to indicate that they shouldn't change the property against the parent, not that they shouldn't call public methods against that property. This is the same for the .parent() method which returns an instance of the child's parent that you can call against (e.g child.parent().detatch(child)), the method is provided to indicate that the parent attribute shouldn't be changed directly not that it's public properties and methods shouldn't be accessed.

The createDOMElement and mount/unmount only being against region

This approach would seem to make it very difficult to change out a single element, for example if I want to drag an element from one position in a region to another having to call mount() against the region would require re-building the entire region DOM element, where as keeping mount against either the parent (as I initially thought you were suggesting) or the child (as is the case now) means you only remove and rebuild the element being moved.

Wrongly or rightly CE doesn't support use a virtual DOM in the sense that React does and has no way to determine how to only re-render the parts that have changed if you ask it to re-build the top level region - there's no immutable state structure in CE or virtual DOM to compare against when re-building.

createImmutableDOMElement

I understand this might just be an idea but as far as I'm aware there's no way to create a DOM element that's immutable in JS, if the function returned a DOM element it would be mutable? Even if the intention is to indicate to the user that this shouldn't be mutated (though I'm not clear from your description why this would be the case) I think you'd be better simply calling them createDOMElement and createEditableDOMElement IMO.

I believe your reasoning behind this is to allow relevant JS for a DOM element to be called at the time an element is re-inserted into the DOM on save, as at the moment save exports the region in HTML format and re-inserts it into the page. I think this is an interesting option regardless of the other points discussed here (it would also be easy to rig up and wouldn't break the API for older releases as they would still work with the html() method).

One thought on this though is that I'm concern about having JS logic for an element in non-editable mode within the element. The CE code base isn't typically present when non-editors view the content and so there's the potential for code that works in the editor to be missing or broken when the content is viewed without the CE code base.

Typically the way I solve this type of integration as we've discussed is binding to the saved event for the given page and adding the JS logic required to re-init any elements at that point - but the createDOMElement method would allow us to keep the relevant logic together for the element which I think overall is cleaner and an improvement.

I don't however think it will necessarily stop the flicker that you see when the page is saved and each region remounted as the same process is undertaken. The change in noticeable flicker would be purely be down to the difference in performance between innerHTML = '...' and manually creating the elements (manually creating is faster but I'm not sure it will prevent the flicker).

[revyh]

Seriously, I'm sorry. I personally don't like people who say "tomorrow" and disappear for a month. But I was very busy all this time.

Example

class ContentEdit.CarouselElement extends ContentEdit.ElementCollection
  constructor: (attributes) ->
     super 'div', attributes

     # instance of carousel class, supplied by some library
     @_carousel = new Carousel()

  mount: ->
    @_carousel.init()

    @_domElement = @_carousel.root()
    @domElement().setAttribute 'data-ce-tag', @type().toLowerCase()
    super

...

carousel = new ContentEdit.CarouselElement()
carousel.mount()

# at this point, the `carousel._domElement` looks like this
# <div class="carousel ce-element ce-element--type-carousel" data-ce-tag="carousel">
#   <div class="carousel-list">
#     <div class="carousel-track">
#     </div>
#   </div>
# </div>
#
# carousel library requires that all slides were added to it
# using it's `Carousel::add` method, which internally inserts slide's DOM element
# in innermost `carousel-track` div and binds needed event handlers

image = new ContentEdit.Image(imageAttrs)
carousel.attach(image)

# `carousel.attach` internally calls `image.mount`,
# which in turn appends `image._domElement` to `carousel._domElement`.
# So, at this point, the `carousel._domElement` looks like this
# <div class="carousel ce-element ce-element--type-carousel" data-ce-tag="carousel">
#   <div class="carousel-list">
#     <div class="carousel-track">
#     </div>
#   </div>
#   <div class="ce-element ce-element--type-image"></div>
# </div>
#
# but it should be something like this
# <div class="carousel ce-element ce-element--type-carousel" data-ce-tag="carousel">
#   <div class="carousel-list">
#     <div class="carousel-track">
#       <div class="ce-element ce-element--type-image"></div>
#     </div>
#   </div>
# </div>

So, we have three options to solve this problem:

1. Redefine mount for every element (that can be attached to carousel), so that it checks if parent type is 'Carousel' and try to call it's _carousel.add method. This is certainly the worst solution.
2. Redefine CarouselElement::attach, so that it unmounts child and uses @_carousel.add to reinsert child's DOM element. This is better, but causes extra DOM insertions/deletions.
3. Add new ElementCollection::attachDOMElement method and redefine Element::mount so that it uses parent().attachDOMElement. Something like this:

class ContentEdit.ElementCollection extends ContentEdit.Element
  attachDOMElement: (element) ->
    # note: `element` is CE element, not DOM element
    # appending element._domElement to @_domElement

  detachDOMElement: (element) ->
    # note: `element` is CE element, not DOM element
    # removing element._domElement from @_domElement

class ContentEdit.Element extends ContentEdit.Node
  mount: ->
    @_createDOMElement()
    @_addDOMClasses()
    @_addDOMAttributes()
    @_addDOMEventListeners()
    @parent() and @parent().attachDOMElement(@)
    ContentEdit.Root.get().trigger('mount', @)

  unmount: ->
    @parent() and @parent().detachDOMElement(@)
    @_removeDOMEventListeners()
    @_removeDOMAttributes()
    @_removeDOMClasses()
    @_destroyDOMElement()
    ContentEdit.Root.get().trigger('unmount', @)

class ContentEdit.CarouselElement extends ContentEdit.ElementCollection
  attachDOMElement: (element) ->
    position = @children.indexOf(element)
    @_carousel.add position, element.domElement()

  detachDOMElement: (element) ->
    position = @children.indexOf(element)
    @_carousel.remove position

Other questions

Getters/setters and public methods

parent._domElement.replaceChild() and parent.domElement().ReplaceChild()

I think this is a matter of interpretation.

Maybe this is. Technically you right. We only use getters and public methods. No direct access to private members. Using parent.domElement().replaceChild(), we can't replace _domElement to other value, but we can change it. This may be absolutely ok. But CE Element is a wrapper, abstraction over DOM element. And it better knows how to manipulate it's internal DOM element. Also CE element may be in kinda sync with internal DOM element and if you directly change this DOM element, it may break the sync. For example, I don't think that call element.domElement().focus() rather than element.focus() is a good idea. After all, I have the issue with carousel because child elements think that they know how to append to parent's _domElement.

createDOMElement and changing single element

The createDOMElement and mount/unmount only being against region

This approach would seem to make it very difficult to change out a single element

I can hardly remember what I wanted to say in that comment. That's funny 😄. As I think this was about starting of editor. Currently, editor.start creates regions, that manipulate DOM elements while they are still attached to the document. This can degrade performance. I suggested to create region's initial DOM tree outside of the document (which should increase performance) and then mount region._domElement (root of that tree) to the document. After editor is started, changing of single element should be the same as it ever was. But I don't think that performance issues are priority for now, so it better wait for a better days.

Merging Element with ElementCollection and Node with NodeCollection

Wrongly or rightly CE doesn't support use a virtual DOM in the sense that React does

Probably, I wrote incomprehensible. I didn't ask virtual DOM support. I just gave an example of React, that it doesn't divide elements and collections. I think, there should be only two base classes: Node and Element.

# I try to mention only methods, that deal with parent-children relationship and DOM manipulations
class ContentEdit.Node
  constructor: ...
  parent: ...
  parents: ...
  closest: ...
  next: ...
  nextContent: ...
  nextSibling: ...
  nextWithTest: ...
  previous: ...
  previousContent: ...
  previousSibling: ...
  previousWithTest: ...
  descendats: ...
  children: ...
  attach: ...
  detach: ...
  html: ...

class ContentEdit.Element extends ContentEdit.Node
  mount: ->
    @_createDOMElement()
    @_addDOMClasses()
    @_addDOMAttributes()
    @_addDOMEventListeners()
    @_mountChildren()
    @parent() and @parent().attachDOMElement(@)
    ContentEdit.Root.get().trigger('mount', @)

  unmount: ->
    @parent() and @parent().detachDOMElement(@)
    @_unmountChildren()
    @_removeDOMEventListeners()
    @_removeDOMAttributes()
    @_removeDOMClasses()
    @_destroyDOMElement()
    ContentEdit.Root.get().trigger('unmount', @)

Immutability

I understand this might just be an idea but as far as I'm aware there's no way to create a DOM element that's immutable in JS

I'm also unaware of immutable DOM elements 😄. The name createImmutableDOMElement is just first thing, that came into my head. It certainly inappropriate. The main idea was that elements aren't mountable or unmountable. There is just no such states as 'mounted' or 'unmounted'. CE elements always have not null _domElement (and in that sense they are always mounted). To let this happen, CE element's constructor should accept DOM element as parameter (much like Region does), store it in _domElement property and retrieve additional info.

Once CE element is initialized, it can change states between 'editable' and 'non-editable' (wrongly named 'immutable'). For example, state changes in TextElement will be insignificant (adding\removing ce-element classes and setting/unsetting contenteditable attribute). Contrary, ImageElement will replace _domElement with every state change (switch between img and div tags). When editor starts and stops, it only switches state of their regions.

Currently, CE elements generates DOM elements from their internal state (_tagName, _attributes, content etc). I think that, with 'editable'/'non-editable' approach this also should be inversed. CE elements should take all needed information directly from their internal _domElement.

# something like this
class ContentEdit.Element extends ContentEdit.Node
  constructor: (domElement) ->
    @_domElement = domElement
    @_attributes = @_getAttributes(domElement)
    @_classes = @_getClasses(domElement)

  isEditable: -> @_hasClass('ce-element')

  makeEditable: ->
    if @isEditable()
      return

    @_addDOMClasses('ce-element', 'ce-element--type-element')
    @_addDOMEventListeners(@_onEvent)
    ...

  makeNonEditable: ->
    unless @isEditable()
      return


    @_removeDOMClasses('ce-element', 'ce-element--type-element')
    @_removeDOMEventListeners(@_onEvent)
    ...

  @fromHTML: (html) ->
    wrapper = document.createElement('div')
    wrapper.innerHTML = html
    domElement = wrapper.firstChild
    new @(domElement)

pros:

1. This might simplify codebase, because we shouldn't care about "is element currently mounted or not".
2. Prevent DOM rebuilding when editor is started / restarted (increase performance).
3. Easy to implement conversion from html to CE element.
4. This preserve CE elements in 'non-editable' state (what you mention as The CE code base isn't typically present when non-editors view the content).

cons:

1. This might complicate codebase, because instead of 'mount'/'unmount' states we get another two states 'editable'/'non-editable'
2. This preserve CE elements in 'non-editable' state. You were worried that external code may break CE, when it in 'non-editable' state. But the fact, that CE is present when non-editors view the content, can not protect from direct changes in DOM elements, that CE relates on. This even can make CE elements more fragile, because they aren't rebuilt periodically and solely rely on internal DOM elements.
3. This might complicate (but I think not very much) storing/restoring from history

Summary

Again, It's just a thoughts. I think that it worth to implement "Merging Element with ElementCollection and Node with NodeCollection" paragraph and not so sure about the others.

P.S. I done my carousel element, so I don't have any issues right now. And I already done hacks for some of aforementioned points, so if you like my proposals, I will try to find time in near weekends and backport them to CE.

[anthonyjb]

Hi @revyh - no worries on the delay (as you might imagine I often have the same issue). Thanks again for your detailed response, I've had an initial read through but need to go through in detail and write up a worthy response.

I can definitely see some of the performance benefits of what your suggesting, some of which (for example createDOMElement and changing single element) I hope to implement in the next minor/bug release version. I think some however will clearly break backwards compatibility and so will have to be planned for a major release version - but I'm OK with that :)

Anyway - I will be reviewing this more over the coming weeks, and will come back to you with comments and questions, thanks again for your insights :)