Multiple legend positioning and sizing fixes #4160

etpinard · 2019-08-30T18:40:43Z

This PR essentially supersedes #3024, fixes #4132 and will resolve #771

In brief,

commit de369f3 improves legend x / y attribute declarations
commits 1cf347a and f12c62c remove obsolete logic from the legend/draw.js
mocks of the problems listed in Legend overlaps since 1.11.0 #771 (with baselines as of 1.49.4) are added in bf586d4 (some of the mocks were taken from improve legend positioning in edge cases #3024 - thanks @antoinerg !)
488cca1 is the most important commit, it fixes most of the issues presented in Legend overlaps since 1.11.0 #771
c33c92f adds extra logic around margin.autoexpand: false in order to present a solution to a Legend overlaps since 1.11.0 #771 scenario
commits 1e6862a and 1f00a40 add some Lib.log calls that should make legend auto-margin / positioning edge cases easier to spot
commits 2b2bd9b and 6b0befe present and fix Ability to constrain legend background to the legend text #4132
the rest are essentially linting commits

Reviewers may be interested in reviewing

Lines 98 to 118 in 6b0befe

    
           var gs = fullLayout._size; 
        
           var bw = opts.borderwidth; 
        
           var lx = gs.l + gs.w * opts.x - FROM_TL[getXanchor(opts)] * opts._width; 
        
           var ly = gs.t + gs.h * (1 - opts.y) - FROM_TL[getYanchor(opts)] * opts._effHeight; 
        
           if(fullLayout.margin.autoexpand) { 
        
               var lx0 = lx; 
        
               var ly0 = ly; 
        
               lx = Lib.constrain(lx, 0, fullLayout.width - opts._width); 
        
               ly = Lib.constrain(ly, 0, fullLayout.height - opts._effHeight); 
        
               if(lx !== lx0) { 
        
                   Lib.log('Constrain legend.x to make legend fit inside graph'); 
        
               } 
        
               if(ly !== ly0) { 
        
                   Lib.log('Constrain legend.y to make legend fit inside graph'); 
        
               } 
        
           }

plotly.js/src/components/legend/draw.js

