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.

Sign up for GitHub

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

Jump to bottom

Patched elements keep inline styles from old vnodes #4227

Closed
ghost opened this issue Nov 17, 2016 · 11 comments · Fixed by #4235
Closed

Patched elements keep inline styles from old vnodes #4227

ghost opened this issue Nov 17, 2016 · 11 comments · Fixed by #4235
Labels
bug discussion

Comments

@ghost
Copy link

ghost commented Nov 17, 2016 •
edited by ghost

Vue.js version

2.0.6+

Reproduction Link

https://jsfiddle.net/gw8g8cos/

vue-glitch

Steps to reproduce

$ vue init webpack-simple .
$ npm install vue@2.0.7 vue-template-compiler@2.0.7

<template>
<div>
    <section style="text-align: center" v-if="loading">
        Should be centered.
    </section>
    <section style="margin-top: 6rem;" v-if="!loading">
        Should not be centered.
    </section>
</div>
</template>

<script>
export default {
    name: 'app',
    data () {
        return {
            loading: true
        }
    },
    mounted: function() {
        setTimeout(() => {
            this.loading = false;
        }, 2000);
    }
}
</script>

What is Expected?

Patched elements should not keep inline styles from old vnodes.

What is actually happening?

During the update operation, patched elements will retain styles from vnodes that will no longer exist in the DOM.

Temporarily solution for end-users: changing inline styles to classes, choosing different tag names or adding a key directive will fix the issue. Thanks to @LinusBorg for the latter.

It looks like this PR has introduced this regression #4138. I've opened a pull request with a failing test that confirms this issue.
@ghost ghost changed the title Random styles applied to elements as of 2.0.6 Random inline styles applied to incorrect elements as of 2.0.6 Nov 17, 2016
@ghost ghost mentioned this issue Nov 17, 2016
Merged
@ghost ghost changed the title Random inline styles applied to incorrect elements as of 2.0.6 Destroyed elements pass their inline styles onto other similar existing elements as of 2.0.6 Nov 17, 2016
@ghost ghost changed the title Destroyed elements pass their inline styles onto other similar existing elements as of 2.0.6 Destroyed elements pass their inline styles onto others Nov 17, 2016
@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Nov 17, 2016 •
edited

This might be related to #4205. But I'm not sure. Can you try adding a key property to section?

Online repro:
https://jsfiddle.net/gw8g8cos/

@ghost
Copy link
Author

ghost commented Nov 17, 2016

@HerringtonDarkholme it's definitely related. Still a little annoying to have to deal with this... Adding the key tag worked... Would be nice to mention something in the docs about diffing.

@ghost ghost closed this as completed Nov 17, 2016
@LinusBorg
Copy link
Member

@Igdexter

Would be nice to mention something in the docs about diffing.

We're on it vuejs/v2.vuejs.org#587

@ghost
Copy link
Author

ghost commented Nov 17, 2016 •
edited by ghost

@LinusBorg cool, thanks for the quick response. had quite a shock in my staging environment today with 2.0.7

@LinusBorg
Copy link
Member

LinusBorg commented Nov 17, 2016 •
edited

had quite a shock in my staging environment today with 2.0.7

This is not an intended breaking change, mind you. It's generally expected behaviour in many situations, but as already mentioned, documentation about this is still lacking.

I assume that this worked "accidentally" before, and recent fixes of bugs which, amongst other things, related to handling of style and class bindings, might have changed that.

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Nov 17, 2016 •
edited

@LinusBorg I wonder whether this is a correct behavior.

The example above can be diffed solely by v-dom. The style attribute should be considered as a v-dom construct.

By comparison, consider this example. https://jsfiddle.net/gw8g8cos/4/
The inner text and class has been updated, the class is not merged, but style is merged.

Note, this issue is different from #4205 , since lifecycle is not managed by virtual dom.
And it is also different from #4216, since inline-template, speaking in implementation, has a different slot than render in vnode representation.

cc @defcc

@ghost
Copy link
Author

ghost commented Nov 17, 2016 •
edited by ghost

@LinusBorg I'm guessing the improvement to preserve static inline styles with dynamic inline styles as per #4138 is what made this issue apparent now. I'm just wondering if this is the correct behavior. Here it's comparing the old node with the new node and then applying the necessary style changes, but the code seems to be merging them. Reusing the same node is fine; I just think it should keep only the styles for the active node that needs to be represented on the DOM.

Edited the OP in light of the new info

@LinusBorg
Copy link
Member

Hm you might be right.

@ghost ghost changed the title Destroyed elements pass their inline styles onto others Patched elements keep inline styles from old vnodes Nov 17, 2016
@defcc
Copy link
Member

defcc commented Nov 17, 2016 •
edited

Seems to be something wrong with static style and style binding merge. I am looking into it.

@defcc defcc added the bug label Nov 17, 2016
@defcc
Copy link
Member

defcc commented Nov 17, 2016

https://github.com/vuejs/vue/blob/dev/src/platforms/web/runtime/modules/style.js#L48

const style = normalizeStyleBinding(vnode.data.style) || {}
vnode.data.style = style.__ob__ ? extend({}, style) : style
const newStyle = getStyle(vnode, true)

The style now only holds the style binding, which will be removed in the next render. The static style and style binding should be removed both when reusing the same node.

@defcc
Copy link
Member

defcc commented Nov 17, 2016

I'll make a PR to fix it

@defcc defcc mentioned this issue Nov 17, 2016
Merged
yyx990803 pushed a commit that referenced this issue Nov 17, 2016
@defcc @yyx990803
Remove old static style when applying style update (fix #4227) (#4235)
8f7c49c 
* both static style and stylebinding should be removed

* update test case

* update test case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove old static style when applying style update (fix #4227)
3 participants
@LinusBorg @defcc @HerringtonDarkholme