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

textNode removed unexpectedly due to Chrome 55 bug #6601

Closed
Peter-WF opened this issue Sep 14, 2017 · 5 comments
Closed

textNode removed unexpectedly due to Chrome 55 bug #6601

Peter-WF opened this issue Sep 14, 2017 · 5 comments

Comments

@Peter-WF
Copy link
Contributor

Peter-WF commented Sep 14, 2017

Version

2.4.3

Reproduction link

https://jsfiddle.net/jLrvghtL/8/

Steps to reproduce

<div id="app">
  <div v-if="status" v-html="status"></div>
  <div v-else class="status">else</div>
  <button @click="toggleEdit">
    toggleEdit
  </button>
</div>
new Vue({
  el: '#app',
  data: {
    status: false
  },
  methods: {
    toggleEdit: function() {
      this.status = !this.status
    }
  }
})

What is expected?

The v-html="status" could be display correctly

What is actually happening?

The status won't be display, if the status equal true and the userAgent were Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36, unless we replace span with other tag.

@Peter-WF
Copy link
Contributor Author

Perhaps the reason of the question is a chrome bug.

document.body.innerHTML = '<a id="test">old value</a>'

var a = document.getElementById('test')
//  cache old TextNode
var cacheTextNode = a.childNodes[0]
// update innerHTML
a.innerHTML = "new value"

// output old TextNode.parentNode
console.log(cacheTextNode.parentNode)

// In chrome(60)
=> null

// In chrome(55) & chrome(47)
=> <a id="test">new value</a>

@defcc defcc added the bug label Sep 14, 2017
@Peter-WF
Copy link
Contributor Author

In old chrome version(55), the ch.elm point to the new TextNode and the parentNode is not null
image

In last chrome version(60), the ch.elm point to the old TextNode and the parentNode is null
image

@defcc
Copy link
Member

defcc commented Sep 14, 2017

The el textNode should not be in the dom tree here.

vue/src/core/vdom/patch.js

Lines 97 to 103 in 7cea42b

function removeNode (el) {
const parent = nodeOps.parentNode(el)
// element may have already been removed due to v-html / v-text
if (isDef(parent)) {
nodeOps.removeChild(parent, el)
}
}

And it seems that only text node will get the buggy behavior.

Maybe we could attach an extra info to the parent node when patching the dom props in updateDomProps to workaround the chrome bug, e.g. check the oldVnode's children and mark the parent element when it only contains a text node.

@yyx990803 yyx990803 added improvement and removed bug labels Sep 14, 2017
@yyx990803 yyx990803 changed the title Content in v-html can`t render correctly textNode removed unexpectedly due to Chrome 55 bug Sep 14, 2017
@yyx990803
Copy link
Member

yyx990803 commented Oct 2, 2017

I honestly don't think we should cater to such obvious buggy behavior in an outdated version of a browser (it's 6 versions behind). The fix also lets platform-specific code leak into the core patching algorithm.

If for some reason your app must be run in an older version of Chrome, you can give the two divs different keys to work around the behavior.

Update: it seems old Chrome has a higher market share than I thought...

@yyx990803 yyx990803 reopened this Oct 2, 2017
ztlevi pushed a commit to ztlevi/vue that referenced this issue Feb 14, 2018
f2009 pushed a commit to f2009/vue that referenced this issue Jan 25, 2019
@jodator
Copy link

jodator commented Sep 13, 2019

This fix doesn't take node type (shouldn't it be 'text' only?) into account and causes other applications to fail: ckeditor/ckeditor5#2016 (comment). The problem was that all inner DOM nodes of the Vue component are expected to be managed by the editor.

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

Successfully merging a pull request may close this issue.

4 participants