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

Attempting to insert VNodes before or after children can result in duplicates #124

Open
taylorhadden opened this issue Dec 21, 2022 · 2 comments

Comments

@taylorhadden
Copy link
Contributor

taylorhadden commented Dec 21, 2022

Use Case

I have a use case where I want to insert an element after any "real" children of a piece of formatting.

           Outer format
       |-------------------|
Here is my example sentence
          ^=======^++++++++^
            Inner   Formats
       Insert element here ^

The Issue

The naive expectation for how to do this is to have the format definition append a child to the list of children that is provided:

formats = {
    outer: {
        render: (attributes, children) => {
            // Append a child to the provided children
            children.push(h('my-child))
            return h('span', null, children)
        } 
    }
}

However, the internal renderer renders the full stack of attributes for each Op section, then merges similar nodes together. Because of this, the example will result in three instances of my-child being added: one for each Op that makes up the larger "Outer format" section.

Potential Solutions

I did some real-life explorations on how to solve this problem, as well as some theoretical ones.

Pre-Merging

My first thought was to collect children together before rendering a parent by using a stack of not-yet rendered parents and appending rendered children to them. See this commit for a working implementation.

An issue here is that this implementation does not take into account attributes without a format assigned to them and how they can affect the rendering of a format. This is definitely a corner case, but it's something I use extensively in Tangent.

A benefit to this approach is that the mergeChildren() step is removed entirely; elements are merged as the renderer processes the operations.

Post Processing

An alternative, and much simpler idea I had was to allow inline formats to provide a postProcess(node: VNode) function that gets invoked after nodes are merged together. See this commit for a working implementation.

This totally feels like a hack, but it functions and has helped resolve some bugs. In the interim, I will be using this locally.

Allow formats to define outer node builders and inner node builders separately

Separate definitions of the outer VNode and inner children would allow for VNodes to be compared for merging while taking into account all of the effects that non-format-type-attributes might have on that rendered node.

For example:

myType: {
  renderNode(attributes) {
    return h('span', props)
  },
  renderChildren(attributes, children) {
    // Do whatever you want to the child list
    children.push(h('my-element'))
    return children
  }
}

I would use the stack concept from the "Pre-Merge" example, but instead of comparing just attributes[typeName], I would compare the generated VNode type and properties. Once children are grouped appropriately, renderChildren() would be called, allowing any customization to work its magic.

Obviously, this kind of format definition would need to be optional. It's also a little awkward.

Make the children argument of render() special

To the best of my knowledge, right now the children argument of FormatType.render() will always have just a single child inside of it.

This idea builds on the Pre-Merge approach. Instead of passing the real child to render(), an array with a placeholder child is passed instead. The resulting VNode could then be pre-merged in a stack. Once it is time to collapse the stack and combine the children, the placeholder child would be replaced by the real children. This way, any additional children prepended or appended in a format type's render() function would only be added (and appear) once in a continuous span of formatting.

One complication would be if a type's render() deeply nests the original children. In such a case, the VNode tree would need to be searched to find it. However, I expect this to not actually be a common case. I expect that most (all?) inline formats are wrapping children in a single VNode.

This solution obviously does not work at all if we want to support types doing different things depending on what kind of children they have or changing those children. This strikes me personally as not something that one should be doing, but I'm doing several things Typewriter doesn't expect one to be doing in my usage of it.

Next?

Hopefully that was all intelligible. I'm curious what others (and especially @jacwright) think of this. Currently, I would lean towards the last example.

@jacwright
Copy link
Member

The order of formats defined matter. The earlier it is in the array of defined formats in Typesetting, the more "outer" it will be. In the default typesetting, you will see that link is first. This is because splitting up bold and italics as you described isn't a big deal, but when you split up a link, it makes makes the mouse only underline certain parts of the link, even though clicking on it will get you to the same location either way.

It sounds like you aren't looking for an additional formatting option though. It sounds like you are just looking at inserting something outside of the formats. If you could share what you are trying to do, that may help.

Ultimately, we need to try and come up with a way to allow what you are trying to do (if we know the why, that may help) without making it complex for those who don't have very custom needs.

@taylorhadden
Copy link
Contributor Author

Aye, I'm quite familiar with the element stacking rules.

The use case is that I'm parsing a markdown image link and then placing the content of the link directly after the text. The intended HTML looks something like this:

<span class="link-text">![](https://something.com/image.png)</span><img src="https://something.com/image.png"/>

In this case, I do not want the image to be an embed; it is not part of the document, just an embellishment of the document.

The issue is that the element merging is expecting each format to produce only a single element, and for that element to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants