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

brush resets to covering entire domain when zooming is enabled, after upgrade to D3 v6 #1772

Open
ewaldman opened this issue Oct 16, 2020 · 20 comments

Comments

@ewaldman
Copy link

Hi.
I am using DC version 4.2.0. I upgraded my D3 to 6.2.0 and now the brush on my LineChart does not work.
When dragging the brush it does indeed filter out the other charts, however when releasing the brush it snaps back and nothing is filtered out. Also, after releasing the brush, the chart continuously redraws itself. When I moved back to D3 version 5.16.0 it works like a charm.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 16, 2020

Hi @ewaldman, thanks for the report!

Brushing works okay in the main page as well as this example.

Are you using any custom event handlers in your application? D3 changed the signature for event handlers so that the event is passed as the first argument instead of using a global, so pretty much all event handlers need to be changed.

Otherwise, if you can provide a reproducible example, I'd be glad to look into it.

@ewaldman
Copy link
Author

Thank you for responding.
Once I added the 'event' parameter to events the brushes do indeed work.

However, I have another question.

The event I am using is renderlet.
The old code was:
this.timeLineChart.on('renderlet', ( chart) => {
chart.select('svg').attr('width', null).attr('height', null).attr('viewBox',
'0 0 ' + TIME_LINE_WIDTH + ' ' + 300).attr('preserveAspectRatio', 'xMinYMin');
});

Based on your answer above I added event , and this is the new function:

this.timeLineChart.on('renderlet', (event, chart) => {
chart.select('svg').attr('width', null).attr('height', null).attr('viewBox',
'0 0 ' + TIME_LINE_WIDTH + ' ' + 300).attr('preserveAspectRatio', 'xMinYMin');
});

However, google chrome is giving errors.

Uncaught TypeError: Cannot read property 'select' of undefined
at LineChart. (myService.js:1887)
at Dispatch.call (dispatch.js:57)
at SVGSVGElement. (dc.js:1979)
at Dispatch.call (dispatch.js:57)
at tick (schedule.js:141)
at timerFlush (timer.js:61)
at wake (timer.js:71)

Any tips ?

Thank you.

@ewaldman
Copy link
Author

On further investigation , it seems that my first way of the function was right.

this.timeLineChart.on('renderlet', ( chart) => {
chart.select('svg').attr('width', null).attr('height', null).attr('viewBox',
'0 0 ' + TIME_LINE_WIDTH + ' ' + 300).attr('preserveAspectRatio', 'xMinYMin');
});

'renderlet' is the event name, and the listener function is the second param. However, doing it that way, the brushes do not work in the time line. Could it be that using the latest D3 the brushes only work in bar chart and not in line chart? The examples you pointed me to (above) are both bar charts.

Please advise.

Thank you.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 19, 2020

Thanks for following up, @ewaldman.

It is only DOM events that have the signature changed, like brush events, mouse events, keyboard, etc.

dc.js chart events are unaffected and still take the arguments documented in the BaseMixin.on examples.

The brush is working in the range series example. I also tried changing the fluctuation chart on the main page to a line chart and that worked. (I just needed to disable some of the bar-specific options.)

image

I guess I would need a running example (like a jsfiddle, codepen, observable, or bl.ock - whatever you are comfortable with) in order to see what is going wrong with your code.

@ewaldman
Copy link
Author

Hi.
We seem to have narrowed down the problem.
Here is the rendlet code - see the bold part

this.timeLineChart.on('renderlet', (chart) => {
chart.select('svg').attr('width', null).attr('height', null).attr('viewBox',
'0 0 ' + TIME_LINE_WIDTH + ' ' + 300).attr('preserveAspectRatio', 'xMinYMin');
});

When that code is there , the brushes don't filter properly. When I take out that code - and leave the rest - the brushes do work.
We use the rendlet so that the svg resizes when we resize the page.

Any tips? Do we even need that preserveAspectRatio code for our purpose?

Thank you.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 19, 2020

Hmm, I guess you must be doing some kind of automatic resizing when the container resizes. Are you using the built-in useViewBoxResizing or is this something homemade?

Personally I don't like viewBox and preserveAspectRatio - it's an easy way to get responsive layout but it stretches text and graphics instead of drawing them at the actual resolution. I prefer detecting a resize and doing a redraw with transitions disabled, as shown in the resizing examples.

Here is the latest explainer of this technique.

Since I don't like viewBox resizing, I have never tried d3-brush with it. It would not surprise me if they changed something so that it no longer works with viewBox / preserveAspectRatio.

@ewaldman
Copy link
Author

Yesterday we thought that the issue is a 'rendlet' problem. However, today we noticed that even when commenting out rendlet code the brushes don't work in a timeline. (Don't know why I thought it did work yesterday).

The package.json file we are using has this config

"dependencies": {
"crossfilter2": "^1.5.4",
"d3": "^6.2.0",
"dc": "^4.2.0"
}