Lines 462 to 627 in 6b0befe

    
           /* 
        
            * Computes in fullLayout.legend: 
        
            * 
        
            *  - _height: legend height including items past scrollbox height 
        
            *  - _maxHeight: maximum legend height before scrollbox is required 
        
            *  - _effHeight: legend height w/ or w/o scrollbox 
        
            * 
        
            *  - _width: legend width 
        
            *  - _maxWidth (for orientation:h only): maximum width before starting new row 
        
            */ 
        
           function computeLegendDimensions(gd, groups, traces) { 
        
               var fullLayout = gd._fullLayout; 
        
               var opts = fullLayout.legend; 
        
               var gs = fullLayout._size; 
        
               var isVertical = helpers.isVertical(opts); 
        
               var isGrouped = helpers.isGrouped(opts); 
        
               var bw = opts.borderwidth; 
        
               var bw2 = 2 * bw; 
        
               var textGap = constants.textGap; 
        
               var itemGap = constants.itemGap; 
        
               var endPad = 2 * (bw + itemGap); 
        
               var yanchor = getYanchor(opts); 
        
               var isBelowPlotArea = opts.y < 0 || (opts.y === 0 && yanchor === 'top'); 
        
               var isAbovePlotArea = opts.y > 1 || (opts.y === 1 && yanchor === 'bottom'); 
        
               // - if below/above plot area, give it the maximum potential margin-push value 
        
               // - otherwise, extend the height of the plot area 
        
               opts._maxHeight = Math.max( 
        
                   (isBelowPlotArea || isAbovePlotArea) ? fullLayout.height / 2 : gs.h, 
        
                   30 
        
               ); 
        
               var toggleRectWidth = 0; 
        
               opts._width = 0; 
        
               opts._height = 0; 
        
               if(isVertical) { 
        
                   traces.each(function(d) { 
        
                       var h = d[0].height; 
        
                       Drawing.setTranslate(this, bw, itemGap + bw + opts._height + h / 2); 
        
                       opts._height += h; 
        
                       opts._width = Math.max(opts._width, d[0].width); 
        
                   }); 
        
                   toggleRectWidth = textGap + opts._width; 
        
                   opts._width += itemGap + textGap + bw2; 
        
                   opts._height += endPad; 
        
                   if(isGrouped) { 
        
                       groups.each(function(d, i) { 
        
                           Drawing.setTranslate(this, 0, i * opts.tracegroupgap); 
        
                       }); 
        
                       opts._height += (opts._lgroupsLength - 1) * opts.tracegroupgap; 
        
                   } 
        
               } else { 
        
                   var xanchor = getXanchor(opts); 
        
                   var isLeftOfPlotArea = opts.x < 0 || (opts.x === 0 && xanchor === 'right'); 
        
                   var isRightOfPlotArea = opts.x > 1 || (opts.x === 1 && xanchor === 'left'); 
        
                   var isBeyondPlotAreaX = isAbovePlotArea || isBelowPlotArea; 
        
                   var hw = fullLayout.width / 2; 
        
                   // - if placed within x-margins, extend the width of the plot area 
        
                   // - else if below/above plot area and anchored in the margin, extend to opposite margin, 
        
                   // - otherwise give it the maximum potential margin-push value 
        
                   opts._maxWidth = Math.max( 
        
                       isLeftOfPlotArea ? ((isBeyondPlotAreaX && xanchor === 'left') ? gs.l + gs.w : hw) : 
        
                       isRightOfPlotArea ? ((isBeyondPlotAreaX && yanchor === 'right') ? gs.r + gs.w : hw) : 
        
                       gs.w, 
        
                   2 * textGap); 
        
                   var maxItemWidth = 0; 
        
                   var combinedItemWidth = 0; 
        
                   traces.each(function(d) { 
        
                       var w = d[0].width + textGap; 
        
                       maxItemWidth = Math.max(maxItemWidth, w); 
        
                       combinedItemWidth += w; 
        
                   }); 
        
                   toggleRectWidth = null; 
        
                   var maxRowWidth = 0; 
        
                   if(isGrouped) { 
        
                       var maxGroupHeightInRow = 0; 
        
                       var groupOffsetX = 0; 
        
                       var groupOffsetY = 0; 
        
                       groups.each(function() { 
        
                           var maxWidthInGroup = 0; 
        
                           var offsetY = 0; 
        
                           d3.select(this).selectAll('g.traces').each(function(d) { 
        
                               var h = d[0].height; 
        
                               Drawing.setTranslate(this, 0, itemGap + bw + h / 2 + offsetY); 
        
                               offsetY += h; 
        
                               maxWidthInGroup = Math.max(maxWidthInGroup, textGap + d[0].width); 
        
                           }); 
        
                           maxGroupHeightInRow = Math.max(maxGroupHeightInRow, offsetY); 
        
                           var next = maxWidthInGroup + itemGap; 
        
                           if((next + bw + groupOffsetX) > opts._maxWidth) { 
        
                               maxRowWidth = Math.max(maxRowWidth, groupOffsetX); 
        
                               groupOffsetX = 0; 
        
                               groupOffsetY += maxGroupHeightInRow + opts.tracegroupgap; 
        
                               maxGroupHeightInRow = offsetY; 
        
                           } 
        
                           Drawing.setTranslate(this, groupOffsetX, groupOffsetY); 
        
                           groupOffsetX += next; 
        
                       }); 
        
                       opts._width = Math.max(maxRowWidth, groupOffsetX) + bw; 
        
                       opts._height = groupOffsetY + maxGroupHeightInRow + endPad; 
        
                   } else { 
        
                       var nTraces = traces.size(); 
        
                       var oneRowLegend = (combinedItemWidth + bw2 + (nTraces - 1) * itemGap) < opts._maxWidth; 
        
                       var maxItemHeightInRow = 0; 
        
                       var offsetX = 0; 
        
                       var offsetY = 0; 
        
                       var rowWidth = 0; 
        
                       traces.each(function(d) { 
        
                           var h = d[0].height; 
        
                           var w = textGap + d[0].width; 
        
                           var next = (oneRowLegend ? w : maxItemWidth) + itemGap; 
        
                           if((next + bw + offsetX) > opts._maxWidth) { 
        
                               maxRowWidth = Math.max(maxRowWidth, rowWidth); 
        
                               offsetX = 0; 
        
                               offsetY += maxItemHeightInRow; 
        
                               opts._height += maxItemHeightInRow; 
        
                               maxItemHeightInRow = 0; 
        
                           } 
        
                           Drawing.setTranslate(this, bw + offsetX, itemGap + bw + h / 2 + offsetY); 
        
                           rowWidth = offsetX + w + itemGap; 
        
                           offsetX += next; 
        
                           maxItemHeightInRow = Math.max(maxItemHeightInRow, h); 
        
                       }); 
        
                       if(oneRowLegend) { 
        
                           opts._width = offsetX + bw2; 
        
                           opts._height = maxItemHeightInRow + endPad; 
        
                       } else { 
        
                           opts._width = Math.max(maxRowWidth, rowWidth) + bw2; 
        
                           opts._height += maxItemHeightInRow + endPad; 
        
                       } 
        
                   } 
        
               } 
        
               opts._width = Math.ceil(opts._width); 
        
               opts._height = Math.ceil(opts._height); 
        
               opts._effHeight = Math.min(opts._height, opts._maxHeight); 
        
               var edits = gd._context.edits; 
        
               var isEditable = edits.legendText || edits.legendPosition; 
        
               traces.each(function(d) { 
        
                   var traceToggle = d3.select(this).select('.legendtoggle'); 
        
                   var h = d[0].height; 
        
                   var w = isEditable ? textGap : (toggleRectWidth || (textGap + d[0].width)); 
        
                   if(!isVertical) w += itemGap / 2; 
        
                   Drawing.setRect(traceToggle, 0, -h / 2, w, h); 
        
               }); 
        
           }

in legend/draw.js with their corresponding blocks on the current master:

plotly.js/src/components/legend/draw.js

