Plotly.js: scrolling legend height not recalculated on resize

[timelyportfolio]

Initially reported in https://github.com/ropensci/plotly/issues/525, I was able to isolate to a non-R JavaScript only example in codepen. I had hoped v1.7.0 would solve this, but unfortunately I just updated, and the problem persists.

I am happy to attempt to tackle this if desired. I see on line where it checks for firstRender to draw the legend. I'm wondering if we can handle by just removing this firstRender check.

[etpinard]

@timelyportfolio thanks for the report!

I am happy to attempt to tackle this if desired.

That would be great. Go for it!

[timelyportfolio]

So, this became far thornier than I initially expected. We have a scope problem with scrollheight defined in the scrollHandler eventListener callback (see lines) defined on initial draw that does not update on relayout. Also, the pattern data([0]).enter().append(...).... means legend items such as clippath (lines so clippath null in lines on relayout) will not be updated. I have almost convinced myself that the legend should partially or completely redraw each relayout but then we might lose legend selections. @etpinard, any thoughts before I assemble a pull, since this will be my first?

The full brute force method would be to

legend.html("");
...
// and then remove the check for firstRender
// // If scrollbar should be shown.
//    if(gd.firstRender && opts.height - scrollheight > 0 && !gd._context.staticPlot) {
if(opts.height - scrollheight > 0 && !gd._context.staticPlot) {

[etpinard]

Moving block 1 and 2 above the gd.firstRender clause should suffice for this PR.

It is important to attach event listeners to the legend only once per graph (hence that gd.firstRender block), but re-layout calls should reset the clip paths and bounding rectangle dimensions,

@mdtusz Any other insights?

[mdtusz]

That should solve the sizing issue.

So long as the existing eventListeners are removed, I see no problem with re-setting them. It may take a bit of fiddling to get the order of operations right and will require a check to see that you aren't adding a listener to a non-existent scroll, but it would ensure the correct value in the scrollHandler.

[etpinard]

For the record, data([0]).enter().append(...).... does not mean that the legend items aren't updated on relayout. It indicates that there will one and only one of these node on the graph at all times.

More thoroughly,

// select all clippath nodes with id 'clipId' => 0 node on first pass, 1 node afterwards
var clipPath = fullLayout._topdefs.selectAll('#' + clipId);
      // bind an array of length 1 to selection 
        .data([0])
      // grabs the enter selection => has 1 node in first pass, 0 node afterwards
      .enter()
     // from enter selection => append <clipPath> with id 'clipId'
    .append('clipPath')
        .attr('id', clipId)
    //  and append a <rect> inside <clipPath>
        .append('rect');

[timelyportfolio]

Thanks for the clarification. I understand what you are saying, and I'll attempt to demonstrate what is happening in this instance with a narrow CodePen.

[timelyportfolio]

@etpinard, I think this CodePen might explain my statement. Notice how only the second p updates. The same thing is happening with clippath. If we look at the original codepen demo of problem after we push the resize button,

and since clipPath === null then the attributes are not changed.

[etpinard]

@timelyportfolio nice catch.

This block should be

var clipPath = fullLayout._topdefs.selectAll('#' + clipId)
        .data([0]);

clipPath.enter().append('clipPath')
        .attr('id', clipId)
        .append('rect')

my mistake.

[timelyportfolio]

So, I think the only thing left is we need to somehow pass the new scrollheight to scrollHandler. Any suggestions here besides remove and add back the eventListeners or some lazy evaluation of a new scrollheight function? In the screenshot below, we can see that scrollheight still carries the same value on resize.

[etpinard]

Any suggestions here besides remove and add back the eventListeners

I don't. I'd would simply remove/add the handlers.