You mentioned earlier that the brushes did work in your code. Can you please check using the above configuration?

Thank you

@gordonwoodhull
Copy link
Contributor

Yes, those are the versions used in the demos on the dc.js site. You can check versions in the browser console with

image

I guess I would need to see a running example to help you further.

@ewaldman
Copy link
Author

ewaldman commented Oct 21, 2020

Hi.

Here is the code we are using (obfuscated a bit to protect the innocent....). Perhaps you can tell what we need to do to fix it so that brushes should work.

        let data = [
            {name: "joe",  timeObserved: 1596819575959},
            { name: "jack",  timeObserved: 1596819575959},
            { name: "jim",  timeObserved: 1596819576000},
            { name: "tom",  timeObserved: 1596819576000},
            { name: "john",  timeObserved: 1596819576000},
            { name: "jill",  timeObserved: 1596819575949},
            { name: "craig",  timeObserved: 1596819580801},
            { name: "norbert",  timeObserved: 1596819575959},
            { name: "donald",  timeObserved: 1596819575949}
        ];
        let cf = crossfilter(data);

        this.timeLineChart = dc.lineChart('#timeLine');

        this.dateDim = cf.dimension((d) => {
            return d3.timeSecond(new Date(d.timeObserved));
        });

        this.group = this.dateDim.group().reduceSum((d) => 1);

        let minDate = d3.min(this.group.all(), (kv) => {
            return kv.key;
        });

        let maxDate = d3.max(this.group.all(), (kv) => {
            return kv.key;
        });


        let min = 0;
        let max = 20;
        this.timeLineChart
        .width(470).height(300)
        .dimension(this.dateDim)
        .group(this.group)
        .curve(d3.curveMonotoneX)
        .renderArea(true)
        .mouseZoomable(true)
        .elasticY(true)
        .yAxisPadding(.1)
        .elasticX(true)
        .x(d3.scaleTime().domain([minDate, maxDate]))
        .xUnits(d3.timeWeek.range)
        .yAxis()
        .tickValues(d3.range(min > 0 ? min - 1 : min, max + 1));
        this.timeLineChart.yAxisMin = function() {
            return 0;
        };

        dc.renderAll();

@gordonwoodhull
Copy link
Contributor

Thanks for the reproducible example!

I should have picked up earlier that you are using a time scale. I had not tested that, and it doesn't look like we currently have an example covering brushing a time-scaled line chart.

I will look into it.

@gordonwoodhull gordonwoodhull changed the title Brush not working in LineChart after upgrade to D3 v6 Brush not working in temporal LineChart after upgrade to D3 v6 Oct 22, 2020
@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 22, 2020

I did some testing and there is an erroneous zoom event sent after brushing is done, the next time the mouse is moved.

A temporary workaround could be to disable mouseZoomable. When mouse zooming is enabled, then any pan or zoom actions also set the filter and thus the brush. (Mouse zooming is strongly tied to range/focus charts.)

You could say that there is an inherent incompatibility between mouseZoomable and brushing: mouseZoomable is only designed to be used when the chart is a focus chart paired with a range chart (and the brush is only displayed on the range chart).

That said, it seems strange that we should get a zoom event when we haven't zoomed, so it's still a bug.

@gordonwoodhull
Copy link
Contributor

I can reproduce this on the fluctuation chart as well if mouseZoomable is enabled. So it's not specific to line charts or time scales.

@gordonwoodhull gordonwoodhull changed the title Brush not working in temporal LineChart after upgrade to D3 v6 brush resets to covering entire domain when zooming is enabled, after upgrade to D3 v6 Oct 22, 2020
@ewaldman
Copy link
Author

ewaldman commented Oct 22, 2020

We took out mouseZoomable and now the brush does indeed work.
Is there anyway we can be notified when mouse zooming is fixed?

Thank you so much for all your help.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 22, 2020

Sure, you are subscribed to this ticket, so you will see when we close it.

However, as I pointed out above, brushOn is not fully compatible with mouseZoomable. Even with d3@5, if you zoom the chart, the brush will be expanded to cover the whole chart, because mouseZoomable is tightly linked to the focus-range feature. This can cause the brush to appear from nowhere and cover the whole chart.

The behavior is awkward and I don't recommend using these features together.

@ewaldman
Copy link
Author

ewaldman commented Oct 22, 2020

We have a follow up question. We ripped out our home grown 'renderlet' code and started using useViewBoxResizing .
The code works, however our jasmine test throws an error now.

