Axes.draw w/o getBoundingClientRect + many axis automargin fixes #4165

etpinard · 2019-09-04T23:44:16Z

This PR resolves #1988, fixes #2434, improves #298 and should help us towards #2704

Before this PR, ax._boundingBox was computed in Axes.drawOne using a raw (and costly) getBoundingClientRect call. In turn, ax._boundingBox was used to: (1) position cartesian spikes (2) position range sliders (3) compute the axis auto-margin push value. Meaning that for most graphs, that getBoundingClientRect was called for no reason. Commit d2650fe introduces a better of doing things, see commit description for the details. Commit 2e7b3dd essentially removes the need for computing a bounding box to render cartesian spikes.

Elsewhere in this PR,

the first two commits are clean-ups commits,
the next two are fixups of things I noticed while working in auto-margin land Automargin pipeline #2704
d54ec94 and a7e8eaf presents a few axis auto-margin bugs I discovered
these get fixed in daa88e0, 40f1b24 and bb127af

cc @plotly/plotly_js

... we no longer need to do this since the Axes.doTicks -> Axes.drawOne refactor

... this thing is has not been used since we move geo subplot to `fullLayout._paper` layer.

... to make `lsInner` more similar with other async subroutines. This patch has no effect on the behavior.

- need to draw them (again) in finalDraw, by the time we get to drawData we have the correct auto-margin and auto-range info required to draw them correctly!

- two showing problems with `ax.mirror` - one showing problems with multiline axis titles - one showing with long tilted labels + rangeslider ... added with 1.49.4 failing baselines

... to show divider span bug, update baseline using 1.49.4

- covering cases where `ax._subplotsWith.length > 1` and ax in question isn't at extremity of graph - covering cases for `anchor:'free'` axes - make other test use `Plotly.plot` instead of `newPlot`, to not purge the graph twice.

... which is a more appropriate name. Moreover, improve its perf by using _anchorAxis cache instead of `getFromId`

... to position cartesian spike edges. This allows us to not have to compute the axis bounding box to render the spikes.

... which can be computed before ax.setScale() - This speeds up Axes.drawOne (e.g. for drag interactions) - rangeslider requires one less loop over the counter axes - make Fx.hover not have to depend on field computed in Axes.drawOne - ax._counterDomain(Min|Max) will also be used for mirror auto-margin computations

- replace it with getLabelLevelBBox (an extension of getLabelLevelSpan), to compute axis margin push values. Use cache to not have to compute the (costly) label bounding boxes unnecessarily - compute ax._depth during Axes.drawOne, which is the measure in px between axis position and outward-most part of bounding box, we use this for (1) multicategory label, (2) axis title and (3) rangeslider positioning

- some range slider baselines have a slightly bigger bottom margin - automargin-rangeslider-and-sidepush now has the correct side margin push (but is still missing its ax title) - multicategory2 now has the correct top margin push (but its dividers are not long enough)

... by registering a mirror-specific push-margin id. - Note that to handle the 'all' and 'allticks', we use ax._counterDomain(Min|Max) added previously. - both automargin-mirror* baselines are now correct

- automargin-multiline-titles and automargin-rangeslider-and-sidepush now push the bottom enough to make their axis title visible

- multicategory2 now has the correct divider length - some multicategory baselines are updated with more precise ax._depth, which tweaks the axis title positioning - multicategory_histograms and multicategory-sorting appear to have (very) slightly longer dividers

etpinard · 2019-09-04T23:45:58Z

Unfortunately, switching from using 1 raw getBoundingClientRect to Drawing.bBox lead to few baseline changes. Please refer to each of the commit description for explanations.

etpinard · 2019-09-04T23:47:57Z

src/plots/cartesian/axes.js

+ // (like in fixLabelOverlaps) instead and use Axes.getPxPosition
+ // together with the makeLabelFns outputs and `tickangle`
+ // to compute one bbox per (tick value x tick style)
+ var bb = Drawing.bBox(thisLabel.node().parentNode);


We should probably copy this comment over to #298 - after this PR gets merged.

etpinard · 2019-09-05T00:04:30Z

src/plots/cartesian/axes.js