Lines 133 to 195 in 37e80cb

    
           // Position and size the legend 
        
           var lxMin = 0; 
        
           var lxMax = fullLayout.width; 
        
           var lyMin = 0; 
        
           var lyMax = fullLayout.height; 
        
           computeLegendDimensions(gd, groups, traces); 
        
           if(opts._height > lyMax) { 
        
               // If the legend doesn't fit in the plot area, 
        
               // do not expand the vertical margins. 
        
               expandHorizontalMargin(gd); 
        
           } else { 
        
               expandMargin(gd); 
        
           } 
        
           // Scroll section must be executed after repositionLegend. 
        
           // It requires the legend width, height, x and y to position the scrollbox 
        
           // and these values are mutated in repositionLegend. 
        
           var gs = fullLayout._size; 
        
           var lx = gs.l + gs.w * opts.x; 
        
           var ly = gs.t + gs.h * (1 - opts.y); 
        
           if(Lib.isRightAnchor(opts)) { 
        
               lx -= opts._width; 
        
           } else if(Lib.isCenterAnchor(opts)) { 
        
               lx -= opts._width / 2; 
        
           } 
        
           if(Lib.isBottomAnchor(opts)) { 
        
               ly -= opts._height; 
        
           } else if(Lib.isMiddleAnchor(opts)) { 
        
               ly -= opts._height / 2; 
        
           } 
        
           // Make sure the legend left and right sides are visible 
        
           var legendWidth = opts._width; 
        
           var legendWidthMax = gs.w; 
        
           if(legendWidth > legendWidthMax) { 
        
               lx = gs.l; 
        
               legendWidth = legendWidthMax; 
        
           } else { 
        
               if(lx + legendWidth > lxMax) lx = lxMax - legendWidth; 
        
               if(lx < lxMin) lx = lxMin; 
        
               legendWidth = Math.min(lxMax - lx, opts._width); 
        
           } 
        
           // Make sure the legend top and bottom are visible 
        
           // (legends with a scroll bar are not allowed to stretch beyond the extended 
        
           // margins) 
        
           var legendHeight = opts._height; 
        
           var legendHeightMax = gs.h; 
        
           if(legendHeight > legendHeightMax) { 
        
               ly = gs.t; 
        
               legendHeight = legendHeightMax; 
        
           } else { 
        
               if(ly + legendHeight > lyMax) ly = lyMax - legendHeight; 
        
               if(ly < lyMin) ly = lyMin; 
        
               legendHeight = Math.min(lyMax - ly, opts._height); 
        
           }

plotly.js/src/components/legend/draw.js