Chrome Headless 86.0.4240.111 (Windows 10) myService add to crossfilter FAILED
        TypeError: Cannot read property 'getAttribute' of null
            at Selection.attr (webpack:///node_modules/d3-selection/src/selection/attr.js:50:1 <- index_test.js:44240:16)
            at RowChart.sizeSvg (webpack:///node_modules/dc/dist/dc.js:1751:16 <- index_test.js:55385:37)
            at RowChart.redraw (webpack:///node_modules/dc/dist/dc.js:2003:1 <- index_test.js:55637:16)
            at Object.redrawAll (webpack:///node_modules/dc/dist/dc.js:394:1 <- index_test.js:54028:21)
            at myService.addToCrossfilter (webpack:///myService/js/main/myService.js:2379:10 <- index_test.js:6962:450968)
            at UserContext.<anonymous> (webpack:///myService/js/test/myService.test.js:927:1 <- index_test.js:8670:26)
            at <Jasmine>

The code for sizeSvg is in base-mixin.js. It seems to be failing in selection in D3 when calling this._svg.attr('viewBox')

    sizeSvg () {
        if (this._svg) {
            if (!this._useViewBoxResizing) {
                this._svg
                    .attr('width', this.width())
                    .attr('height', this.height());
            } else if (!this._svg.attr('viewBox')) {
                this._svg
                    .attr('viewBox', `0 0 ${this.width()} ${this.height()}`);
            }
        }
    }

What can we do for this?

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 22, 2020

The error indicates that this._svg is an empty selection. (Not null, but contains an empty array.)

A couple of conditions could cause this:

  • redrawing before rendering
  • bad selector passed to the constructor, so this._root is empty and therefore this._svg is empty as well

Often dc.js will just do nothing in these cases, but it's not guaranteed and sometimes it will crash.

@ewaldman
Copy link
Author

Our test case runs in a "headless chrome". This probably causes it to call redraw before rendering.
How would we work around this problem so that the test doesn't crash/fail?

As an aside, just a suggestion: The code above in sizeSvg is:

    if (this._svg) {

Why not add another part to the "if" statement:
if this._svg is not empty , so that the code further on does not crash.

Thank you

@gordonwoodhull
Copy link
Contributor

I don’t see why headless chrome would cause this. We use headless chrome for testing, and it still renders everything okay. It’s a fully functional browser; it just isn’t connected to any display.

The error indicates that something in your code is not doing what you expect. So, sure, we could work around the error, but it’s very likely that you have a bug. I don’t see a lot of profit in making sure that operations succeed when the chart is in an inconsistent state.

The most likely issue is the selector being wrong. Please check that chart.root().size() is not zero, and then check your selector (and make sure that the corresponding div exists).

Another common error is constructing the chart but never initializing it.

@ewaldman
Copy link
Author

Regarding the crash when we do testing - see above:

To recap, dc.renderAll does lots of work, and part of it is a call to sizeSvg. In there the code calls

if (!this._svg.attr('viewBox'))

The code in attr.js does the following:

export default function(name, value) {
  var fullname = namespace(name);

  if (arguments.length < 2) {
    var node = this.node();
    return fullname.local
        ? node.getAttributeNS(fullname.space, fullname.local)
        : node.getAttribute(fullname);
  }

  return this.each((value == null
      ? (fullname.local ? attrRemoveNS : attrRemove) : (typeof value === "function"
      ? (fullname.local ? attrFunctionNS : attrFunction)
      : (fullname.local ? attrConstantNS : attrConstant)))(fullname, value));
}

Following the code we see that the arguments dc passes in is indded less than 2 , - (svg.attr('viewBox')) - so the code
calls var node = this.node(); Turns out that node is null, so the next line of code crashes.

Question: What can we do about that? When running the code in real time, the code works well. But by jasmin testing it crashes as above. Our code merely calls dc.renderAll. That call causes this entire domino effect.

Any tips, ideas?

Thank you

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 30, 2020

Thanks @ewaldman.

As I said above, your charts are in an inconsistent state. There is no reason in normal operation why this._svg would contain a null selection when this.sizeSvg is called.

In particular, your selector is probably wrong when you initialize the chart. Take a look at generateSvg(), which will call resetSvg in the render case:

generateSvg () {
this._svg = this.root().append('svg');
if (this._svgDescription || this._keyboardAccessible) {
this._svg.append('desc')
.attr('id', `desc-id-${this.__dcFlag__}`)
.html(`${this.svgDescription()}`);
this._svg
.attr('tabindex', '0')
.attr('role', 'img')
.attr('aria-labelledby', `desc-id-${this.__dcFlag__}`);
}
this.sizeSvg();
return this._svg;
}

See how this._svg is generated at the top of the function and sizeSvg is called at the bottom, with nothing in between that would invalidate it.

Most likely, the selector passed to the chart contructor was wrong, so this.root() is an empty selection. Attempting to append an element to an empty selection will create another empty selection, and as you saw, attempting to fetch an attribute on an empty selection will crash D3.

I reproduced the error in this fiddle. Note the typo in the selector passed to the chart constructor, #testf:

    var chart = new dc.BarChart("#testf");

when #test would work.

My guess is that you have a selector which is not selecting anything, so your test was previously "running in a vacuum" and is now crashing.

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

2 participants