Merge pull request #417 from n-riesco/issue-384-legend-negative-y-no-strict-margins

mdtusz · mdtusz · commit b9a17056c774 · 2016-04-18T13:58:22.000-04:00
Fix issue #384 (invisible legends when y is negative)
diff --git a/src/components/legend/draw.js b/src/components/legend/draw.js
@@ -173,7 +173,18 @@ module.exports = function draw(gd) {
         });
 
     // Position and size the legend
-    repositionLegend(gd, traces);
+    var lyMin = 0,
+        lyMax = fullLayout.height;
+
+    computeLegendDimensions(gd, 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
@@ -185,27 +196,41 @@ module.exports = function draw(gd) {
     if(anchorUtils.isRightAnchor(opts)) {
         lx -= opts.width;
     }
-    if(anchorUtils.isCenterAnchor(opts)) {
+    else if(anchorUtils.isCenterAnchor(opts)) {
         lx -= opts.width / 2;
     }
 
     if(anchorUtils.isBottomAnchor(opts)) {
         ly -= opts.height;
     }
-    if(anchorUtils.isMiddleAnchor(opts)) {
+    else if(anchorUtils.isMiddleAnchor(opts)) {
         ly -= opts.height / 2;
     }
 
+    // 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,
+        legendHeightMax = gs.h;
+
+    if(legendHeight > legendHeightMax) {
+        ly = gs.t;
+        legendHeight = legendHeightMax;
+    }
+    else {
+        if(ly > lyMax) ly = lyMax - legendHeight;
+        if(ly < lyMin) ly = lyMin;
+        legendHeight = Math.min(lyMax - ly, opts.height);
+    }
+
     // Deal with scrolling
-    var plotHeight = fullLayout.height - fullLayout.margin.b,
-        scrollheight = Math.min(plotHeight - ly, opts.height),
-        scrollPosition = scrollBox.attr('data-scroll') ? scrollBox.attr('data-scroll') : 0;
+    var scrollPosition = scrollBox.attr('data-scroll') || 0;
 
     scrollBox.attr('transform', 'translate(0, ' + scrollPosition + ')');
 
     bg.attr({
         width: opts.width - 2 * opts.borderwidth,
-        height: scrollheight - 2 * opts.borderwidth,
+        height: legendHeight - 2 * opts.borderwidth,
         x: opts.borderwidth,
         y: opts.borderwidth
     });
@@ -214,71 +239,81 @@ module.exports = function draw(gd) {
 
     clipPath.select('rect').attr({
         width: opts.width,
-        height: scrollheight,
+        height: legendHeight,
         x: 0,
         y: 0
     });
 
     legend.call(Drawing.setClipUrl, clipId);
 
     // If scrollbar should be shown.
-    if(opts.height - scrollheight > 0 && !gd._context.staticPlot) {
+    if(opts.height - legendHeight > 0 && !gd._context.staticPlot) {
 
+        // increase the background and clip-path width
+        // by the scrollbar width and margin
         bg.attr({
-            width: opts.width - 2 * opts.borderwidth + constants.scrollBarWidth
+            width: opts.width -
+                2 * opts.borderwidth +
+                constants.scrollBarWidth +
+                constants.scrollBarMargin
         });
 
-        clipPath.attr({
-            width: opts.width + constants.scrollBarWidth
+        clipPath.select('rect').attr({
+            width: opts.width +
+                constants.scrollBarWidth +
+                constants.scrollBarMargin
         });
 
         if(gd.firstRender) {
             // Move scrollbar to starting position
-            scrollBar.call(
-                Drawing.setRect,
-                opts.width - (constants.scrollBarWidth + constants.scrollBarMargin),
-                constants.scrollBarMargin,
-                constants.scrollBarWidth,
-                constants.scrollBarHeight
-            );
-            scrollBox.attr('data-scroll',0);
+            scrollHandler(constants.scrollBarMargin, 0);
         }
 
-        scrollHandler(0,scrollheight);
+        var scrollBarYMax = legendHeight -
+                constants.scrollBarHeight -
+                2 * constants.scrollBarMargin,
+            scrollBoxYMax = opts.height - legendHeight,
+            scrollBarY = constants.scrollBarMargin,
+            scrollBoxY = 0;
 
-        legend.on('wheel',null);
+        scrollHandler(scrollBarY, scrollBoxY);
 
+        legend.on('wheel',null);
         legend.on('wheel', function() {
-            var e = d3.event;
-            e.preventDefault();
-            scrollHandler(e.deltaY / 20, scrollheight);
+            scrollBoxY = Lib.constrain(
+                scrollBox.attr('data-scroll') -
+                    d3.event.deltaY / scrollBarYMax * scrollBoxYMax,
+                -scrollBoxYMax, 0);
+            scrollBarY = constants.scrollBarMargin -
+                scrollBoxY / scrollBoxYMax * scrollBarYMax;
+            scrollHandler(scrollBarY, scrollBoxY);
+            d3.event.preventDefault();
         });
 
         scrollBar.on('.drag',null);
         scrollBox.on('.drag',null);
-        var drag = d3.behavior.drag()
-            .on('drag', function() {
-                scrollHandler(d3.event.dy, scrollheight);
-            });
+        var drag = d3.behavior.drag().on('drag', function() {
+            scrollBarY = Lib.constrain(
+                d3.event.y - constants.scrollBarHeight / 2,
+                constants.scrollBarMargin,
+                constants.scrollBarMargin + scrollBarYMax);
+            scrollBoxY = - (scrollBarY - constants.scrollBarMargin) /
+                scrollBarYMax * scrollBoxYMax;
+            scrollHandler(scrollBarY, scrollBoxY);
+        });
 
         scrollBar.call(drag);
         scrollBox.call(drag);
 
     }
 
 
-    function scrollHandler(delta, scrollheight) {
-
-        var scrollBarTrack = scrollheight - constants.scrollBarHeight - 2 * constants.scrollBarMargin,
-            translateY = scrollBox.attr('data-scroll'),
-            scrollBoxY = Lib.constrain(translateY - delta, scrollheight-opts.height, 0),
-            scrollBarY = -scrollBoxY / (opts.height - scrollheight) * scrollBarTrack + constants.scrollBarMargin;
-
+    function scrollHandler(scrollBarY, scrollBoxY) {
         scrollBox.attr('data-scroll', scrollBoxY);
         scrollBox.attr('transform', 'translate(0, ' + scrollBoxY + ')');
         scrollBar.call(
             Drawing.setRect,
-            opts.width - (constants.scrollBarWidth + constants.scrollBarMargin),
+            opts.width,
             scrollBarY,
             constants.scrollBarWidth,
             constants.scrollBarHeight
@@ -348,7 +383,10 @@ function drawTexts(context, gd, d, i, traces) {
 
     function textLayout(s) {
         Plotly.util.convertToTspans(s, function() {
-            if(gd.firstRender) repositionLegend(gd, traces);
+            if(gd.firstRender) {
+                computeLegendDimensions(gd, traces);
+                expandMargin(gd);
+            }
         });
         s.selectAll('tspan.line').attr({x: s.attr('x')});
     }
@@ -367,9 +405,8 @@ function drawTexts(context, gd, d, i, traces) {
     else text.call(textLayout);
 }
 
-function repositionLegend(gd, traces) {
+function computeLegendDimensions(gd, traces) {
     var fullLayout = gd._fullLayout,
-        gs = fullLayout._size,
         opts = fullLayout.legend,
         borderwidth = opts.borderwidth;
 
@@ -421,7 +458,6 @@ function repositionLegend(gd, traces) {
         opts.width = Math.max(opts.width, tWidth || 0);
     });
 
-
     opts.width += 45 + borderwidth * 2;
     opts.height += 10 + borderwidth * 2;
 
@@ -432,41 +468,31 @@ function repositionLegend(gd, traces) {
     traces.selectAll('.legendtoggle')
         .attr('width', (gd._context.editable ? 0 : opts.width) + 40);
 
-    // now position the legend. for both x,y the positions are recorded as
-    // fractions of the plot area (left, bottom = 0,0). Outside the plot
-    // area is allowed but position will be clipped to the page.
-    // values <1/3 align the low side at that fraction, 1/3-2/3 align the
-    // center at that fraction, >2/3 align the right at that fraction
+    // make sure we're only getting full pixels
+    opts.width = Math.ceil(opts.width);
+    opts.height = Math.ceil(opts.height);
+}
 
-    var lx = gs.l + gs.w * opts.x,
-        ly = gs.t + gs.h * (1-opts.y);
+function expandMargin(gd) {
+    var fullLayout = gd._fullLayout,
+        opts = fullLayout.legend;
 
     var xanchor = 'left';
     if(anchorUtils.isRightAnchor(opts)) {
-        lx -= opts.width;
         xanchor = 'right';
     }
-    if(anchorUtils.isCenterAnchor(opts)) {
-        lx -= opts.width / 2;
+    else if(anchorUtils.isCenterAnchor(opts)) {
         xanchor = 'center';
     }
 
     var yanchor = 'top';
     if(anchorUtils.isBottomAnchor(opts)) {
-        ly -= opts.height;
         yanchor = 'bottom';
     }
-    if(anchorUtils.isMiddleAnchor(opts)) {
-        ly -= opts.height / 2;
+    else if(anchorUtils.isMiddleAnchor(opts)) {
         yanchor = 'middle';
     }
 
-    // make sure we're only getting full pixels
-    opts.width = Math.ceil(opts.width);
-    opts.height = Math.ceil(opts.height);
-    lx = Math.round(lx);
-    ly = Math.round(ly);
-
     // lastly check if the margin auto-expand has changed
     Plots.autoMargin(gd, 'legend', {
         x: opts.x,
@@ -477,3 +503,26 @@ function repositionLegend(gd, traces) {
         t: opts.height * ({bottom: 1, middle: 0.5}[yanchor] || 0)
     });
 }
+
+function expandHorizontalMargin(gd) {
+    var fullLayout = gd._fullLayout,
+        opts = fullLayout.legend;
+
+    var xanchor = 'left';
+    if(anchorUtils.isRightAnchor(opts)) {
+        xanchor = 'right';
+    }
+    else if(anchorUtils.isCenterAnchor(opts)) {
+        xanchor = 'center';
+    }
+
+    // lastly check if the margin auto-expand has changed
+    Plots.autoMargin(gd, 'legend', {
+        x: opts.x,
+        y: 0.5,
+        l: opts.width * ({right: 1, center: 0.5}[xanchor] || 0),
+        r: opts.width * ({left: 1, center: 0.5}[xanchor] || 0),
+        b: 0,
+        t: 0
+    });
+}
diff --git a/test/image/baselines/legend_negative_y.png b/test/image/baselines/legend_negative_y.png
diff --git a/test/image/mocks/legend_negative_y.json b/test/image/mocks/legend_negative_y.json
@@ -0,0 +1,62 @@
+{
+   "data":[
+      {
+         "x":[
+            0,
+            1,
+            2,
+            3,
+            4,
+            5,
+            6,
+            7,
+            8
+         ],
+         "y":[
+            0,
+            3,
+            6,
+            4,
+            5,
+            2,
+            3,
+            5,
+            4
+         ],
+         "type":"scatter"
+      },
+      {
+         "x":[
+            0,
+            1,
+            2,
+            3,
+            4,
+            5,
+            6,
+            7,
+            8
+         ],
+         "y":[
+            0,
+            4,
+            7,
+            8,
+            3,
+            6,
+            3,
+            3,
+            4
+         ],
+         "type":"scatter"
+      }
+   ],
+   "layout":{
+      "showlegend":true,
+      "legend":{
+         "x":1,
+         "y":-1,
+         "xanchor":"left"
+      }
+   }
+}
diff --git a/test/jasmine/tests/legend_scroll_test.js b/test/jasmine/tests/legend_scroll_test.js
@@ -1,5 +1,6 @@
 var Plotly = require('@lib/index');
 var Lib = require('@src/lib');
+var constants = require('@src/components/legend/constants');
 
 var createGraph = require('../assets/create_graph_div');
 var destroyGraph = require('../assets/destroy_graph_div');
@@ -55,14 +56,23 @@ describe('The legend', function() {
         });
 
         it('should scroll when there\'s a wheel event', function() {
-            var scrollBox = legend.getElementsByClassName('scrollbox')[0];
-
-            legend.dispatchEvent(scrollTo(100));
-
-            // Compare against -5 because of a scroll factor of 20
-            // ( 100 / 20 === 5 )
-            expect(scrollBox.getAttribute('transform')).toBe('translate(0, -5)');
-            expect(scrollBox.getAttribute('data-scroll')).toBe('-5');
+            var scrollBox = legend.getElementsByClassName('scrollbox')[0],
+                legendHeight = getBBox(legend).height,
+                scrollBoxYMax = gd._fullLayout.legend.height - legendHeight,
+                scrollBarYMax = legendHeight -
+                    constants.scrollBarHeight -
+                    2 * constants.scrollBarMargin,
+                initialDataScroll = scrollBox.getAttribute('data-scroll'),
+                wheelDeltaY = 100,
+                finalDataScroll = '' + Lib.constrain(initialDataScroll -
+                    wheelDeltaY / scrollBarYMax * scrollBoxYMax,
+                    -scrollBoxYMax, 0);
+
+            legend.dispatchEvent(scrollTo(wheelDeltaY));
+
+            expect(scrollBox.getAttribute('data-scroll')).toBe(finalDataScroll);
+            expect(scrollBox.getAttribute('transform')).toBe(
+                'translate(0, ' + finalDataScroll + ')');
         });
 
         it('should constrain scrolling to the contents', function() {