Lines 541 to 724 in 37e80cb

    
           function computeLegendDimensions(gd, groups, traces) { 
        
               var fullLayout = gd._fullLayout; 
        
               var opts = fullLayout.legend; 
        
               var borderwidth = opts.borderwidth; 
        
               var isGrouped = helpers.isGrouped(opts); 
        
               var extraWidth = 0; 
        
               var traceGap = 5; 
        
               opts._width = 0; 
        
               opts._height = 0; 
        
               if(helpers.isVertical(opts)) { 
        
                   if(isGrouped) { 
        
                       groups.each(function(d, i) { 
        
                           Drawing.setTranslate(this, 0, i * opts.tracegroupgap); 
        
                       }); 
        
                   } 
        
                   traces.each(function(d) { 
        
                       var legendItem = d[0]; 
        
                       var textHeight = legendItem.height; 
        
                       var textWidth = legendItem.width; 
        
                       Drawing.setTranslate(this, 
        
                           borderwidth, 
        
                           (5 + borderwidth + opts._height + textHeight / 2)); 
        
                       opts._height += textHeight; 
        
                       opts._width = Math.max(opts._width, textWidth); 
        
                   }); 
        
                   opts._width += 45 + borderwidth * 2; 
        
                   opts._height += 10 + borderwidth * 2; 
        
                   if(isGrouped) { 
        
                       opts._height += (opts._lgroupsLength - 1) * opts.tracegroupgap; 
        
                   } 
        
                   extraWidth = 40; 
        
               } else if(isGrouped) { 
        
                   var maxHeight = 0; 
        
                   var maxWidth = 0; 
        
                   var groupData = groups.data(); 
        
                   var maxItems = 0; 
        
                   var i; 
        
                   for(i = 0; i < groupData.length; i++) { 
        
                       var group = groupData[i]; 
        
                       var groupWidths = group.map(function(legendItemArray) { 
        
                           return legendItemArray[0].width; 
        
                       }); 
        
                       var groupWidth = Lib.aggNums(Math.max, null, groupWidths); 
        
                       var groupHeight = group.reduce(function(a, b) { 
        
                           return a + b[0].height; 
        
                       }, 0); 
        
                       maxWidth = Math.max(maxWidth, groupWidth); 
        
                       maxHeight = Math.max(maxHeight, groupHeight); 
        
                       maxItems = Math.max(maxItems, group.length); 
        
                   } 
        
                   maxWidth += traceGap; 
        
                   maxWidth += 40; 
        
                   var groupXOffsets = [opts._width]; 
        
                   var groupYOffsets = []; 
        
                   var rowNum = 0; 
        
                   for(i = 0; i < groupData.length; i++) { 
        
                       if(fullLayout._size.w < (borderwidth + opts._width + traceGap + maxWidth)) { 
        
                           groupXOffsets[groupXOffsets.length - 1] = groupXOffsets[0]; 
        
                           opts._width = maxWidth; 
        
                           rowNum++; 
        
                       } else { 
        
                           opts._width += maxWidth + borderwidth; 
        
                       } 
        
                       var rowYOffset = (rowNum * maxHeight); 
        
                       rowYOffset += rowNum > 0 ? opts.tracegroupgap : 0; 
        
                       groupYOffsets.push(rowYOffset); 
        
                       groupXOffsets.push(opts._width); 
        
                   } 
        
                   groups.each(function(d, i) { 
        
                       Drawing.setTranslate(this, groupXOffsets[i], groupYOffsets[i]); 
        
                   }); 
        
                   groups.each(function() { 
        
                       var group = d3.select(this); 
        
                       var groupTraces = group.selectAll('g.traces'); 
        
                       var groupHeight = 0; 
        
                       groupTraces.each(function(d) { 
        
                           var legendItem = d[0]; 
        
                           var textHeight = legendItem.height; 
        
                           Drawing.setTranslate(this, 
        
                               0, 
        
                               (5 + borderwidth + groupHeight + textHeight / 2)); 
        
                           groupHeight += textHeight; 
        
                       }); 
        
                   }); 
        
                   var maxYLegend = groupYOffsets[groupYOffsets.length - 1] + maxHeight; 
        
                   opts._height = 10 + (borderwidth * 2) + maxYLegend; 
        
                   var maxOffset = Math.max.apply(null, groupXOffsets); 
        
                   opts._width = maxOffset + maxWidth + 40; 
        
                   opts._width += borderwidth * 2; 
        
               } else { 
        
                   var rowHeight = 0; 
        
                   var maxTraceHeight = 0; 
        
                   var maxTraceWidth = 0; 
        
                   var offsetX = 0; 
        
                   var fullTracesWidth = 0; 
        
                   // calculate largest width for traces and use for width of all legend items 
        
                   traces.each(function(d) { 
        
                       maxTraceWidth = Math.max(40 + d[0].width, maxTraceWidth); 
        
                       fullTracesWidth += 40 + d[0].width + traceGap; 
        
                   }); 
        
                   // check if legend fits in one row 
        
                   var oneRowLegend = fullLayout._size.w > borderwidth + fullTracesWidth - traceGap; 
        
                   traces.each(function(d) { 
        
                       var legendItem = d[0]; 
        
                       var traceWidth = oneRowLegend ? 40 + d[0].width : maxTraceWidth; 
        
                       if((borderwidth + offsetX + traceGap + traceWidth) > fullLayout._size.w) { 
        
                           offsetX = 0; 
        
                           rowHeight += maxTraceHeight; 
        
                           opts._height += maxTraceHeight; 
        
                           // reset for next row 
        
                           maxTraceHeight = 0; 
        
                       } 
        
                       Drawing.setTranslate(this, 
        
                           (borderwidth + offsetX), 
        
                           (5 + borderwidth + legendItem.height / 2) + rowHeight); 
        
                       opts._width += traceGap + traceWidth; 
        
                       // keep track of tallest trace in group 
        
                       offsetX += traceGap + traceWidth; 
        
                       maxTraceHeight = Math.max(legendItem.height, maxTraceHeight); 
        
                   }); 
        
                   if(oneRowLegend) { 
        
                       opts._height = maxTraceHeight; 
        
                   } else { 
        
                       opts._height += maxTraceHeight; 
        
                   } 
        
                   opts._width += borderwidth * 2; 
        
                   opts._height += 10 + borderwidth * 2; 
        
               } 
        
               // make sure we're only getting full pixels 
        
               opts._width = Math.ceil(opts._width); 
        
               opts._height = Math.ceil(opts._height); 
        
               var isEditable = ( 
        
                   gd._context.edits.legendText || 
        
                   gd._context.edits.legendPosition 
        
               ); 
        
               traces.each(function(d) { 
        
                   var legendItem = d[0]; 
        
                   var bg = d3.select(this).select('.legendtoggle'); 
        
                   Drawing.setRect(bg, 
        
                       0, 
        
                       -legendItem.height / 2, 
        
                       (isEditable ? 0 : opts._width) + extraWidth, 
        
                       legendItem.height 
        
                   ); 
        
               }); 
        
           }

cc @plotly/plotly_js

... src/lib/anchor_utils.js has the same logic

- do not list `dflt` in schema as their value depends on the legend orientation (and rangeslider ;) - improve attr descriptions - add comment for potential v2 improvement

... during calcdata loop, instead of in Legend.draw this allows us to 🔪 one loop over the data that plus some linting.

- set scrollBar enter attrs in constants file - rename textOffsetX -> textGap, add itemGap constants ... and use `bw` for borderwidth and `h` for d[0].height making formula cleaner in computeLegendDimensions

... which doesn't appear to do anything, and mv computeLegendDimension to separate step

- use fullLayout.(width|height) instead of ly(Min|Max) - use bw instead of opts.borderwidth - 🔪 double-empty-line - 🔪 some useless comments

... the current width didn't lead to any bugs, but it was too large in most cases. - make horiz. grouped / non-grouped logic more similar - DRY up x/y anchor logic

- mocks with negative x with orientation h - mock with small viewports - mock with legend beyond plotarea + scrollbox - mock with long item leading to x margin push

- see #771 - introduce measures _maxHeight, _maxWidth and _effHeight to track margin pushes, scrollbox and horizontal row wrapping - simplify (and fix) legend (x,y) constraint into graph width/height - introduce some l,r,b,y "max" margin-push logic - paves way to expose legend `maxheight` and `maxwidth` - update baselines! + from previous "new legend mocks" commit + and `horizontal_wrap-alll-lines` which now spans the graph's width

