From 83b3d6493f519616f4d035308a5413853adf81d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 12 May 2017 15:03:43 -0400 Subject: [PATCH 1/8] Revert "Cache DOM nodes and selected d3 node" This reverts commit 29abb5d8ec3dea92cc71f438fffa6785463d1fb1. --- src/components/drawing/index.js | 25 ++++++++++--------------- src/lib/svg_text_utils.js | 4 +--- 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/src/components/drawing/index.js b/src/components/drawing/index.js index a1e4eb011be..5eb5be186fa 100644 --- a/src/components/drawing/index.js +++ b/src/components/drawing/index.js @@ -589,7 +589,6 @@ drawing.makeTester = function(gd) { // always returns a copy of the bbox, so the caller can modify it safely var savedBBoxes = [], maxSavedBBoxes = 10000; - drawing.bBox = function(node) { // cache elements we've already measured so we don't have to // remeasure the same thing many times @@ -598,14 +597,12 @@ drawing.bBox = function(node) { return Lib.extendFlat({}, savedBBoxes[saveNum.value]); } - if(!drawing.test3) { - drawing.test3 = d3.select('#js-plotly-tester'); - drawing.tester = drawing.test3.node(); - } + var test3 = d3.select('#js-plotly-tester'), + tester = test3.node(); // copy the node to test into the tester var testNode = node.cloneNode(true); - drawing.tester.appendChild(testNode); + tester.appendChild(testNode); // standardize its position... do we really want to do this? d3.select(testNode).attr({ x: 0, @@ -613,21 +610,19 @@ drawing.bBox = function(node) { transform: '' }); - var testRect = testNode.getBoundingClientRect(); - if(!drawing.refRect) { - drawing.refRect = drawing.test3.select('.js-reference-point') + var testRect = testNode.getBoundingClientRect(), + refRect = test3.select('.js-reference-point') .node().getBoundingClientRect(); - } - drawing.tester.removeChild(testNode); + tester.removeChild(testNode); var bb = { height: testRect.height, width: testRect.width, - left: testRect.left - drawing.refRect.left, - top: testRect.top - drawing.refRect.top, - right: testRect.right - drawing.refRect.left, - bottom: testRect.bottom - drawing.refRect.top + left: testRect.left - refRect.left, + top: testRect.top - refRect.top, + right: testRect.right - refRect.left, + bottom: testRect.bottom - refRect.top }; // make sure we don't have too many saved boxes, diff --git a/src/lib/svg_text_utils.js b/src/lib/svg_text_utils.js index bb76fa2d283..f6d4419dc0c 100644 --- a/src/lib/svg_text_utils.js +++ b/src/lib/svg_text_utils.js @@ -19,8 +19,6 @@ var stringMappings = require('../constants/string_mappings'); // Append SVG -var parser = new DOMParser(); - d3.selection.prototype.appendSVG = function(_svgString) { var skeleton = [ '' ].join(''); - var dom = parser.parseFromString(skeleton, 'application/xml'), + var dom = new DOMParser().parseFromString(skeleton, 'application/xml'), childNode = dom.documentElement.firstChild; while(childNode) { From 9af534ab5650462448003bb205d6aa6a3564bac1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 12 May 2017 16:23:31 -0400 Subject: [PATCH 2/8] add drawing bb test - to simulate report https://github.com/plotly/plotly.js/issues/1678 --- test/jasmine/tests/drawing_test.js | 53 +++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/test/jasmine/tests/drawing_test.js b/test/jasmine/tests/drawing_test.js index 6f88b6bc316..4263b7357d0 100644 --- a/test/jasmine/tests/drawing_test.js +++ b/test/jasmine/tests/drawing_test.js @@ -1,9 +1,6 @@ -var Drawing = require('@src/components/drawing'); - var d3 = require('d3'); - var Plotly = require('@lib/index'); - +var Drawing = require('@src/components/drawing'); var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); var fail = require('../assets/fail_test'); @@ -348,6 +345,54 @@ describe('Drawing', function() { expect(g.attr('transform')).toEqual('translate(8,9) scale(4,5) translate(-8,-9) translate(1, 2)'); }); }); + + describe('bBox', function() { + afterEach(destroyGraphDiv); + + it('should update bounding box dimension on window scroll', function(done) { + var gd = createGraphDiv(); + + // allow page to scroll + gd.style.position = 'static'; + + Plotly.plot(gd, [{ + y: [1, 2, 1] + }], { + annotations: [{ + text: 'hello' + }], + height: window.innerHeight * 2, + width: 500 + }) + .then(function() { + var node = d3.select('text.annotation').node(); + expect(Drawing.bBox(node)).toEqual({ + height: 14, + width: 27.671875, + left: -13.671875, + top: -11, + right: 14, + bottom: 3 + }); + + window.scroll(0, 200); + return Plotly.relayout(gd, 'annotations[0].text', 'HELLO'); + }) + .then(function() { + var node = d3.select('text.annotation').node(); + expect(Drawing.bBox(node)).toEqual({ + height: 14, + width: 41.015625, + left: -20.671875, + top: -11, + right: 20.34375, + bottom: 3 + }); + }) + .catch(fail) + .then(done); + }); + }); }); describe('gradients', function() { From 3fdb3081ad2c8231145a57443cf381b6eac14688 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Mon, 15 May 2017 17:04:04 -0400 Subject: [PATCH 3/8] make bbox test more robust --- test/jasmine/tests/drawing_test.js | 34 ++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/drawing_test.js b/test/jasmine/tests/drawing_test.js index 4263b7357d0..3a3137dcd52 100644 --- a/test/jasmine/tests/drawing_test.js +++ b/test/jasmine/tests/drawing_test.js @@ -4,10 +4,15 @@ var Drawing = require('@src/components/drawing'); var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); var fail = require('../assets/fail_test'); +var customMatchers = require('../assets/custom_matchers'); describe('Drawing', function() { 'use strict'; + beforeAll(function() { + jasmine.addMatchers(customMatchers); + }); + describe('setClipUrl', function() { beforeEach(function() { @@ -349,6 +354,17 @@ describe('Drawing', function() { describe('bBox', function() { afterEach(destroyGraphDiv); + function assertBBox(actual, expected) { + expect(actual.height).toEqual(expected.height, 'height'); + expect(actual.top).toEqual(expected.top, 'top'); + expect(actual.bottom).toEqual(expected.bottom, 'bottom'); + + var TOL = 3; + expect(actual.width).toBeWithin(expected.width, TOL, 'width'); + expect(actual.left).toBeWithin(expected.left, TOL, 'left'); + expect(actual.right).toBeWithin(expected.right, TOL, 'right'); + } + it('should update bounding box dimension on window scroll', function(done) { var gd = createGraphDiv(); @@ -366,7 +382,7 @@ describe('Drawing', function() { }) .then(function() { var node = d3.select('text.annotation').node(); - expect(Drawing.bBox(node)).toEqual({ + assertBBox(Drawing.bBox(node), { height: 14, width: 27.671875, left: -13.671875, @@ -380,7 +396,7 @@ describe('Drawing', function() { }) .then(function() { var node = d3.select('text.annotation').node(); - expect(Drawing.bBox(node)).toEqual({ + assertBBox(Drawing.bBox(node), { height: 14, width: 41.015625, left: -20.671875, @@ -388,6 +404,20 @@ describe('Drawing', function() { right: 20.34375, bottom: 3 }); + + window.scroll(200, 0); + return Plotly.relayout(gd, 'annotations[0].font.size', 20); + }) + .then(function() { + var node = d3.select('text.annotation').node(); + assertBBox(Drawing.bBox(node), { + height: 22, + width: 66.015625, + left: -32.78125, + top: -18, + right: 33.234375, + bottom: 4 + }); }) .catch(fail) .then(done); From 0c62890b6f04e87201c921581d9c9d8abc193f2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Mon, 15 May 2017 17:30:59 -0400 Subject: [PATCH 4/8] cache ref to tester div and tester ref on drawing module object - so that Drawing.bBox does not have to query the DOM to fing them on every call. --- src/components/drawing/index.js | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/components/drawing/index.js b/src/components/drawing/index.js index 5eb5be186fa..724d99dec5d 100644 --- a/src/components/drawing/index.js +++ b/src/components/drawing/index.js @@ -579,16 +579,17 @@ drawing.makeTester = function(gd) { tester.node()._cache = {}; } - gd._tester = tester; - gd._testref = testref; + drawing.tester = gd._tester = tester; + drawing.testref = gd._testref = testref; }; // use our offscreen tester to get a clientRect for an element, // in a reference frame where it isn't translated and its anchor // point is at (0,0) // always returns a copy of the bbox, so the caller can modify it safely -var savedBBoxes = [], - maxSavedBBoxes = 10000; +var savedBBoxes = []; +var maxSavedBBoxes = 10000; + drawing.bBox = function(node) { // cache elements we've already measured so we don't have to // remeasure the same thing many times @@ -597,12 +598,13 @@ drawing.bBox = function(node) { return Lib.extendFlat({}, savedBBoxes[saveNum.value]); } - var test3 = d3.select('#js-plotly-tester'), - tester = test3.node(); + var tester3 = drawing.tester; + var tester = tester3.node(); // copy the node to test into the tester var testNode = node.cloneNode(true); tester.appendChild(testNode); + // standardize its position... do we really want to do this? d3.select(testNode).attr({ x: 0, @@ -610,9 +612,10 @@ drawing.bBox = function(node) { transform: '' }); - var testRect = testNode.getBoundingClientRect(), - refRect = test3.select('.js-reference-point') - .node().getBoundingClientRect(); + var testRect = testNode.getBoundingClientRect(); + var refRect = drawing.testref + .node() + .getBoundingClientRect(); tester.removeChild(testNode); From 273a4c37701241d6224a9c4c7fa3c9f84444f66f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Mon, 15 May 2017 17:31:38 -0400 Subject: [PATCH 5/8] cache DOMParser instance on svg utils module object - to not have to instantiate one on every appendSVG call --- src/lib/svg_text_utils.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/lib/svg_text_utils.js b/src/lib/svg_text_utils.js index f6d4419dc0c..d1544c1a359 100644 --- a/src/lib/svg_text_utils.js +++ b/src/lib/svg_text_utils.js @@ -17,6 +17,17 @@ var Lib = require('../lib'); var xmlnsNamespaces = require('../constants/xmlns_namespaces'); var stringMappings = require('../constants/string_mappings'); +exports.getDOMParser = function() { + if(exports.domParser) { + return exports.domParser; + } else if(window.DOMParser) { + exports.domParser = new window.DOMParser(); + return exports.domParser; + } else { + throw new Error('Cannot initialize DOMParser'); + } +}; + // Append SVG d3.selection.prototype.appendSVG = function(_svgString) { @@ -27,8 +38,9 @@ d3.selection.prototype.appendSVG = function(_svgString) { '' ].join(''); - var dom = new DOMParser().parseFromString(skeleton, 'application/xml'), - childNode = dom.documentElement.firstChild; + var domParser = exports.getDOMParser(); + var dom = domParser.parseFromString(skeleton, 'application/xml'); + var childNode = dom.documentElement.firstChild; while(childNode) { this.node().appendChild(this.node().ownerDocument.importNode(childNode, true)); From e0050f73c57c54755a6131584b152151bf0c7c06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 16 May 2017 12:11:43 -0400 Subject: [PATCH 6/8] don't expose DOMParser instance on svg utils module object --- src/lib/svg_text_utils.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/lib/svg_text_utils.js b/src/lib/svg_text_utils.js index d1544c1a359..2c9e47d40b8 100644 --- a/src/lib/svg_text_utils.js +++ b/src/lib/svg_text_utils.js @@ -17,12 +17,14 @@ var Lib = require('../lib'); var xmlnsNamespaces = require('../constants/xmlns_namespaces'); var stringMappings = require('../constants/string_mappings'); +var DOM_PARSER; + exports.getDOMParser = function() { - if(exports.domParser) { - return exports.domParser; + if(DOM_PARSER) { + return DOM_PARSER; } else if(window.DOMParser) { - exports.domParser = new window.DOMParser(); - return exports.domParser; + DOM_PARSER = new window.DOMParser(); + return DOM_PARSER; } else { throw new Error('Cannot initialize DOMParser'); } From c3c3b3ab331fc96769885a3c6abaa0e64b2fe846 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 16 May 2017 11:51:18 -0400 Subject: [PATCH 7/8] :hocho: gd._testref (wasn't used anywhere) --- src/components/drawing/index.js | 2 +- src/plots/plots.js | 1 - test/jasmine/tests/plots_test.js | 1 - 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/components/drawing/index.js b/src/components/drawing/index.js index 724d99dec5d..2ec765c3458 100644 --- a/src/components/drawing/index.js +++ b/src/components/drawing/index.js @@ -580,7 +580,7 @@ drawing.makeTester = function(gd) { } drawing.tester = gd._tester = tester; - drawing.testref = gd._testref = testref; + drawing.testref = testref; }; // use our offscreen tester to get a clientRect for an element, diff --git a/src/plots/plots.js b/src/plots/plots.js index 4efb546fa1d..8e8e21abd98 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1174,7 +1174,6 @@ plots.purge = function(gd) { // these get recreated on Plotly.plot anyway, but just to be safe // (and to have a record of them...) delete gd._tester; - delete gd._testref; delete gd._promises; delete gd._redrawTimer; delete gd.firstscatter; diff --git a/test/jasmine/tests/plots_test.js b/test/jasmine/tests/plots_test.js index b8ea16c9878..f95e98685f5 100644 --- a/test/jasmine/tests/plots_test.js +++ b/test/jasmine/tests/plots_test.js @@ -429,7 +429,6 @@ describe('Test Plots', function() { expect(gd.autoplay).toBeUndefined(); expect(gd.changed).toBeUndefined(); expect(gd._tester).toBeUndefined(); - expect(gd._testref).toBeUndefined(); expect(gd._promises).toBeUndefined(); expect(gd._redrawTimer).toBeUndefined(); expect(gd.firstscatter).toBeUndefined(); From a8fba0336791e92956f8a0da6708ed0ab89329b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 16 May 2017 12:11:03 -0400 Subject: [PATCH 8/8] replace gd._tester with drawing._tester --- src/components/drawing/index.js | 6 +++--- src/components/sliders/draw.js | 4 ++-- src/components/updatemenus/draw.js | 2 +- src/plot_api/plot_api.js | 4 ++-- src/plots/plots.js | 1 - src/traces/carpet/plot.js | 4 ++-- test/jasmine/tests/plots_test.js | 1 - 7 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/components/drawing/index.js b/src/components/drawing/index.js index 2ec765c3458..deaf8b2441b 100644 --- a/src/components/drawing/index.js +++ b/src/components/drawing/index.js @@ -542,11 +542,11 @@ drawing.steps = function(shape) { }; // off-screen svg render testing element, shared by the whole page -// uses the id 'js-plotly-tester' and stores it in gd._tester +// uses the id 'js-plotly-tester' and stores it in drawing.tester // makes a hash of cached text items in tester.node()._cache // so we can add references to rendered text (including all info // needed to fully determine its bounding rect) -drawing.makeTester = function(gd) { +drawing.makeTester = function() { var tester = d3.select('body') .selectAll('#js-plotly-tester') .data([0]); @@ -579,7 +579,7 @@ drawing.makeTester = function(gd) { tester.node()._cache = {}; } - drawing.tester = gd._tester = tester; + drawing.tester = tester; drawing.testref = testref; }; diff --git a/src/components/sliders/draw.js b/src/components/sliders/draw.js index 50c2ef0f35e..3965237d2ce 100644 --- a/src/components/sliders/draw.js +++ b/src/components/sliders/draw.js @@ -117,7 +117,7 @@ function keyFunction(opts) { // Compute the dimensions (mutates sliderOpts): function findDimensions(gd, sliderOpts) { - var sliderLabels = gd._tester.selectAll('g.' + constants.labelGroupClass) + var sliderLabels = Drawing.tester.selectAll('g.' + constants.labelGroupClass) .data(sliderOpts.steps); sliderLabels.enter().append('g') @@ -154,7 +154,7 @@ function findDimensions(gd, sliderOpts) { if(sliderOpts.currentvalue.visible) { // Get the dimensions of the current value label: - var dummyGroup = gd._tester.append('g'); + var dummyGroup = Drawing.tester.append('g'); sliderLabels.each(function(stepOpts) { var curValPrefix = drawCurrentValue(dummyGroup, sliderOpts, stepOpts.label); diff --git a/src/components/updatemenus/draw.js b/src/components/updatemenus/draw.js index 3c9c30968b2..558c4148a2d 100644 --- a/src/components/updatemenus/draw.js +++ b/src/components/updatemenus/draw.js @@ -504,7 +504,7 @@ function findDimensions(gd, menuOpts) { menuOpts.lx = 0; menuOpts.ly = 0; - var fakeButtons = gd._tester.selectAll('g.' + constants.dropdownButtonClassName) + var fakeButtons = Drawing.tester.selectAll('g.' + constants.dropdownButtonClassName) .data(menuOpts.buttons); fakeButtons.enter().append('g') diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index ecb623db380..cf0c268d955 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -93,9 +93,9 @@ Plotly.plot = function(gd, data, layout, config) { d3.select(gd).classed('js-plotly-plot', true); // off-screen getBoundingClientRect testing space, - // in #js-plotly-tester (and stored as gd._tester) + // in #js-plotly-tester (and stored as Drawing.tester) // so we can share cached text across tabs - Drawing.makeTester(gd); + Drawing.makeTester(); // collect promises for any async actions during plotting // any part of the plotting code can push to gd._promises, then diff --git a/src/plots/plots.js b/src/plots/plots.js index 8e8e21abd98..067eb0fb56e 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1173,7 +1173,6 @@ plots.purge = function(gd) { // these get recreated on Plotly.plot anyway, but just to be safe // (and to have a record of them...) - delete gd._tester; delete gd._promises; delete gd._redrawTimer; delete gd.firstscatter; diff --git a/src/traces/carpet/plot.js b/src/traces/carpet/plot.js index 9e62b3221c3..333f9f9c66e 100644 --- a/src/traces/carpet/plot.js +++ b/src/traces/carpet/plot.js @@ -59,8 +59,8 @@ function plotOne(gd, plotinfo, cd) { drawGridLines(xa, ya, boundaryLayer, aax, 'a-boundary', aax._boundarylines); drawGridLines(xa, ya, boundaryLayer, bax, 'b-boundary', bax._boundarylines); - var maxAExtent = drawAxisLabels(gd._tester, xa, ya, trace, t, labelLayer, aax._labels, 'a-label'); - var maxBExtent = drawAxisLabels(gd._tester, xa, ya, trace, t, labelLayer, bax._labels, 'b-label'); + var maxAExtent = drawAxisLabels(Drawing.tester, xa, ya, trace, t, labelLayer, aax._labels, 'a-label'); + var maxBExtent = drawAxisLabels(Drawing.tester, xa, ya, trace, t, labelLayer, bax._labels, 'b-label'); drawAxisTitles(labelLayer, trace, t, xa, ya, maxAExtent, maxBExtent); diff --git a/test/jasmine/tests/plots_test.js b/test/jasmine/tests/plots_test.js index f95e98685f5..4634488c94e 100644 --- a/test/jasmine/tests/plots_test.js +++ b/test/jasmine/tests/plots_test.js @@ -428,7 +428,6 @@ describe('Test Plots', function() { expect(gd.undonum).toBeUndefined(); expect(gd.autoplay).toBeUndefined(); expect(gd.changed).toBeUndefined(); - expect(gd._tester).toBeUndefined(); expect(gd._promises).toBeUndefined(); expect(gd._redrawTimer).toBeUndefined(); expect(gd.firstscatter).toBeUndefined();