d3 / d3-transition

Animated transitions for D3 selections.
https://d3js.org/d3-transition
ISC License
224 stars 64 forks source link

Transitions should use computed value for end, too? #47

Closed wlodzislav closed 7 years ago

wlodzislav commented 8 years ago

When transition 'left' or 'right' style from negative % to positive % sets wrong values. When do the same for 'top' or 'bottom' it sets she same value all the animation frames.

Code to reproduce:

<div style="position: absolute"></div>
var div = d3.select("div");
div
    .style("left", "-50%")
    .transition().duration(1000)
    .style("left", "50%")
    .tween("log", function () {
        return function (t) {
            console.log(div.style("left")); // will be something like -5470.53px to 523px
            console.log(div.node().style.left); // will be something like -522.996% to 50%
        };
    });

I used https://d3js.org/d3.v4.js for testing.

curran commented 8 years ago

Reproduction: http://bl.ocks.org/curran/cbd4695616e8128049e2614464c4140c

wlodzislav commented 8 years ago

Bug reproduces in Chrome 51 and Firefox 47 but not in Safary.

mbostock commented 8 years ago

This is the expected behavior because transition.style uses the computed value as the starting value (hence converting percentage to pixels), then followed by d3.interpolateString.

You can avoid this by using transition.styleTween and specifying the start value explicitly by returning an interpolator that both starts and ends with percentages.

I suppose an alternative solution would be for D3 to set the target value temporarily, compute the resulting computed value, and then use that for the interpolator. But that is a fairly significant change and also means that the resulting assigned style property value will be different than what was passed to transition.style when the transition ends. So I am inclined to close this as designed.

wlodzislav commented 8 years ago

I'm not sure, note that in my case div.style("left") is -4597.61px, but window.getComputedStyle(document.querySelector("div")).left when left is set to -50% is -480px

mbostock commented 8 years ago

Yes. That is because div.style("left") is again returning the computed value.

mbostock commented 8 years ago

We already set the target value temporarily in the case of transition.style(name, null) so that we know how to transition when removing a style. I suppose it wouldn’t be too much of a stretch to temporarily set the style in all cases, so that transition.style uses the computed value for both the start and end. (Related bug #49.)

I’m a little worried that this will be detrimental to performance, especially since a naïve implementation would interleave reading and writing to the DOM and waste effort. But perhaps there’s a simple four-pass approach where, on all starting elements, we first read the starting computing values, then temporarily set the style to the target value, then read the ending computed values, and then finally restore to the starting value.

wlodzislav commented 8 years ago

I don't think I get it, why not just interpolate value from node.style.left if it's presented to the end value without computing and converting to px? I think it's common situation when inline style is already set or set just before the transition.

mbostock commented 8 years ago

Because often the style is not inlined: it is inherited or cascaded.

mbostock commented 7 years ago

I think it would be reasonable for style transitions to use the inline value if present as the starting value, and only fallback to the computed value if the inline value is not present. That way it’s a lot easier to control the starting value of a transition, and by extension the interpolator. Also, it’s more consistent with how attribute transitions work. And it’s faster!