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

<d3-group auto-resize> breaks when used with transitions #1665

Open
EndilWayfare opened this issue Jan 20, 2021 · 9 comments
Open

<d3-group auto-resize> breaks when used with transitions #1665

EndilWayfare opened this issue Jan 20, 2021 · 9 comments

Comments

@EndilWayfare
Copy link

When rendering a Cartesian chart with a transition,

//...
const area = fc.seriesSvgMulti().series(/* ... */);
const chart = fc.chartCartesian(xScale, yScale).svgPlotArea(area);
d3.select("#chart").data(data).transition().call(chart);

resizing the window does not update the chart. Many Uncaught Error: transition 1 not found errors are logged to console.

Seems related to d3/d3-transition#59, since redraw retains a reference to the selection originally used to render the chart.

Attempting to redraw the chart without the transition once completed, e.g.

//...
d3.select("#chart")
  .data(data)
  .transition()
  .on("end", () => {
    d3.select("#chart").data(data).call(chart)
  })
  .call(chart);

causes the axes to resize properly, but the body of the chart is still broken

Recreating all components inside the "end" listener does fix the chart, but seems inefficient (especially for complex charts)

Luckily, it seems sufficient to recreate only the component directly passed to the chart. I.e. if area is a multi-series, the series passed to area.series() do not need to be recreated. The problem is not unique to multi-series; if area is a "regular" series, it needs to be recreated as well.

//...
const a = fc.seriesSvgPoint();
const b = fc.seriesSvgLine();

const createArea = () => fc.seriesSvgMulti().series([a, b]);

const chart = fc.chartCartesian(xScale, yScale).svgPlotArea(createArea());

d3.select("#chart")
  .data(data)
  .transition()
  .on("end", () => {
    chart.svgPlotArea(createArea());

    d3.select("#chart").data(data).call(chart)
  })
  .call(chart);

This works, but seems very hacky. Then again, I'm very new to the d3 ecosystem, and it seems like solutions like this aren't uncommon. If there's a blessed solution to this, it would be nice to add to the documentation.

@EndilWayfare
Copy link
Author

Maybe propagateTransition should catch errors when trying to inherit?

const propagateTransition = maybeTransition => selection =>
isTransition(maybeTransition) ? selection.transition(maybeTransition) : selection;

const propagateTransition = maybeTransition => selection => {
  let selection = selection;
  
  if (isTransition(maybeTransition)) {
    try {
      selection = selection.transition(maybeTransition)
    }
  }

  return selection
}

There's probably a more functional way to write that...
I'm not sure what style of FP exception handling in vanilla JS this project would prefer.

This seems like the least surprising way to handle this case. Maybe there's a way to capture the transition before it fires and use it as a template?

@EndilWayfare
Copy link
Author

Extends to other places where transition inheritance happens, like in dataJoin

const transition = implicitTransition || explicitTransition;
if (transition) {
update = update.transition(transition).style('opacity', 1);
enter.style('opacity', effectivelyZero);
exit = exit
.transition(transition)
.style('opacity', effectivelyZero);
}

@ColinEberhardt
Copy link
Member

Thanks for raising this - it is a big can of worms that very much relates to the complexities of the D3 transition lifecycle:

https://github.com/d3/d3-transition#the-life-of-a-transition

With d3fc, the various components (chart, series, ...) create new elements using data joins. The goal is to ensure that we propagate any transitions from the select used to render the components over to the elements it creates itself.

This will need further investigation!

@EndilWayfare
Copy link
Author

Yes, I've been digging through the source (both d3fc and d3 itself) more since I posted, as well as fiddling with local copies of various components to investigate possible fixes/workarounds.

Perhaps with some guidance I can hack on the library itself and write tests at some point. My current focus is getting my project to function correctly, at least from an outside user perspective, but I'd love to figure out how to contribute.

@chrisprice
Copy link
Contributor

Disclaimer: I've not invested too much time in understanding how transitions should work, I've just having a quick poke around the code.

I think the "proper" fix would be to observe the lifecycle events of the transition and only propagate it if it is still active.

The error we are seeing is ultimately caused because the transition has been removed from the root note once it has ended. This can happen here or here. In each code path I've followed, this removal is immediately preceded by firing the event interrupt, cancel or end.

If we're adding this functionality in then I think it should be encapsulated in one place if we can (like isTransition).

@chrisprice
Copy link
Contributor

Thinking a bit more about this, there are 3 common things we do which would break transition propagation without intervention -

  • selection.selection().selectAll(/*...*/).data(/*...*/).append(/*...*/) - It is only possible to perform a data join on a selection, therefore must explicitly unwrap a transition first. However, dataJoin wraps up our handling of this behaviour.
  • selection.each((d, i, nodes) => select(nodes[i])/*...*/) - Unwrapping selections using each loses the transition. We perform this in almost every component that accepts a selection to ensure we apply the component logic to each node and its associated data. In the majority of cases, if (dataJoin.isTransition(x)) { dataJoin.transition(x) } can be used to propagate the transition. However, in some components which use trees of SVG nodes to represent each item and have calculations which are shared for the entire tree e.g. seriesSvgOhlc an additional selection.each on the update selection breaks this.
  • select('d3fc-*').on('draw', (d, i, nodes) => select(nodes[i])/*...*/)- Again unwrapping selections using on loses the transition. This is localised to cartesian chart at the moment but would affect anything using d3fc-element.

@chrisprice
Copy link
Contributor

So after all that pontificating, I think we need a new utility component/helper that -

  • implements the propagate transition pattern in one place (pulling the implementations out of dataJoin/chart/seriesSvg*/etc.)
  • gates its behaviour depending upon the transition state (observed by listening for the state change events above)

@EndilWayfare
Copy link
Author

Hmm, so registering an event handler inside the outer propagateTransition closure, perhaps?

Where would such a component live? It seems like d3fc-data-join package is the natural place, because that's generally the engine of propagation.

@chrisprice
Copy link
Contributor

Hmm, so registering an event handler inside the outer propagateTransition closure, perhaps?

Yea that's what I was thinking. Assuming there's no direct way of tracking the state of the transition (which I couldn't easily find).

Where would such a component live? It seems like d3fc-data-join package is the natural place, because that's generally the engine of propagation.

Agreed. I'm keen it doesn't become a utils package but this problem is tightly coupled to DOM manipulation for which we almost always used3fc-data-join so I think it fits well.

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

No branches or pull requests

3 participants