Skip to content

Drawing bbox fix for window scroll #1683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 16, 2017
38 changes: 18 additions & 20 deletions src/components/drawing/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down Expand Up @@ -579,16 +579,16 @@ drawing.makeTester = function(gd) {
tester.node()._cache = {};
}

gd._tester = tester;
gd._testref = testref;
drawing.tester = tester;
drawing.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
Expand All @@ -598,14 +598,13 @@ 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 tester3 = drawing.tester;
var tester = tester3.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,
Expand All @@ -614,20 +613,19 @@ drawing.bBox = function(node) {
});

var testRect = testNode.getBoundingClientRect();
if(!drawing.refRect) {
drawing.refRect = drawing.test3.select('.js-reference-point')
.node().getBoundingClientRect();
}
var refRect = drawing.testref
.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,
Expand Down
4 changes: 2 additions & 2 deletions src/components/sliders/draw.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/components/updatemenus/draw.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
20 changes: 16 additions & 4 deletions src/lib/svg_text_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,20 @@ var Lib = require('../lib');
var xmlnsNamespaces = require('../constants/xmlns_namespaces');
var stringMappings = require('../constants/string_mappings');

// Append SVG
var DOM_PARSER;

exports.getDOMParser = function() {
if(DOM_PARSER) {
return DOM_PARSER;
} else if(window.DOMParser) {
DOM_PARSER = new window.DOMParser();
return DOM_PARSER;
} else {
throw new Error('Cannot initialize DOMParser');
}
};

var parser = new DOMParser();
// Append SVG

d3.selection.prototype.appendSVG = function(_svgString) {
var skeleton = [
Expand All @@ -29,8 +40,9 @@ d3.selection.prototype.appendSVG = function(_svgString) {
'</svg>'
].join('');

var dom = parser.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));
Expand Down
4 changes: 2 additions & 2 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -1173,8 +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._testref;
delete gd._promises;
delete gd._redrawTimer;
delete gd.firstscatter;
Expand Down
4 changes: 2 additions & 2 deletions src/traces/carpet/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
83 changes: 79 additions & 4 deletions test/jasmine/tests/drawing_test.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
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');
var customMatchers = require('../assets/custom_matchers');

describe('Drawing', function() {
'use strict';

beforeAll(function() {
jasmine.addMatchers(customMatchers);
});

describe('setClipUrl', function() {

beforeEach(function() {
Expand Down Expand Up @@ -348,6 +350,79 @@ 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);

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();

// 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();
assertBBox(Drawing.bBox(node), {
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();
assertBBox(Drawing.bBox(node), {
height: 14,
width: 41.015625,
left: -20.671875,
top: -11,
right: 20.34375,
bottom: 3
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 29abb5d made the bottom and top keys here incorrectly change - as the bbox computation were based on a outdated cached tester element.

Copy link
Contributor Author

@etpinard etpinard May 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The width, left and right keys change correctly here in response to the annotation text relayout update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Ha, those values appear to be machine-dependent (I suspect a font issue) ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 3fdb308

});

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);
});
});
});

describe('gradients', function() {
Expand Down
2 changes: 0 additions & 2 deletions test/jasmine/tests/plots_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -428,8 +428,6 @@ describe('Test Plots', function() {
expect(gd.undonum).toBeUndefined();
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();
Expand Down