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

Fix morph from text node to comment node #122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bennlich
Copy link

@bennlich bennlich commented Jun 1, 2021

I'm not entirely sure what all the ramifications of this change are, but they fix a bug I was running into with HTML comments.

The bug was that occasionally my comment text would wind up getting rendered as DOM text after a morph. Sometimes during a walk morph would try to morph a text node into a comment node, which was no good.

I think my change prevents morph from ever trying to go from text node to comment node. tagName is undefined for both text and comment nodes, but nodeName is meaningful. And I think nodeName has the same value as tagName in most other cases, but I'm not too sure about this. See: https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeName.

Here's some code that repro's the bug. When you click, you'll see the number 1 show up in the DOM, even though it's supposed to be in a comment.

test.html

<div id="test"></div>

<script type="module">

  import morph from './nanomorph.es6.js'
  import html from './nanohtml.es6.js'

  let parent = document.querySelector('#test')

  let fragment = () => html`
    <!-- 1 -->
    <!-- 2 -->
  `

  morph(parent, html`
    <div id="test">
      ${ fragment() }
    </div>
  `)

  document.addEventListener('click', () => {
    morph(parent, html`
      <div id="test">
        <div>Hello</div>
        ${ fragment() }
      </div>
    `)
  })

</script>

@@ -59,7 +59,7 @@ function walk (newNode, oldNode) {
return null
} else if (newNode.isSameNode && newNode.isSameNode(oldNode)) {
return oldNode
} else if (newNode.tagName !== oldNode.tagName || getComponentId(newNode) !== getComponentId(oldNode)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be nice having a related unit test

Copy link
Author

@bennlich bennlich Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble reproducing this in the test environment. Could it have something to do with the browser DOM?

In the browser, this successfully fails:

<div id="test">
  <!-- 1 -->
  <!-- 2 -->
</div>

<script type="module">

  import morph from './nanomorph.es6.js'
  import html from './nanohtml.es6.js'

  let parent = document.querySelector('#test')

  document.addEventListener('click', () => {
    morph(parent, html`
      <div id="test">
        <div>Hello</div>
        <!-- 1 -->
        <!-- 2 -->
      </div>
    `)
  })

</script>

but this test does not fail:

t.test('comments', function (t) {
    var a = html`<div>
  <!-- 1 -->
  <!-- 2 -->
</div>`
    var b = html`<div>
  <div>Hello</div>
  <!-- 1 -->
  <!-- 2 -->
</div>`
    var expected = b.outerHTML
    var c = nanomorph(a, b)
    t.equal(c.outerHTML, expected)
    t.end()
})

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

Successfully merging this pull request may close these issues.

None yet

2 participants