+ * - ax._counterDomainMin, ax._counterDomainMax (optionally, from linkSubplots)
+ * - ax._mainLinePosition (from lsInner)
+ * - ax._mainMirrorPosition
+ * - ax._linepositions


Computing and stashing _mainLinePosition, _mainMirrorPosition and _linepositions in lsInner is less than ideal. This makes Axes.drawOne "depend" on lsInner i.e. lsInner must get called before Axes.drawOne on first draw and 'ticks' edits.

Now in relation to #2704, if we choose to add a "pure margin computation" step e.g. called Axes.pushMargin, we'll need to un-coupled drawOne and lsInner. After this PR, Axes.pushMargin for a single axis could look like:

axes.pushMargin = function(gd, ax) { var seq = []; /* compute mainLinePosition */ seq.push(function() { return drawLabels(gd, ax, { vals: axes.calcTicks(ax), layer: /* some dummy layer in the tester div */, transFn: axes.makeTransFn(ax), labelFns: axes.makeLabelFns(ax, mainLinePosition) }) }) /* N.B. we don't need to draw ticks, grid lines nor axis lines to compute the margin push values */ seq.push(function() { /* compute the margin push values */ Plots.autoMargin(gd, /* */) }) }

or maybe we could have a set of getter Axes helpers so we won't have to stash the values.

etpinard · 2019-09-05T00:07:11Z

src/plots/cartesian/axes.js

 var hasRangeSlider = Registry.getComponentMethod('rangeslider', 'isVisible')(ax);

