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

draw throws exceptions for line charts #265

Open
stephen-dunn opened this issue Feb 20, 2017 · 6 comments
Open

draw throws exceptions for line charts #265

stephen-dunn opened this issue Feb 20, 2017 · 6 comments

Comments

@stephen-dunn
Copy link

In the latest version dimple 2.3.0 and d3 4.3.0 draw() throws an exception when called after receiving a data update.
here is an example

If I switch back to dimple 2.1.6 and d3 3.4.8 then it works as expected and does not throw an exception.

Here is the exception from the example above:

Uncaught DOMException: Failed to execute 'querySelectorAll' on 'Element': '.dimple-marker.dimple-series-0.' is not a valid selector.
@tscizzle
Copy link

tscizzle commented Mar 8, 2017

I am getting this precise error as well.

Note that there doesn't even need to be a data update. Re-calling chart.draw at all yields this error.

To confirm, I did:

chart.draw();
console.log('drew once');
chart.draw();
console.log('drew twice');

and didn't get to the last print out, instead erroring as @stephen-dunn did.

@peterj-freestar
Copy link

peterj-freestar commented May 12, 2017

I have been experiencing this error as well, and it prevents interactive filtering when rendering a dual-axis chart. When the user selects a filter value, from e.g. an element created with d3.select("#somedivid").append("div").append("select"), the first series gets redrawn but the DOMException interrupts the draw method and so the second series remains unchanged.

Confirming, though, that switching to d3.v3 and dimple.v2.1.6 solves the issue.

@camille-s
Copy link

I'm having this same problem with line charts as well, same error as given above. The best I can pinpoint, based on the error message '.dimple-marker.dimple-series-0.' is not a valid selector and checking in chrome dev tools, is that this comes from the method _drawMarkers. The section

if (series._markers === null || series._markers === undefined || series._markers[lineDataRow.keyString] === undefined) {
            markers = series._group.selectAll("." + markerClasses.join(".")).data(lineDataRow.markerData);
        } else {
            markers = series._markers[lineDataRow.keyString].data(lineDataRow.markerData, function (d) {
                return d.key;
            });
        }

looks like where names of classes are being joined by a dot. One of these dots is put at the end of the selector string--maybe some class in the array being joined is just blank text? Not sure the best way to fix it, but that seems to be where the issue lies.

@guylando
Copy link

guylando commented Aug 5, 2017

tldr: THE FIX IS VERY SIMPLE:
Change line 4411 from:
series._markers[lineDataRow.keyString] = markers;
To:
series._markers[lineDataRow.keyString] = markers.merge(shapes);
Change line 4311 from:
series._markerBacks[lineDataRow.keyString] = markerBacks;
To:
series._markerBacks[lineDataRow.keyString] = markerBacks.merge(shapes);
Change everywhere(Appears twice):
series.shapes = series._group.selectAll("." + className);
To:
series.shapes = theseShapes.merge(entered);

It seems the problem is indeed in _drawMarkers.
It is called twice once for "add", second time for "update" in the draw line function and in previous version the lines:
// Deal with markers in the same way as main series to fix #28
if (series._markers === null || series._markers === undefined || series._markers[lineDataRow.keyString] === undefined) {
markers = series._group.selectAll("." + markerClasses.join(".")).data(lineDataRow.markerData);
} else {
markers = series._markers[lineDataRow.keyString].data(lineDataRow.markerData, function (d) {
return d.key;
});
}
// Add
if (lineShape.nextSibling && lineShape.nextSibling.id) {
shapes = markers.enter().insert("circle", '#' + lineShape.nextSibling.id);
} else {
shapes = markers.enter().append("circle");
}
For update returned array of nulls which did not create elements. While in new version it DOES create new elements which don't have keyString and cause the exception later on because they keyString is undefined and is used to generate the class name.

The key differance with the new d3 version is: "The selection.sort and selection.data methods now return new selections rather than modifying the selection in-place." https://github.com/d3/d3/blob/master/CHANGES.md#selections-d3-selection
And thus setting the data in the first conditions create new selection and thus the code afterwards thinks its new elements and d3 doesn't understand that its the old elements and it performs enter once again.

Calling markers.enter().append("circle") in new d3 will create new elements over and over again while in previous version it will not create anything (can put breakpoint in chrome devtools after the mentioned code block and test see this).

What we basically want to solve this is to save in the markers variable the markers in a way that calling d3 enter on existing markers will not cause them to re-enter.

SO FINALLY THE FIX IS VERY SIMPLE:
Change line 4411 from:
series._markers[lineDataRow.keyString] = markers;
To:
series._markers[lineDataRow.keyString] = markers.merge(shapes);

NOTE: SAME BUG IN _drawMarkerBacks:
Change line 4311 from:
series._markerBacks[lineDataRow.keyString] = markerBacks;
To:
series._markerBacks[lineDataRow.keyString] = markerBacks.merge(shapes);

NOTE: SEEMS THERE WAS A WRONG ATTEMPT TO HANDLE THIS BUG IN THE LINE SHAPES WHICH IN PREVIOUS VERSION WAS:
series.shapes = theseShapes;
And in current version it changed to:
series.shapes = series._group.selectAll("." + className);
WHICH IS WRONG AND CAUSES BUG BECAUSE LOSES INFORMATION SUCH AS KEYSTRING RESULTING IN THE CONSOLE ERROR ABOUT SELECT CLASS
Need fix it to:
series.shapes = theseShapes.merge(entered);
(Appears twice)

huubbouma added a commit to PythonUnited/dimple that referenced this issue Aug 23, 2017
@huubbouma
Copy link

@guylando I tried your fix and I can confirm that it works. Thanks!

jmalonzo added a commit to taguchimail/dimple that referenced this issue Dec 30, 2018
@aidandunsdon
Copy link

Thank you a few years later!

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

No branches or pull requests

7 participants