From 31ed9774cad75d3b59b48ed05bb0d59fa130a894 Mon Sep 17 00:00:00 2001 From: archmoj Date: Thu, 26 Sep 2019 10:17:12 -0400 Subject: [PATCH 1/4] handle text positions for out of range cases e.g. when transitioning with maxdepth --- src/traces/treemap/draw_ancestors.js | 13 ++------- src/traces/treemap/draw_descendants.js | 40 +++++++++++++++----------- test/jasmine/tests/treemap_test.js | 2 +- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/traces/treemap/draw_ancestors.js b/src/traces/treemap/draw_ancestors.js index 87b87451f36..6aac2206202 100644 --- a/src/traces/treemap/draw_ancestors.js +++ b/src/traces/treemap/draw_ancestors.js @@ -148,16 +148,9 @@ module.exports = function drawAncestors(gd, cd, entry, slices, opts) { .call(svgTextUtils.convertToTspans, gd); pt.textBB = Drawing.bBox(sliceText.node()); - pt.transform = toMoveInsideSlice( - pt.x0, - pt.x1, - pt.y0, - pt.y1, - pt.textBB, - { - onPathbar: true - } - ); + pt.transform = toMoveInsideSlice(pt.x0, pt.x1, pt.y0, pt.y1, pt.textBB, { + onPathbar: true + }); if(helpers.isOutsideText(trace, pt)) { // consider in/out diff font sizes diff --git a/src/traces/treemap/draw_descendants.js b/src/traces/treemap/draw_descendants.js index 88e481ae0ce..f1366628ec8 100644 --- a/src/traces/treemap/draw_descendants.js +++ b/src/traces/treemap/draw_descendants.js @@ -12,7 +12,7 @@ var d3 = require('d3'); var Lib = require('../../lib'); var Drawing = require('../../components/drawing'); var svgTextUtils = require('../../lib/svg_text_utils'); - +var TEXTPAD = require('../bar/constants').TEXTPAD; var partition = require('./partition'); var styleOne = require('./style').styleOne; var constants = require('./constants'); @@ -164,13 +164,28 @@ module.exports = function drawDescendants(gd, cd, entry, slices, opts) { s.attr('data-notex', 1); }); + var _x0 = pt.x0; + var _x1 = pt.x1; + var _y0 = pt.y0; + var _y1 = pt.y1; + var tx; - if(isHeader) { - if(noRoomForHeader) return; + if(_x0 < _x1 && _y0 < _y1) { + if(isHeader) { + if(noRoomForHeader) return; - tx = helpers.getPtLabel(pt); - } else { - tx = formatSliceLabel(pt, entry, trace, cd, fullLayout) || ' '; + tx = helpers.getPtLabel(pt); + } else { + tx = formatSliceLabel(pt, entry, trace, cd, fullLayout) || ' '; + } + } else { // handle text positions for out of range cases e.g. with maxdepth + tx = ' '; + + var expand = 2 * TEXTPAD; + _x0 -= expand; + _x1 += expand; + _y0 -= expand; + _y1 += expand; } sliceText.text(tx) @@ -180,16 +195,9 @@ module.exports = function drawDescendants(gd, cd, entry, slices, opts) { .call(svgTextUtils.convertToTspans, gd); pt.textBB = Drawing.bBox(sliceText.node()); - pt.transform = toMoveInsideSlice( - pt.x0, - pt.x1, - pt.y0, - pt.y1, - pt.textBB, - { - isHeader: isHeader - } - ); + pt.transform = toMoveInsideSlice(_x0, _x1, _y0, _y1, pt.textBB, { + isHeader: isHeader + }); if(helpers.isOutsideText(trace, pt)) { // consider in/out diff font sizes diff --git a/test/jasmine/tests/treemap_test.js b/test/jasmine/tests/treemap_test.js index b812e58b282..5fd78ec537d 100644 --- a/test/jasmine/tests/treemap_test.js +++ b/test/jasmine/tests/treemap_test.js @@ -1225,7 +1225,7 @@ describe('Test treemap tweening:', function() { 'M284.375,188.5L548.375,188.5L548.375,308.5L284.375,308.5Z' ); _assert('move B text to new position', 'transform', 'B', [220.25126, 0]); - _assert('enter b text to new position', 'transform', 'b', [287.375195, 5]); + _assert('enter b text to new position', 'transform', 'b', [284.66071, 35714285714286]); }) .catch(failTest) .then(done); From a1a2a2b298fb2699dfd8f5fd4ba81749041d28a3 Mon Sep 17 00:00:00 2001 From: archmoj Date: Thu, 26 Sep 2019 12:47:57 -0400 Subject: [PATCH 2/4] rework text transform to move inside slice function - cast _text to pt for easier debugging - pass pt object to the function instead of separated arguments - do not return in the middle of draw functions - expand void area for blank text --- src/traces/treemap/draw_ancestors.js | 8 ++-- src/traces/treemap/draw_descendants.js | 40 ++++++------------ src/traces/treemap/plot.js | 57 +++++++++++++++----------- test/jasmine/tests/treemap_test.js | 6 +-- 4 files changed, 53 insertions(+), 58 deletions(-) diff --git a/src/traces/treemap/draw_ancestors.js b/src/traces/treemap/draw_ancestors.js index 6aac2206202..764a4dbf6b2 100644 --- a/src/traces/treemap/draw_ancestors.js +++ b/src/traces/treemap/draw_ancestors.js @@ -132,6 +132,8 @@ module.exports = function drawAncestors(gd, cd, entry, slices, opts) { hovered: false }); + pt._text = (helpers.getPtLabel(pt) || '').split('
').join(' ') || ''; + var sliceTextGroup = Lib.ensureSingle(sliceTop, 'g', 'slicetext'); var sliceText = Lib.ensureSingle(sliceTextGroup, 'text', '', function(s) { // prohibit tex interpretation until we can handle @@ -139,16 +141,14 @@ module.exports = function drawAncestors(gd, cd, entry, slices, opts) { s.attr('data-notex', 1); }); - var tx = (helpers.getPtLabel(pt) || ' ').split('
').join(' '); - - sliceText.text(tx) + sliceText.text(pt._text) .classed('slicetext', true) .attr('text-anchor', 'start') .call(Drawing.font, helpers.determineTextFont(trace, pt, fullLayout.font, trace.pathdir)) .call(svgTextUtils.convertToTspans, gd); pt.textBB = Drawing.bBox(sliceText.node()); - pt.transform = toMoveInsideSlice(pt.x0, pt.x1, pt.y0, pt.y1, pt.textBB, { + pt.transform = toMoveInsideSlice(pt, { onPathbar: true }); diff --git a/src/traces/treemap/draw_descendants.js b/src/traces/treemap/draw_descendants.js index f1366628ec8..076db18c8fb 100644 --- a/src/traces/treemap/draw_descendants.js +++ b/src/traces/treemap/draw_descendants.js @@ -12,7 +12,7 @@ var d3 = require('d3'); var Lib = require('../../lib'); var Drawing = require('../../components/drawing'); var svgTextUtils = require('../../lib/svg_text_utils'); -var TEXTPAD = require('../bar/constants').TEXTPAD; + var partition = require('./partition'); var styleOne = require('./style').styleOne; var constants = require('./constants'); @@ -157,6 +157,16 @@ module.exports = function drawDescendants(gd, cd, entry, slices, opts) { hovered: false }); + if(pt.x0 === pt.x1 && pt.y0 === pt.y1) { + pt._text = ' '; // use one space character instead of a blank string in this case + } else { + if(isHeader) { + pt._text = noRoomForHeader ? '' : helpers.getPtLabel(pt) || ''; + } else { + pt._text = formatSliceLabel(pt, entry, trace, cd, fullLayout) || ''; + } + } + var sliceTextGroup = Lib.ensureSingle(sliceTop, 'g', 'slicetext'); var sliceText = Lib.ensureSingle(sliceTextGroup, 'text', '', function(s) { // prohibit tex interpretation until we can handle @@ -164,38 +174,14 @@ module.exports = function drawDescendants(gd, cd, entry, slices, opts) { s.attr('data-notex', 1); }); - var _x0 = pt.x0; - var _x1 = pt.x1; - var _y0 = pt.y0; - var _y1 = pt.y1; - - var tx; - if(_x0 < _x1 && _y0 < _y1) { - if(isHeader) { - if(noRoomForHeader) return; - - tx = helpers.getPtLabel(pt); - } else { - tx = formatSliceLabel(pt, entry, trace, cd, fullLayout) || ' '; - } - } else { // handle text positions for out of range cases e.g. with maxdepth - tx = ' '; - - var expand = 2 * TEXTPAD; - _x0 -= expand; - _x1 += expand; - _y0 -= expand; - _y1 += expand; - } - - sliceText.text(tx) + sliceText.text(pt._text) .classed('slicetext', true) .attr('text-anchor', hasRight ? 'end' : (hasLeft || isHeader) ? 'start' : 'middle') .call(Drawing.font, helpers.determineTextFont(trace, pt, fullLayout.font)) .call(svgTextUtils.convertToTspans, gd); pt.textBB = Drawing.bBox(sliceText.node()); - pt.transform = toMoveInsideSlice(_x0, _x1, _y0, _y1, pt.textBB, { + pt.transform = toMoveInsideSlice(pt, { isHeader: isHeader }); diff --git a/src/traces/treemap/plot.js b/src/traces/treemap/plot.js index a72f03d6c8c..4406b17cb5d 100644 --- a/src/traces/treemap/plot.js +++ b/src/traces/treemap/plot.js @@ -162,14 +162,14 @@ function plotOne(gd, cd, element, transitionOpts) { var cenX = -vpw / 2 + gs.l + gs.w * (domain.x[1] + domain.x[0]) / 2; var cenY = -vph / 2 + gs.t + gs.h * (1 - (domain.y[1] + domain.y[0]) / 2); - var viewMapX = function(x) { return cenX + x; }; - var viewMapY = function(y) { return cenY + y; }; + var viewMapX = function(x) { return cenX + (x || 0); }; + var viewMapY = function(y) { return cenY + (y || 0); }; var barY0 = viewMapY(0); var barX0 = viewMapX(0); - var viewBarX = function(x) { return barX0 + x; }; - var viewBarY = function(y) { return barY0 + y; }; + var viewBarX = function(x) { return barX0 + (x || 0); }; + var viewBarY = function(y) { return barY0 + (y || 0); }; function pos(x, y) { return x + ',' + y; @@ -273,7 +273,23 @@ function plotOne(gd, cd, element, transitionOpts) { ); }; - var toMoveInsideSlice = function(x0, x1, y0, y1, textBB, opts) { + var toMoveInsideSlice = function(pt, opts) { + var x0 = pt.x0; + var x1 = pt.x1; + var y0 = pt.y0; + var y1 = pt.y1; + var textBB = pt.textBB; + + if(x0 === x1) { + x0 -= TEXTPAD; + x1 += TEXTPAD; + } + + if(y0 === y1) { + y0 -= TEXTPAD; + y1 += TEXTPAD; + } + var hasFlag = function(f) { return trace.textposition.indexOf(f) !== -1; }; var hasBottom = hasFlag('bottom'); @@ -327,20 +343,13 @@ function plotOne(gd, cd, element, transitionOpts) { else if(offsetDir === 'right') transform.targetX += deltaX; } - transform.targetX = viewMapX(transform.targetX); - transform.targetY = viewMapY(transform.targetY); - - if(isNaN(transform.targetX) || isNaN(transform.targetY)) { - return {}; - } - return { scale: transform.scale, rotate: transform.rotate, textX: transform.textX, textY: transform.textY, - targetX: transform.targetX, - targetY: transform.targetY + targetX: viewMapX(transform.targetX), + targetY: viewMapY(transform.targetY) }; }; @@ -428,16 +437,16 @@ function plotOne(gd, cd, element, transitionOpts) { var origin = getOrigin(pt, onPathbar, refRect, size); Lib.extendFlat(prev, { - transform: toMoveInsideSlice( - origin.x0, - origin.x1, - origin.y0, - origin.y1, - pt.textBB, - { - isHeader: helpers.isHeader(pt, trace) - } - ) + transform: toMoveInsideSlice({ + x0: origin.x0, + x1: origin.x1, + y0: origin.y0, + y1: origin.y1, + textBB: pt.textBB, + _text: pt._text + }, { + isHeader: helpers.isHeader(pt, trace) + }) }); if(prev0) { diff --git a/test/jasmine/tests/treemap_test.js b/test/jasmine/tests/treemap_test.js index 5fd78ec537d..57b609bf1d9 100644 --- a/test/jasmine/tests/treemap_test.js +++ b/test/jasmine/tests/treemap_test.js @@ -1066,7 +1066,7 @@ describe('Test treemap restyle:', function() { .then(_restyle({textinfo: 'value'})) .then(_assert('show input values', ['Root', 'B', '1', '3'])) .then(_restyle({textinfo: 'none'})) - .then(_assert('no textinfo', ['Root', 'B', ' ', ' '])) // N.B. replaced empty string with space character for better transitions + .then(_assert('no textinfo', ['Root', 'B', '', ''])) .then(_restyle({textinfo: 'label+text+value'})) .then(_assert('show everything', ['Root', 'B', 'A\n1\nnode1', 'b\n3\nnode3'])) .then(_restyle({textinfo: null})) @@ -1127,7 +1127,7 @@ describe('Test treemap tweening:', function() { if(attrName === 'transform') { var fake = {attr: function() { return actual; }}; var xy = Drawing.getTranslate(fake); - expect([xy.x, xy.y]).toBeWithinArray(exp, 1, msg2); + expect([xy.x, xy.y]).toBeWithinArray(exp, 2, msg2); } else { // we could maybe to bring in: // https://github.com/hughsk/svg-path-parser @@ -1225,7 +1225,7 @@ describe('Test treemap tweening:', function() { 'M284.375,188.5L548.375,188.5L548.375,308.5L284.375,308.5Z' ); _assert('move B text to new position', 'transform', 'B', [220.25126, 0]); - _assert('enter b text to new position', 'transform', 'b', [284.66071, 35714285714286]); + _assert('enter b text to new position', 'transform', 'b', [286.16071428571433, 35714285714286]); }) .catch(failTest) .then(done); From 212b144766b178af615157e73247c6844e18872f Mon Sep 17 00:00:00 2001 From: archmoj Date: Fri, 27 Sep 2019 08:28:51 -0400 Subject: [PATCH 3/4] using one space character instead of a blank string to avoid jumps during treemap transition --- src/traces/treemap/draw_ancestors.js | 2 +- src/traces/treemap/draw_descendants.js | 6 +++--- test/jasmine/tests/treemap_test.js | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/traces/treemap/draw_ancestors.js b/src/traces/treemap/draw_ancestors.js index 764a4dbf6b2..41f2dfd6637 100644 --- a/src/traces/treemap/draw_ancestors.js +++ b/src/traces/treemap/draw_ancestors.js @@ -141,7 +141,7 @@ module.exports = function drawAncestors(gd, cd, entry, slices, opts) { s.attr('data-notex', 1); }); - sliceText.text(pt._text) + sliceText.text(pt._text || ' ') // use one space character instead of a blank string to avoid jumps during transition .classed('slicetext', true) .attr('text-anchor', 'start') .call(Drawing.font, helpers.determineTextFont(trace, pt, fullLayout.font, trace.pathdir)) diff --git a/src/traces/treemap/draw_descendants.js b/src/traces/treemap/draw_descendants.js index 076db18c8fb..6bd1bd5989e 100644 --- a/src/traces/treemap/draw_descendants.js +++ b/src/traces/treemap/draw_descendants.js @@ -157,8 +157,8 @@ module.exports = function drawDescendants(gd, cd, entry, slices, opts) { hovered: false }); - if(pt.x0 === pt.x1 && pt.y0 === pt.y1) { - pt._text = ' '; // use one space character instead of a blank string in this case + if(pt.x0 === pt.x1 || pt.y0 === pt.y1) { + pt._text = ''; } else { if(isHeader) { pt._text = noRoomForHeader ? '' : helpers.getPtLabel(pt) || ''; @@ -174,7 +174,7 @@ module.exports = function drawDescendants(gd, cd, entry, slices, opts) { s.attr('data-notex', 1); }); - sliceText.text(pt._text) + sliceText.text(pt._text || ' ') // use one space character instead of a blank string to avoid jumps during transition .classed('slicetext', true) .attr('text-anchor', hasRight ? 'end' : (hasLeft || isHeader) ? 'start' : 'middle') .call(Drawing.font, helpers.determineTextFont(trace, pt, fullLayout.font)) diff --git a/test/jasmine/tests/treemap_test.js b/test/jasmine/tests/treemap_test.js index 57b609bf1d9..1fb4ed4d2e4 100644 --- a/test/jasmine/tests/treemap_test.js +++ b/test/jasmine/tests/treemap_test.js @@ -1066,7 +1066,7 @@ describe('Test treemap restyle:', function() { .then(_restyle({textinfo: 'value'})) .then(_assert('show input values', ['Root', 'B', '1', '3'])) .then(_restyle({textinfo: 'none'})) - .then(_assert('no textinfo', ['Root', 'B', '', ''])) + .then(_assert('no textinfo', ['Root', 'B', ' ', ' '])) // use one space character instead of a blank string to avoid jumps during transition .then(_restyle({textinfo: 'label+text+value'})) .then(_assert('show everything', ['Root', 'B', 'A\n1\nnode1', 'b\n3\nnode3'])) .then(_restyle({textinfo: null})) From 6d6f946d0cf4c3b6d27a7529e422939401e53907 Mon Sep 17 00:00:00 2001 From: archmoj Date: Fri, 27 Sep 2019 20:31:14 -0400 Subject: [PATCH 4/4] revert handling of NaN cases to before this PR - add extra check to avoid header jumps --- src/traces/treemap/plot.js | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/traces/treemap/plot.js b/src/traces/treemap/plot.js index 4406b17cb5d..df4ee7ad923 100644 --- a/src/traces/treemap/plot.js +++ b/src/traces/treemap/plot.js @@ -162,14 +162,14 @@ function plotOne(gd, cd, element, transitionOpts) { var cenX = -vpw / 2 + gs.l + gs.w * (domain.x[1] + domain.x[0]) / 2; var cenY = -vph / 2 + gs.t + gs.h * (1 - (domain.y[1] + domain.y[0]) / 2); - var viewMapX = function(x) { return cenX + (x || 0); }; - var viewMapY = function(y) { return cenY + (y || 0); }; + var viewMapX = function(x) { return cenX + x; }; + var viewMapY = function(y) { return cenY + y; }; var barY0 = viewMapY(0); var barX0 = viewMapX(0); - var viewBarX = function(x) { return barX0 + (x || 0); }; - var viewBarY = function(y) { return barY0 + (y || 0); }; + var viewBarX = function(x) { return barX0 + x; }; + var viewBarY = function(y) { return barY0 + y; }; function pos(x, y) { return x + ',' + y; @@ -315,6 +315,11 @@ function plotOne(gd, cd, element, transitionOpts) { if(opts.isHeader) { x0 += pad.l - TEXTPAD; x1 -= pad.r - TEXTPAD; + if(x0 >= x1) { + var mid = (x0 + x1) / 2; + x0 = mid - TEXTPAD; + x1 = mid + TEXTPAD; + } // limit the drawing area for headers var limY; @@ -343,13 +348,20 @@ function plotOne(gd, cd, element, transitionOpts) { else if(offsetDir === 'right') transform.targetX += deltaX; } + transform.targetX = viewMapX(transform.targetX); + transform.targetY = viewMapY(transform.targetY); + + if(isNaN(transform.targetX) || isNaN(transform.targetY)) { + return {}; + } + return { scale: transform.scale, rotate: transform.rotate, textX: transform.textX, textY: transform.textY, - targetX: viewMapX(transform.targetX), - targetY: viewMapY(transform.targetY) + targetX: transform.targetX, + targetY: transform.targetY }; };