- function doAutoMargins() {
+ seq.push(function() {


This diff may be a little hard to read, but essentially this commits 🔪 the calcBoundingBox step and makes the margin push value computation step this:

plotly.js/src/plots/cartesian/axes.js

Lines 1864 to 1946 in d2650fe

seq.push(function() {

var s = ax.side.charAt(0);

var pos = axes.getPxPosition(gd, ax);

var outsideTickLen = ax.ticks === 'outside' ? ax.ticklen : 0;

var llbbox;

var push;

var rangeSliderPush;

if(ax.automargin || hasRangeSlider) {

if(ax.type === 'multicategory') {

llbbox = getLabelLevelBbox('tick2');

} else {

llbbox = getLabelLevelBbox();

if(axLetter === 'x' && s === 'b') {

ax._depth = Math.max(llbbox.width > 0 ? llbbox.bottom - pos : 0, outsideTickLen);

}

}

}

if(ax.automargin) {

push = {x: 0, y: 0, r: 0, l: 0, t: 0, b: 0};

var domainIndices = [0, 1];

if(axLetter === 'x') {

if(s === 'b') {

push[s] = ax._depth;

} else {

push[s] = ax._depth = Math.max(llbbox.width > 0 ? pos - llbbox.top : 0, outsideTickLen);

domainIndices.reverse();

}

if(llbbox.width > 0) {

var rExtra = llbbox.right - (ax._offset + ax._length);

if(rExtra > 0) {

push.x = 1;

push.r = rExtra;

}

var lExtra = ax._offset - llbbox.left;

if(lExtra > 0) {

push.x = 0;

push.l = lExtra;

}

}

} else {

if(s === 'l') {

push[s] = ax._depth = Math.max(llbbox.height > 0 ? pos - llbbox.left : 0, outsideTickLen);

} else {

push[s] = ax._depth = Math.max(llbbox.height > 0 ? llbbox.right - pos : 0, outsideTickLen);

domainIndices.reverse();

}

if(llbbox.height > 0) {

var bExtra = llbbox.bottom - (ax._offset + ax._length);

if(bExtra > 0) {

push.y = 0;

push.b = bExtra;

}

var tExtra = ax._offset - llbbox.top;

if(tExtra > 0) {

push.y = 1;

push.t = tExtra;

}

}

}

push[counterLetter] = ax.anchor === 'free' ?

ax.position :

ax._anchorAxis.domain[domainIndices[0]];

// TODO won't work for multi-line titles !!

if(ax.title.text !== fullLayout._dfltTitle[axLetter]) {

push[s] += ax.title.font.size;

}

}

if(hasRangeSlider) {

rangeSliderPush = Registry.getComponentMethod('rangeslider', 'autoMarginOpts')(gd, ax);

}

Plots.autoMargin(gd, axAutoMarginID(ax), push);

Plots.autoMargin(gd, rangeSliderAutoMarginID(ax), rangeSliderPush);

});

antoinerg · 2019-09-05T19:49:58Z

Pretty solid PR @etpinard 💪 It's well commented and the commits are well organized.

I'm tempted to give you a dancer but I feel like someone else should also take a look at it considering it is touching cartesian axes. @alexcjohnson would be an ideal reviewer.

archmoj

Great PR.
@etpinard please find my question below.

src/plots/cartesian/axes.js

archmoj · 2019-09-06T21:14:28Z

@antoinerg Looking good to me as well.
@etpinard 💃 💃

alexcjohnson

Beautiful. Nothing to add 💃

archmoj · 2019-09-06T21:23:29Z

Thanks @alexcjohnson for the review.

etpinard added 15 commits September 4, 2019 16:02

🔪 useless axis mocking in polar

93ef043

... we no longer need to do this since the Axes.doTicks -> Axes.drawOne refactor

🔪 obsolete geocontainer purge call

e907b9f

... this thing is has not been used since we move geo subplot to `fullLayout._paper` layer.

use Plots.previousPromises in lsInner

0cf93f8

... to make `lsInner` more similar with other async subroutines. This patch has no effect on the behavior.

draw in shapes, images and annotations in drawData only

2a9bbee

- need to draw them (again) in finalDraw, by the time we get to drawData we have the correct auto-margin and auto-range info required to draw them correctly!

add four failing axis automargin mocks

d54ec94

- two showing problems with `ax.mirror` - one showing problems with multiline axis titles - one showing with long tilted labels + rangeslider ... added with 1.49.4 failing baselines

add outside ticks to multicategory2 mock

a7e8eaf

... to show divider span bug, update baseline using 1.49.4

add spikeline tests

16542f3

- covering cases where `ax._subplotsWith.length > 1` and ax in question isn't at extremity of graph - covering cases for `anchor:'free'` axes - make other test use `Plotly.plot` instead of `newPlot`, to not purge the graph twice.

rename titleOffset -> Axes.getPxPosition

9781e8c

... which is a more appropriate name. Moreover, improve its perf by using _anchorAxis cache instead of `getFromId`

use Axes.getPxPosition instead of ax._boundingBox

2e7b3dd

... to position cartesian spike edges. This allows us to not have to compute the axis bounding box to render the spikes.

fix #2434 - handle ax.automargin + ax.mirror cases

daa88e0

... by registering a mirror-specific push-margin id. - Note that to handle the 'all' and 'allticks', we use ax._counterDomain(Min|Max) added previously. - both automargin-mirror* baselines are now correct

fix ax automargin for multi-line ax titles

40f1b24

- automargin-multiline-titles and automargin-rangeslider-and-sidepush now push the bottom enough to make their axis title visible

etpinard added bug something broken status: reviewable labels Sep 4, 2019

etpinard added this to the v1.50.0 milestone Sep 4, 2019

etpinard commented Sep 4, 2019

View reviewed changes

etpinard commented Sep 5, 2019

View reviewed changes

archmoj reviewed Sep 6, 2019

View reviewed changes

src/plots/cartesian/axes.js Show resolved Hide resolved

alexcjohnson approved these changes Sep 6, 2019

View reviewed changes

etpinard merged commit b5f0316 into master Sep 6, 2019

etpinard deleted the draw-axes-no-bounding-box-final branch September 6, 2019 21:28

This was referenced Sep 6, 2019

Splom open items #2555

Open

Plotly.redraw / drawLabels performance #298

Closed

snyk-bot mentioned this pull request Feb 4, 2020

[Snyk] Upgrade plotly.js from 1.45.3 to 1.52.0 beauremus/ftp-viewer#1

Merged

snyk-bot mentioned this pull request Feb 22, 2020

[Snyk] Upgrade plotly.js from 1.35.2 to 1.52.1 raindigi/eShopOnContainersAI#1

Open

snyk-bot mentioned this pull request Apr 17, 2020

[Snyk] Upgrade plotly.js from 1.41.3 to 1.52.3 dunzoit/redash#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Axes.draw w/o getBoundingClientRect + many axis automargin fixes #4165

Axes.draw w/o getBoundingClientRect + many axis automargin fixes #4165

etpinard commented Sep 4, 2019

etpinard commented Sep 4, 2019 •

edited

Loading

etpinard Sep 4, 2019

etpinard Sep 5, 2019

etpinard Sep 5, 2019

antoinerg commented Sep 5, 2019

archmoj left a comment

archmoj commented Sep 6, 2019

alexcjohnson left a comment

archmoj commented Sep 6, 2019

	seq.push(function() {
	var s = ax.side.charAt(0);
	var pos = axes.getPxPosition(gd, ax);
	var outsideTickLen = ax.ticks === 'outside' ? ax.ticklen : 0;
	var llbbox;

	var push;
	var rangeSliderPush;

	if(ax.automargin \|\| hasRangeSlider) {
	if(ax.type === 'multicategory') {
	llbbox = getLabelLevelBbox('tick2');
	} else {
	llbbox = getLabelLevelBbox();
	if(axLetter === 'x' && s === 'b') {
	ax._depth = Math.max(llbbox.width > 0 ? llbbox.bottom - pos : 0, outsideTickLen);
	}
	}
	}

	if(ax.automargin) {
	push = {x: 0, y: 0, r: 0, l: 0, t: 0, b: 0};
	var domainIndices = [0, 1];

	if(axLetter === 'x') {
	if(s === 'b') {
	push[s] = ax._depth;
	} else {
	push[s] = ax._depth = Math.max(llbbox.width > 0 ? pos - llbbox.top : 0, outsideTickLen);
	domainIndices.reverse();
	}

	if(llbbox.width > 0) {
	var rExtra = llbbox.right - (ax._offset + ax._length);
	if(rExtra > 0) {
	push.x = 1;
	push.r = rExtra;
	}
	var lExtra = ax._offset - llbbox.left;
	if(lExtra > 0) {
	push.x = 0;
	push.l = lExtra;
	}
	}
	} else {
	if(s === 'l') {
	push[s] = ax._depth = Math.max(llbbox.height > 0 ? pos - llbbox.left : 0, outsideTickLen);
	} else {
	push[s] = ax._depth = Math.max(llbbox.height > 0 ? llbbox.right - pos : 0, outsideTickLen);
	domainIndices.reverse();
	}

	if(llbbox.height > 0) {
	var bExtra = llbbox.bottom - (ax._offset + ax._length);
	if(bExtra > 0) {
	push.y = 0;
	push.b = bExtra;
	}
	var tExtra = ax._offset - llbbox.top;
	if(tExtra > 0) {
	push.y = 1;
	push.t = tExtra;
	}
	}
	}

	push[counterLetter] = ax.anchor === 'free' ?
	ax.position :
	ax._anchorAxis.domain[domainIndices[0]];

	// TODO won't work for multi-line titles !!
	if(ax.title.text !== fullLayout._dfltTitle[axLetter]) {
	push[s] += ax.title.font.size;
	}
	}

	if(hasRangeSlider) {
	rangeSliderPush = Registry.getComponentMethod('rangeslider', 'autoMarginOpts')(gd, ax);
	}

	Plots.autoMargin(gd, axAutoMarginID(ax), push);
	Plots.autoMargin(gd, rangeSliderAutoMarginID(ax), rangeSliderPush);
	});

Axes.draw w/o getBoundingClientRect + many axis automargin fixes #4165

Axes.draw w/o getBoundingClientRect + many axis automargin fixes #4165

Conversation

etpinard commented Sep 4, 2019

etpinard commented Sep 4, 2019 • edited Loading

etpinard Sep 4, 2019

Choose a reason for hiding this comment

etpinard Sep 5, 2019

Choose a reason for hiding this comment

etpinard Sep 5, 2019

Choose a reason for hiding this comment

antoinerg commented Sep 5, 2019

archmoj left a comment

Choose a reason for hiding this comment

archmoj commented Sep 6, 2019

alexcjohnson left a comment

Choose a reason for hiding this comment

archmoj commented Sep 6, 2019

etpinard commented Sep 4, 2019 •

edited

Loading