Bounding box error when initializing a graph

Kyrio commented 5 years ago

With Cytoscape v3.4.2 and cytoscape-dagre v2.2.2, I'm getting the following error when initializing a fairly large, compound graph:

ERROR TypeError: Cannot read property 'x1' of undefined
    at updateBoundsFromBox (cytoscape.cjs.js:8653)
    at cachedBoundingBoxImpl (cytoscape.cjs.js:9172)
    at Collection.push../node_modules/cytoscape/dist/cytoscape.cjs.js.elesfn$j.boundingBox (cytoscape.cjs.js:9233)
    at update (cytoscape.cjs.js:8507)
    at Element.push../node_modules/cytoscape/dist/cytoscape.cjs.js.elesfn$j.updateCompoundBounds (cytoscape.cjs.js:8614)
    at Element.dimImpl [as width] (cytoscape.cjs.js:9342)
    at Element.outerDimImpl [as outerWidth] (cytoscape.cjs.js:9370)
    at Element.layoutDimensions (cytoscape.cjs.js:10602)
    at Layout.DagreLayout.run (cytoscape-dagre.js:158)
    at setElesAndLayout (cytoscape.cjs.js:17611)

The graph is not rendered and the "ready" event is never emitted. This happens consistently in Firefox 65 but only rarely in Chrome 72.

Unfortunately I cannot attach the graph for reproduction as the data is under an NDA, but if you need specific information I'll try my best to provide it. When reducing the number of elements (from 102 nodes and 135 edges), the error doesn't seem to occur.

maxkfranz commented 5 years ago

Unfortunately, there's nothing that we can do until we have a reproducible test case including data. You could consider anonymising your existing data by removing and renaming the appropriate fields. The output topology and style should be all that matters.

Kyrio commented 5 years ago

I've put together a StackBlitz to reproduce the issue and eventually managed to find what exactly triggers it, so anonymizing will not be necessary.

So first, while the stack trace made me assume that it was Dagre-related, the issue actually appears with other layouts (e.g. breadth-first). Then, it looks like it is triggered by a node which has a parent node but also the style display: 'none'.

// In the stylesheet:
{
  selector: 'node.hidden',
  style: {
    display: 'none',
  }
}

// In the node list:
{
  "data": {
    "id": "1",
    "name": "Compound node",
  }
},
{
  "data": {
    "id": "2",
    "name": "Hidden node",
    "parent": "1",
  },
  "classes": "hidden"
}

Here's the StackBlitz for quick reproduction (remove the "hidden" class from the last node in the list to have a working graph): https://stackblitz.com/edit/cytoscape-dagre-42?file=src/app/graph-data.ts

maxkfranz commented 5 years ago

It doesn't make sense to include display:none elements in a layout: They take up no space.

I could remove the exception, but you'll still get no result for display:none elements.

Kyrio commented 5 years ago

In my application, 'hidden' is a class added or removed to some nodes at runtime, through a button. But in some cases, I need to recreate the graph and have them still hidden, so that I can show them again later with the same button (by removing the class).

This is why I sometimes have display: none nodes in the initial dataset. They're meant to be displayed later, and it is convenient not having to add them at runtime - it doesn't trigger any errors if they don't have a parent node. I understand this is an edge case, but it might not be an error case - your call.

maxkfranz commented 5 years ago

I thought this was just an issue of specifying display:none element in a layout, but it turns out it's a more general issue. See https://github.com/cytoscape/cytoscape.js/issues/2339

cytoscape / cytoscape.js-dagre

Bounding box error when initializing a graph #42