- when one sets autoexpand to false, fullLayout.margin are honoured no matter how big the legend gets. In this case, (I'd argue) the legend should NOT move from its provided (or default) (x,y) position to make it fit on the graph. - if some users want the legend to auto-expand the margin while keeping the provided/default (x,y), we could eventually add a `legend.fitinside` boolean attribute

- N.B. need to remove legend from splom test to get Lib.log counter right.

- which gives better margin pushes, fixes legendgroup_horizontal_wrapping mock - use per-item width for toggle-rect width in ALL horizontal legends, we could eventually compute "per-column" width, if someone finds it useful

nicolaskruchten · 2019-09-03T13:46:15Z

Is this on the way to resolving #1199 or unrelated?

etpinard · 2019-09-03T15:03:45Z

@antoinerg would you mind reviewing this one?

antoinerg · 2019-09-03T16:29:39Z

Thanks for the nice PR and the clear breakdown of your different commits 👍

Here's my first comment/question following Alex's suggestion on #3024 to:

Can you check what happens with config: {editable: true} when you drag the legend around and bump up against these various limits? Does the legend end up where you expected after you drop it?

@etpinard is this the expected behavior:

etpinard · 2019-09-03T18:59:14Z

Ok, here's what happens when you drag the legend to the right off the legend_negative_x mock under editable:true:

relayout with a new legend.x (and legend.y) is called on mouse up
the new automargin push values are set to 0 here:

plotly.js/src/plots/plots.js

Line 1824 in 6b0befe

Lib.log('Margin push', id, 'is too big in x, dropping');

as the legend width is larger than half the layout width.

when the new legend.x is less than 1, this results in no right-margin push.
as we can't fit the legend within the graph's width with the new legend.x, we constrain its position here:

plotly.js/src/components/legend/draw.js

Line 108 in 6b0befe

lx = Lib.constrain(lx, 0, fullLayout.width - opts._width);

snapping the legend back to the left.

Now here's what happens when one drags the legend to the right past the plot area:

here, the max horizontal legend width is computed with:

plotly.js/src/components/legend/draw.js

Lines 528 to 531 in 6b0befe

    
           opts._maxWidth = Math.max( 
        
               isLeftOfPlotArea ? ((isBeyondPlotAreaX && xanchor === 'left') ? gs.l + gs.w : hw) : 
        
               isRightOfPlotArea ? ((isBeyondPlotAreaX && yanchor === 'right') ? gs.r + gs.w : hw) : 
        
               gs.w,

is fullLayout.width / 2, and the right-margin gets pushed.

It's not obvious to me how we should fix this problem. I'm thinking we could refine

plotly.js/src/plots/plots.js

Line 1824 in 6b0befe

Lib.log('Margin push', id, 'is too big in x, dropping');

by considering the push-margin x (and y) position before "dropping" the push-margin values. In #3024, looks like @antoinerg tried to scale down the push-margin values instead of dropping them. If someone has a better idea, please let me know.

antoinerg · 2019-09-03T20:07:58Z

as we can't fit the legend within the graph's width with the new legend.x, we constrain its position here:

plotly.js/src/components/legend/draw.js

Line 108 in 6b0befe

lx = Lib.constrain(lx, 0, fullLayout.width - opts._width);

snapping the legend back to the left.

@etpinard It seems however that it does not snap back to the left enough: the item for trace 0 is clipped (the blue trace).

etpinard · 2019-09-04T13:48:50Z

It seems however that it does not snap back to the left enough: the item for trace 0 is clipped (the blue trace).

Hmm. I can't replicate that. Can you check the fullLayout.legend.x value for which this happens? That would help me out a lot!

antoinerg · 2019-09-04T19:58:21Z

Hmm. I can't replicate that. Can you check the fullLayout.legend.x value for which this happens? That would help me out a lot!

The value for gd._fullLayout.legend.x will depend on how far I drop the legend to the right. On Chrome and FF it reproducibly position the legend such that trace 0 is outside the figure and clipped. 🤔

etpinard · 2019-09-05T16:01:20Z

Is this on the way to resolving #1199 or unrelated?

This PR fixes a bunch of positioning bugs for horizontal legends, so it should make it easier for users to workaround the problems of #1199. But this PR does not make an attempt at fixing axis label <--> legend overlaps.

... to avoid race conditions leading to wrong legend position and size.

etpinard · 2019-09-10T19:20:02Z

@antoinerg thanks very much for spotting that bug!!

There was a race condition in Legend.draw - commit 42d5e4e fixes it.

Dragging around the legend_negative_x legend in editable:true mode now does this:

which appears "ok" to me. There are still some less-than-ideal scenarios. These should be much easier to solve once #2704 is completed. Let me know what you think!

antoinerg · 2019-09-23T17:59:42Z

This is a massive improvement for legend positioning. It seems like the several issues grouped in #771 are fixed as well as #4132 🎉

There are still some less-than-ideal scenarios.

I think the most confusing scenario for users is when they set legend.x to a value between 0 and 1. In this case, we don't expand the right margin and force the legend's position (as shown in your GIF). If the user really wants the legend to be at the provided legend.x, he/she will need to manually increase the right margin. It's probably OK for now but maybe we should improve on this once #2704 is completed.

One last non-blocking comment: I noticed that layout.margin.autoexpand doesn't have a description. It might be a good idea to add one either here or in #2704 .

Great job 💪 @etpinard

💃 💃 💃

etpinard · 2019-09-23T20:24:47Z

Thanks very much for the review @antoinerg

I pushed d043151 adding a description for autoexpand

Merging.

destradafilm · 2020-02-18T18:18:23Z

Has the suggested "fitinside" option ever been added? I've got an issue with legends crawling up to overlap the chart after resizing the window, and nothing in any of these commits or issues fixes it - but your codepen demoing "fitinside" on issue #771 had the correct behavior.

I still see the problem of either choosing the legend overlapping the chart or legend with cut off text when legend labels are long. Would it be possible to have an option to wrap text in the legend or overflow auto with a max-width and a scroll bar?

etpinard added 15 commits August 30, 2019 11:10

rm useless legend/anchor_utils.js file

3ab7713

... src/lib/anchor_utils.js has the same logic

improve legend x, y, xanchor, yanchor attr declarations

de369f3

- do not list `dflt` in schema as their value depends on the legend orientation (and rangeslider ;) - improve attr descriptions - add comment for potential v2 improvement

drop firstRender logic in Legend.draw

1cf347a

calc max name/label length in getLegendData

82b9769

... during calcdata loop, instead of in Legend.draw this allows us to 🔪 one loop over the data that plus some linting.

add a few things to legend/constants.js

0c497bb

- set scrollBar enter attrs in constants file - rename textOffsetX -> textGap, add itemGap constants ... and use `bw` for borderwidth and `h` for d[0].height making formula cleaner in computeLegendDimensions

drop expandHorizontalMargin logic

f12c62c

... which doesn't appear to do anything, and mv computeLegendDimension to separate step

some more linting

d770138

- use fullLayout.(width|height) instead of ly(Min|Max) - use bw instead of opts.borderwidth - 🔪 double-empty-line - 🔪 some useless comments

trim toggle-rect to more-precise width

51df66d

... the current width didn't lead to any bugs, but it was too large in most cases. - make horiz. grouped / non-grouped logic more similar - DRY up x/y anchor logic

add more legend mocks (with 1.49.4 baselines)

bf586d4

- mocks with negative x with orientation h - mock with small viewports - mock with legend beyond plotarea + scrollbox - mock with long item leading to x margin push

Lib.log when margin push are dropped

1e6862a

add Lib.log when legend (x,y) are constrained into graph width/height

1f00a40

- N.B. need to remove legend from splom test to get Lib.log counter right.

add mocks showing #4132 bug

2b2bd9b

fix #4132 - fix horizontal legend dimension computations

6b0befe

- which gives better margin pushes, fixes legendgroup_horizontal_wrapping mock - use per-item width for toggle-rect width in ALL horizontal legends, we could eventually compute "per-column" width, if someone finds it useful

etpinard added bug something broken status: reviewable labels Aug 30, 2019

etpinard added this to the v1.50.0 milestone Aug 30, 2019

etpinard mentioned this pull request Aug 30, 2019

improve legend positioning in edge cases #3024

Closed

antoinerg self-requested a review September 3, 2019 16:06

antoinerg self-assigned this Sep 3, 2019

antoinerg removed their request for review September 3, 2019 16:06

etpinard added 4 commits September 9, 2019 13:31

Merge branch 'master' into legend-horizontal-computed-dims-fix

6b2b63b

cleanup legend relayout tests

e92ca98

fixup variable name in legend maxWidth logic block

35b0c92

end Legend.draw early when margin-redraw in-progress

42d5e4e

... to avoid race conditions leading to wrong legend position and size.

add margin.autoexpand attr description

d043151

etpinard merged commit 43c4e0d into master Sep 23, 2019

etpinard deleted the legend-horizontal-computed-dims-fix branch September 23, 2019 20:24

etpinard mentioned this pull request Sep 30, 2019

Legend drag gets misaligned in the bottom 2/3 of the plot #1438

Closed

etpinard mentioned this pull request Oct 11, 2019

Guard against mirrored+automargin anchor:'free' axes #4273

Merged

archmoj mentioned this pull request Nov 6, 2019

Grouped legend alignment issue from 1.50 #4336

Open

snyk-bot mentioned this pull request Feb 4, 2020

[Snyk] Upgrade plotly.js from 1.45.3 to 1.52.0 beauremus/ftp-viewer#1

Merged

snyk-bot mentioned this pull request Feb 22, 2020

[Snyk] Upgrade plotly.js from 1.35.2 to 1.52.1 raindigi/eShopOnContainersAI#1

Open

snyk-bot mentioned this pull request Apr 17, 2020

[Snyk] Upgrade plotly.js from 1.41.3 to 1.52.3 dunzoit/redash#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple legend positioning and sizing fixes #4160

Multiple legend positioning and sizing fixes #4160

etpinard commented Aug 30, 2019

nicolaskruchten commented Sep 3, 2019

etpinard commented Sep 3, 2019

antoinerg commented Sep 3, 2019

etpinard commented Sep 3, 2019

antoinerg commented Sep 3, 2019 •

edited

Loading

etpinard commented Sep 4, 2019 •

edited

Loading

antoinerg commented Sep 4, 2019

etpinard commented Sep 5, 2019

etpinard commented Sep 10, 2019

antoinerg commented Sep 23, 2019

etpinard commented Sep 23, 2019

destradafilm commented Feb 18, 2020 •

edited

Loading

	var gs = fullLayout._size;
	var bw = opts.borderwidth;

	var lx = gs.l + gs.w * opts.x - FROM_TL[getXanchor(opts)] * opts._width;
	var ly = gs.t + gs.h * (1 - opts.y) - FROM_TL[getYanchor(opts)] * opts._effHeight;

	if(fullLayout.margin.autoexpand) {
	var lx0 = lx;
	var ly0 = ly;

	lx = Lib.constrain(lx, 0, fullLayout.width - opts._width);
	ly = Lib.constrain(ly, 0, fullLayout.height - opts._effHeight);

	if(lx !== lx0) {
	Lib.log('Constrain legend.x to make legend fit inside graph');
	}
	if(ly !== ly0) {
	Lib.log('Constrain legend.y to make legend fit inside graph');
	}
	}

	/*
	* Computes in fullLayout.legend:
	*
	* - _height: legend height including items past scrollbox height
	* - _maxHeight: maximum legend height before scrollbox is required
	* - _effHeight: legend height w/ or w/o scrollbox
	*
	* - _width: legend width
	* - _maxWidth (for orientation:h only): maximum width before starting new row
	*/
	function computeLegendDimensions(gd, groups, traces) {
	var fullLayout = gd._fullLayout;
	var opts = fullLayout.legend;
	var gs = fullLayout._size;
	var isVertical = helpers.isVertical(opts);
	var isGrouped = helpers.isGrouped(opts);

	var bw = opts.borderwidth;
	var bw2 = 2 * bw;
	var textGap = constants.textGap;
	var itemGap = constants.itemGap;
	var endPad = 2 * (bw + itemGap);

	var yanchor = getYanchor(opts);
	var isBelowPlotArea = opts.y < 0 \|\| (opts.y === 0 && yanchor === 'top');
	var isAbovePlotArea = opts.y > 1 \|\| (opts.y === 1 && yanchor === 'bottom');

	// - if below/above plot area, give it the maximum potential margin-push value
	// - otherwise, extend the height of the plot area
	opts._maxHeight = Math.max(
	(isBelowPlotArea \|\| isAbovePlotArea) ? fullLayout.height / 2 : gs.h,
	30
	);

	var toggleRectWidth = 0;
	opts._width = 0;
	opts._height = 0;

	if(isVertical) {
	traces.each(function(d) {
	var h = d[0].height;
	Drawing.setTranslate(this, bw, itemGap + bw + opts._height + h / 2);
	opts._height += h;
	opts._width = Math.max(opts._width, d[0].width);
	});

	toggleRectWidth = textGap + opts._width;
	opts._width += itemGap + textGap + bw2;
	opts._height += endPad;

	if(isGrouped) {
	groups.each(function(d, i) {
	Drawing.setTranslate(this, 0, i * opts.tracegroupgap);
	});
	opts._height += (opts._lgroupsLength - 1) * opts.tracegroupgap;
	}
	} else {
	var xanchor = getXanchor(opts);
	var isLeftOfPlotArea = opts.x < 0 \|\| (opts.x === 0 && xanchor === 'right');
	var isRightOfPlotArea = opts.x > 1 \|\| (opts.x === 1 && xanchor === 'left');
	var isBeyondPlotAreaX = isAbovePlotArea \|\| isBelowPlotArea;
	var hw = fullLayout.width / 2;

	// - if placed within x-margins, extend the width of the plot area
	// - else if below/above plot area and anchored in the margin, extend to opposite margin,
	// - otherwise give it the maximum potential margin-push value
	opts._maxWidth = Math.max(
	isLeftOfPlotArea ? ((isBeyondPlotAreaX && xanchor === 'left') ? gs.l + gs.w : hw) :
	isRightOfPlotArea ? ((isBeyondPlotAreaX && yanchor === 'right') ? gs.r + gs.w : hw) :
	gs.w,
	2 * textGap);
	var maxItemWidth = 0;
	var combinedItemWidth = 0;
	traces.each(function(d) {
	var w = d[0].width + textGap;
	maxItemWidth = Math.max(maxItemWidth, w);
	combinedItemWidth += w;
	});

	toggleRectWidth = null;
	var maxRowWidth = 0;

	if(isGrouped) {
	var maxGroupHeightInRow = 0;
	var groupOffsetX = 0;
	var groupOffsetY = 0;
	groups.each(function() {
	var maxWidthInGroup = 0;
	var offsetY = 0;
	d3.select(this).selectAll('g.traces').each(function(d) {
	var h = d[0].height;
	Drawing.setTranslate(this, 0, itemGap + bw + h / 2 + offsetY);
	offsetY += h;
	maxWidthInGroup = Math.max(maxWidthInGroup, textGap + d[0].width);
	});
	maxGroupHeightInRow = Math.max(maxGroupHeightInRow, offsetY);

	var next = maxWidthInGroup + itemGap;

	if((next + bw + groupOffsetX) > opts._maxWidth) {
	maxRowWidth = Math.max(maxRowWidth, groupOffsetX);
	groupOffsetX = 0;
	groupOffsetY += maxGroupHeightInRow + opts.tracegroupgap;
	maxGroupHeightInRow = offsetY;
	}

	Drawing.setTranslate(this, groupOffsetX, groupOffsetY);

	groupOffsetX += next;
	});

	opts._width = Math.max(maxRowWidth, groupOffsetX) + bw;
	opts._height = groupOffsetY + maxGroupHeightInRow + endPad;
	} else {
	var nTraces = traces.size();
	var oneRowLegend = (combinedItemWidth + bw2 + (nTraces - 1) * itemGap) < opts._maxWidth;

	var maxItemHeightInRow = 0;
	var offsetX = 0;
	var offsetY = 0;
	var rowWidth = 0;
	traces.each(function(d) {
	var h = d[0].height;
	var w = textGap + d[0].width;
	var next = (oneRowLegend ? w : maxItemWidth) + itemGap;

	if((next + bw + offsetX) > opts._maxWidth) {
	maxRowWidth = Math.max(maxRowWidth, rowWidth);
	offsetX = 0;
	offsetY += maxItemHeightInRow;
	opts._height += maxItemHeightInRow;
	maxItemHeightInRow = 0;
	}

	Drawing.setTranslate(this, bw + offsetX, itemGap + bw + h / 2 + offsetY);

	rowWidth = offsetX + w + itemGap;
	offsetX += next;
	maxItemHeightInRow = Math.max(maxItemHeightInRow, h);
	});

	if(oneRowLegend) {
	opts._width = offsetX + bw2;
	opts._height = maxItemHeightInRow + endPad;
	} else {
	opts._width = Math.max(maxRowWidth, rowWidth) + bw2;
	opts._height += maxItemHeightInRow + endPad;
	}
	}
	}

	opts._width = Math.ceil(opts._width);
	opts._height = Math.ceil(opts._height);

	opts._effHeight = Math.min(opts._height, opts._maxHeight);

	var edits = gd._context.edits;
	var isEditable = edits.legendText \|\| edits.legendPosition;
	traces.each(function(d) {
	var traceToggle = d3.select(this).select('.legendtoggle');
	var h = d[0].height;
	var w = isEditable ? textGap : (toggleRectWidth \|\| (textGap + d[0].width));
	if(!isVertical) w += itemGap / 2;
	Drawing.setRect(traceToggle, 0, -h / 2, w, h);
	});
	}

	// Position and size the legend
	var lxMin = 0;
	var lxMax = fullLayout.width;
	var lyMin = 0;
	var lyMax = fullLayout.height;

	computeLegendDimensions(gd, groups, traces);

	if(opts._height > lyMax) {
	// If the legend doesn't fit in the plot area,
	// do not expand the vertical margins.
	expandHorizontalMargin(gd);
	} else {
	expandMargin(gd);
	}

	// Scroll section must be executed after repositionLegend.
	// It requires the legend width, height, x and y to position the scrollbox
	// and these values are mutated in repositionLegend.
	var gs = fullLayout._size;
	var lx = gs.l + gs.w * opts.x;
	var ly = gs.t + gs.h * (1 - opts.y);

	if(Lib.isRightAnchor(opts)) {
	lx -= opts._width;
	} else if(Lib.isCenterAnchor(opts)) {
	lx -= opts._width / 2;
	}

	if(Lib.isBottomAnchor(opts)) {
	ly -= opts._height;
	} else if(Lib.isMiddleAnchor(opts)) {
	ly -= opts._height / 2;
	}

	// Make sure the legend left and right sides are visible
	var legendWidth = opts._width;
	var legendWidthMax = gs.w;

	if(legendWidth > legendWidthMax) {
	lx = gs.l;
	legendWidth = legendWidthMax;
	} else {
	if(lx + legendWidth > lxMax) lx = lxMax - legendWidth;
	if(lx < lxMin) lx = lxMin;
	legendWidth = Math.min(lxMax - lx, opts._width);
	}

	// Make sure the legend top and bottom are visible
	// (legends with a scroll bar are not allowed to stretch beyond the extended
	// margins)
	var legendHeight = opts._height;
	var legendHeightMax = gs.h;

	if(legendHeight > legendHeightMax) {
	ly = gs.t;
	legendHeight = legendHeightMax;
	} else {
	if(ly + legendHeight > lyMax) ly = lyMax - legendHeight;
	if(ly < lyMin) ly = lyMin;
	legendHeight = Math.min(lyMax - ly, opts._height);
	}

Multiple legend positioning and sizing fixes #4160

Multiple legend positioning and sizing fixes #4160

Conversation

etpinard commented Aug 30, 2019

nicolaskruchten commented Sep 3, 2019

etpinard commented Sep 3, 2019

antoinerg commented Sep 3, 2019

etpinard commented Sep 3, 2019

antoinerg commented Sep 3, 2019 • edited Loading

etpinard commented Sep 4, 2019 • edited Loading

antoinerg commented Sep 4, 2019

etpinard commented Sep 5, 2019

etpinard commented Sep 10, 2019

antoinerg commented Sep 23, 2019

etpinard commented Sep 23, 2019

destradafilm commented Feb 18, 2020 • edited Loading

antoinerg commented Sep 3, 2019 •

edited

Loading

etpinard commented Sep 4, 2019 •

edited

Loading

destradafilm commented Feb 18, 2020 •

edited

Loading