fix #4132 - fix horizontal legend dimension computations

- 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

Author: etpinard

src/components/legend/draw.js

@@ -538,64 +538,55 @@ function computeLegendDimensions(gd, groups, traces) {
             combinedItemWidth += w;
         });
 
+        toggleRectWidth = null;
+        var maxRowWidth = 0;
+
         if(isGrouped) {
-            var groupData = groups.data();
-            var i;
+            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 maxGroupHeight = 0;
-            for(i = 0; i < groupData.length; i++) {
-                var groupHeight = groupData[i].reduce(function(a, b) { return a + b[0].height; }, 0);
-                maxGroupHeight = Math.max(maxGroupHeight, groupHeight);
-            }
+                var next = maxWidthInGroup + itemGap;
 
-            var groupXOffsets = [opts._width];
-            var groupYOffsets = [];
-            var rowNum = 0;
-            for(i = 0; i < groupData.length; i++) {
-                if((opts._width + itemGap + maxItemWidth + bw) > opts._maxWidth) {
-                    groupXOffsets[groupXOffsets.length - 1] = groupXOffsets[0];
-                    opts._width = maxItemWidth + itemGap;
-                    rowNum++;
-                } else {
-                    opts._width += maxItemWidth + itemGap;
+                if((next + bw + groupOffsetX) > opts._maxWidth) {
+                    maxRowWidth = Math.max(maxRowWidth, groupOffsetX);
+                    groupOffsetX = 0;
+                    groupOffsetY += maxGroupHeightInRow + opts.tracegroupgap;
+                    maxGroupHeightInRow = offsetY;
                 }
 
-                groupXOffsets.push(opts._width);
-                groupYOffsets.push(rowNum * maxGroupHeight + (rowNum > 0 ? opts.tracegroupgap : 0));
-            }
-
-            groups.each(function(d, i) {
-                Drawing.setTranslate(this, groupXOffsets[i], groupYOffsets[i]);
-            });
+                Drawing.setTranslate(this, groupOffsetX, groupOffsetY);
 
-            groups.each(function() {
-                var group = d3.select(this);
-                var groupTraces = group.selectAll('g.traces');
-                var groupHeight = 0;
-
-                groupTraces.each(function(d) {
-                    var h = d[0].height;
-                    Drawing.setTranslate(this, 0, itemGap + bw + groupHeight + h / 2);
-                    groupHeight += h;
-                });
+                groupOffsetX += next;
             });
 
-            opts._height = groupYOffsets[groupYOffsets.length - 1] + maxGroupHeight + endPad;
-            opts._width = Math.max.apply(null, groupXOffsets) + maxItemWidth + textGap + bw2;
-            toggleRectWidth = maxItemWidth;
+            opts._width = Math.max(maxRowWidth, groupOffsetX) + bw;
+            opts._height = groupOffsetY + maxGroupHeightInRow + endPad;
         } else {
-            var oneRowLegend = (combinedItemWidth + bw2 + (traces.size() - 1) * itemGap) < opts._maxWidth;
+            var nTraces = traces.size();
+            var oneRowLegend = (combinedItemWidth + bw2 + (nTraces - 1) * itemGap) < opts._maxWidth;
 
-            var maxRowWidth = 0;
             var maxItemHeightInRow = 0;
             var offsetX = 0;
             var offsetY = 0;
+            var rowWidth = 0;
             traces.each(function(d) {
                 var h = d[0].height;
-                var next = (oneRowLegend ? textGap + d[0].width : maxItemWidth) + itemGap;
+                var w = textGap + d[0].width;
+                var next = (oneRowLegend ? w : maxItemWidth) + itemGap;
 
                 if((next + bw + offsetX) > opts._maxWidth) {
-                    maxRowWidth = Math.max(maxRowWidth, offsetX);
+                    maxRowWidth = Math.max(maxRowWidth, rowWidth);
                     offsetX = 0;
                     offsetY += maxItemHeightInRow;
                     opts._height += maxItemHeightInRow;
@@ -604,18 +595,17 @@ function computeLegendDimensions(gd, groups, traces) {
 
                 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;
-                toggleRectWidth = null;
             } else {
-                opts._width = Math.max(maxRowWidth, offsetX) + bw;
+                opts._width = Math.max(maxRowWidth, rowWidth) + bw2;
                 opts._height += maxItemHeightInRow + endPad;
-                toggleRectWidth = maxItemWidth;
             }
         }
     }

test/image/baselines/legend_horizontal_bg_fit.png

@@ -719,15 +719,15 @@ describe('legend relayout update', function() {
             var mockCopy = Lib.extendDeep({}, mock);
             Plotly.newPlot(gd, mockCopy)
             .then(function() {
-                expect(gd._fullLayout._size.b).toBe(130);
+                expect(gd._fullLayout._size.b).toBe(113);
                 return Plotly.relayout(gd, 'legend.tracegroupgap', 70);
             })
             .then(function() {
-                expect(gd._fullLayout._size.b).toBe(185);
+                expect(gd._fullLayout._size.b).toBe(167);
                 return Plotly.relayout(gd, 'legend.tracegroupgap', 10);
             })
             .then(function() {
-                expect(gd._fullLayout._size.b).toBe(130);
+                expect(gd._fullLayout._size.b).toBe(113);
             })
             .catch(failTest)
             .then(done);