From f506306b439db46e2f9de421aaddfb0d225ed988 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 24 Sep 2019 17:40:56 -0400 Subject: [PATCH 1/5] use (xl,xr,yt,yb) instead of (x,y) as push margin coordinates ... this enables an axis to push the margin on both side of its span --- src/plots/cartesian/axes.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/plots/cartesian/axes.js b/src/plots/cartesian/axes.js index c4bb577a8f8..14d810eb831 100644 --- a/src/plots/cartesian/axes.js +++ b/src/plots/cartesian/axes.js @@ -1897,12 +1897,12 @@ axes.drawOne = function(gd, ax, opts) { if(llbbox.width > 0) { var rExtra = llbbox.right - (ax._offset + ax._length); if(rExtra > 0) { - push.x = 1; + push.xr = 1; push.r = rExtra; } var lExtra = ax._offset - llbbox.left; if(lExtra > 0) { - push.x = 0; + push.xl = 0; push.l = lExtra; } } @@ -1917,12 +1917,12 @@ axes.drawOne = function(gd, ax, opts) { if(llbbox.height > 0) { var bExtra = llbbox.bottom - (ax._offset + ax._length); if(bExtra > 0) { - push.y = 0; + push.yb = 0; push.b = bExtra; } var tExtra = ax._offset - llbbox.top; if(tExtra > 0) { - push.y = 1; + push.yt = 1; push.t = tExtra; } } From 7a8eedf44b72456e8341802cc4efb7a19d6349fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 24 Sep 2019 17:51:29 -0400 Subject: [PATCH 2/5] do not try to re-fix axis label overlaps when current autoangle:=90 N.B. during auto-margin redraw, if the axis fixed its label overlaps by rotating 90 degrees, do not attempt to re-fix its label overlaps as this can lead to infinite redraw loops! Moreover, use ax._prevTickAngles for retrieve "lastAngle". Previous ax._tickAngles was used, but this one gets clear early Axes.drawOne, so it didn't really do anything. --- src/plots/cartesian/axes.js | 44 +++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/src/plots/cartesian/axes.js b/src/plots/cartesian/axes.js index 14d810eb831..77ee7a853dd 100644 --- a/src/plots/cartesian/axes.js +++ b/src/plots/cartesian/axes.js @@ -1643,6 +1643,7 @@ axes.draw = function(gd, arg, opts) { * - ax._anchorAxis * - ax._subplotsWith * - ax._counterDomainMin, ax._counterDomainMax (optionally, from linkSubplots) + * - ax._tickAngles (on redraw only, old value relinked during supplyDefaults) * - ax._mainLinePosition (from lsInner) * - ax._mainMirrorPosition * - ax._linepositions @@ -1684,6 +1685,8 @@ axes.drawOne = function(gd, ax, opts) { // - stash tickLabels selection, so that drawTitle can use it to scoot title ax._selections = {}; // stash tick angle (including the computed 'auto' values) per tick-label class + // linkup 'previous' tick angles on redraws + if(ax._tickAngles) ax._prevTickAngles = ax._tickAngles; ax._tickAngles = {}; // measure [in px] between axis position and outward-most part of bounding box // (touching either the tick label or ticks) @@ -2400,6 +2403,7 @@ axes.drawZeroLine = function(gd, ax, opts) { * - {number} tickangle * - {object (optional)} _selections * - {object} (optional)} _tickAngles + * - {object} (optional)} _prevTickAngles * @param {object} opts * - {array of object} vals (calcTicks output-like) * - {d3 selection} layer @@ -2416,13 +2420,14 @@ axes.drawZeroLine = function(gd, ax, opts) { axes.drawLabels = function(gd, ax, opts) { opts = opts || {}; + var fullLayout = gd._fullLayout; var axId = ax._id; var axLetter = axId.charAt(0); var cls = opts.cls || axId + 'tick'; var vals = opts.vals; var labelFns = opts.labelFns; var tickAngle = opts.secondary ? 0 : ax.tickangle; - var lastAngle = (ax._tickAngles || {})[cls]; + var prevAngle = (ax._prevTickAngles || {})[cls]; var tickLabels = opts.layer.selectAll('g.' + cls) .data(ax.showticklabels ? vals : [], tickDataFn); @@ -2507,17 +2512,17 @@ axes.drawLabels = function(gd, ax, opts) { // do this without waiting, using the last calculated angle to // minimize flicker, then do it again when we know all labels are // there, putting back the prescribed angle to check for overlaps. - positionLabels(tickLabels, lastAngle || tickAngle); + positionLabels(tickLabels, (prevAngle + 1) ? prevAngle : tickAngle); function allLabelsReady() { return labelsReady.length && Promise.all(labelsReady); } + var autoangle = null; + function fixLabelOverlaps() { positionLabels(tickLabels, tickAngle); - var autoangle = null; - // check for auto-angling if x labels overlap // don't auto-angle at all for log axes with // base and digit format @@ -2584,19 +2589,36 @@ axes.drawLabels = function(gd, ax, opts) { positionLabels(tickLabels, autoangle); } } - - if(ax._tickAngles) { - ax._tickAngles[cls] = autoangle === null ? - (isNumeric(tickAngle) ? tickAngle : 0) : - autoangle; - } } if(ax._selections) { ax._selections[cls] = tickLabels; } - var done = Lib.syncOrAsync([allLabelsReady, fixLabelOverlaps]); + var seq = [allLabelsReady]; + + // N.B. during auto-margin redraw, if the axis fixed its label overlaps + // by rotating 90 degrees, do not attempt to re-fix its label overlaps + // as this can lead to infinite redraw loops! + if(fullLayout._redrawFromAutoMarginCount && prevAngle === 90) { + autoangle = 90; + seq.push(function() { + positionLabels(tickLabels, prevAngle); + }); + } else { + seq.push(fixLabelOverlaps); + } + + // save current tick angle for future redraws + if(ax._tickAngles) { + seq.push(function() { + ax._tickAngles[cls] = autoangle === null ? + (isNumeric(tickAngle) ? tickAngle : 0) : + autoangle; + }); + } + + var done = Lib.syncOrAsync(seq); if(done && done.then) gd._promises.push(done); return done; }; From 631014a1688c4a495125c9774d8e8b438d995555 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 24 Sep 2019 17:57:05 -0400 Subject: [PATCH 3/5] Add (large) upper bound on the number of redraw calls ... that Plots.doAutoMargin can trigger. --- src/plots/plots.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index b34c025d72f..0927035c725 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1960,7 +1960,19 @@ plots.doAutoMargin = function(gd) { } else { fullLayout._redrawFromAutoMarginCount = 1; } - return Registry.call('plot', gd); + + // Always allow at least one redraw and give each margin-push + // call 3 loops to converge. Of course, for most cases this way too many, + // but let's keep things on the safe side until we fix our + // auto-margin pipeline problems: + // https://github.com/plotly/plotly.js/issues/2704 + var maxNumberOfRedraws = 3 * (1 + Object.keys(pushMarginIds).length); + + if(fullLayout._redrawFromAutoMarginCount < maxNumberOfRedraws) { + return Registry.call('plot', gd); + } else { + Lib.warn('Too many auto-margin redraws.'); + } } }; From 58fd6327481a261fc2392725051bc9f81e8219b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 24 Sep 2019 18:04:01 -0400 Subject: [PATCH 4/5] add mock :lock: case that currently result in infinite loop --- .../baselines/automargin-small-width.png | Bin 0 -> 8933 bytes test/image/mocks/automargin-small-width.json | 21 ++++++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 test/image/baselines/automargin-small-width.png create mode 100644 test/image/mocks/automargin-small-width.json diff --git a/test/image/baselines/automargin-small-width.png b/test/image/baselines/automargin-small-width.png new file mode 100644 index 0000000000000000000000000000000000000000..93c0c9a8e93e43769712349ef527df4439ce47aa GIT binary patch literal 8933 zcmeHtXHXQ;+9pw800}yS2ofdE5CjQ=fMg`63FHAmB!dz}G9o!h8bClLN*G|sL9!rW z$V$#iQi2Ld9{8Gj?^f;pxV8IdzpCB3Q`I#+-RGR?bNYGT_j#V~D1BXZTF^}p5fKsX zLk$%};2T3kM7&RV1=w5E{7OMY#6k2>MbY@V<#sl0daCMl&o7LrNznzRmd)~5~oE*jUqTt8m zW0aW3su|ga!Dt3DZV)|;yGqO`RP{#2+!TO9lbNji4Zs_@F~Uiek9tZj@G9g=prb3BQok`7;M3_aB@z@w} zbJUkDf(E2e6)gvEVCD__$4Hl%n#6L8kNv-wKdmNd2}hNJg)p0x-4*ZLaA*B0xHG}E z&wFI}IPaQr2pPVK*CX%2e}4U6SqW>L?mKy*)z#IiK*EdSii$MX`KG?3Ki_%8#j6c+ zsHv$7s;b2Nod`=EFL^{nL|j*&^VN8{EO3#7UV5D!d6-l`j0S{{ zj}}HF;_9uNgm8I`J#+R8Q5r~7y?{|6B?q&#JKC71w4}s1jRNYuzhUrqvJ@%e@Z`zs zReTzLZ7_43|BfXWO83vVMloyLM7afYZ*4GH(q%@4@O)>k(GZE`d$u-UfcOD~f!L$Z zG{nv>vvsMh0jK?u4BYufA9Ts^)mu9Xy%|1-v(m3`GGZUt`dk!{;R%vg^TWxY^waY} z0v%7=;z)}*J3uPlhgO=nIk)c*95rwxiSvXsM%aJ4{hu#MvD?271@Usz=I(`n$Zz8# zQmCE;_B08n2@P-Wx*E4dON_9EAOQd5Vxtd?*sr$yu4Mr)!I}8Ca(?+mMH-q~T2zn@ z5cKt3R4K9o(k|h>g$h;0?FW0U^fCy*U@6np_7MTt!^I#4&4o&+;A*2(0*cdj^(zaO zKcZa8nC*T!v^0P3$7I1HW46IM)cE)~sO0R-zpuX^WN(T@GW`u5#n<&!r$!_RH^kE{ zyz!2ScgFVRO)7&YQg{%vk3--_M%>AKdYHL6b7oQWgXGljMI+|3KAANW=4(1>KQ)24iLXm0JlWc*!p#A0g0uOMM7zT1+Gl3WGS8hCF&bjOqT3|-( z{i+FEu&28|&uX+^`J{4SRFyqO9>dT?2?2&^0;7>dW(G39JQoJD zqMV=??Cw`Df#Wb)p8a)l5?=x@&CktM0#Kjo2td`~rw*#oNYJ}A4>|gHt+O9lZa<_#c&2#lv^x+l z2ql}6-IIj&=akaVU#v4*nDC?yyQKMGlIEa)U#2D}b8BH>iC1sQ+!f>qz~;|j``GJG z%5fznnqI!Y3*y=2O3q*W{mCQhjZYIv8_bj{HLz(`!BHYmAyehp*dxT7TF^-~2maOb+zI>Nrv^QUr5fzjYbGbJR=Gj>E32Viil zjOe4%eDt}Lf+@@t#_b7!Ru$(7Z7v#QDVqsXhm%Y}i zwE3$}_vVysApiPj!no<;^xQ!CCPuh^zwgQJx=FRAgX@85P{dP+{h=f^!a2$;d9T&= zRMCqYOY6`fhopij8VM9MDTNkLXoZ5mKLRj8PF{100Z{2JorUo zEZWx~3VnBPst!pBzAv8f{lKvYYS%KCi0CK|AEQ}=YK3#5T6yxice$85 zc5@kpFm9=o7`MJBmCgSd;(xam_gpKzw`X_~PeW8Ho}vKx7#s z5Spf@Z%*nvUQ%dTT3VWuQz4>XdaVsGH~wlpwcXzseL5&H+1sBAwZmhC#DMy#)=$GO zZyp^J!y_jrm*YqV%~A1ScVBDm%$D`(?|emlS#V2oX=!Pn9?tvVlX$dNn)ilcO8i}2 z%TrQQUkM?M#u%{IDK?MS-ytE6-@2l#dDP-=@&H7lZGUH8Rx|tGByF58O`}WYM41_% zTN|P7CrH_$GmPr#ua-ki1Oj2a>g429VDZu0(8@}r6j7EG9Ssr?5`qtB$#@C=oHo#F z5gtacjq&^LRnN!~CU}ElpXN48<{v9pyjCO#J6G|i=>`mi=av9R69xP~{iGIY=E#(E zNqU&f`|2W~E0STios>@FoqWI>Z2N0Y<^2476)`cf{K7)spVJ>l=?GDz>vd7M8JeERx!4@vO{j;MS9$rH=bZIJ}_g{*j@2 z0vkN2<;7patLM9`csqce|7?67g)Wdr6`e1%_!R&Q?y@ETD*~VA%Gw3!d0*DOXDRT` zw!!o}${qisOuWa(N-e&+4=_E)w z+uL%u05vf&*~MF(iwV{jr)K-FxE_rDAjC@?hxQ-N^czvgqzi@h%tct5a}$OtoOT^D zYyZ+t{Ldi*m9y!h^j!*}$k%%(zq^vG_aY@eq4CoE@&2_SlpuGI4nyG{{J0Fxd-v{v zisEm{Hetb=Ys$on=$v&{N$2!f)_Y?21^xeyBMTZDrh1%?i#7pwhB|a>{v$FvI(h{V znq#$2$Z;X>ow;eRGL!;4=q@#k{djNmIy@L%@Z?iwxnze}nS1n_9*#!R-_r8!*_OS6 z_|cUBli_SxQS=rYR|(+RA}7u)XEvsDp1vUL|Mlq!||5RgS!)7t)h zr<#?E-=FC4$hH%BbVsy4l7o@NCaFMX%vl?S{mW%1<%j^R* z)0+VsK>dd%bcgR(1QDR+bvaemghQ6aqwq|$s$_ePa>OOS#7Ra)O`5qjs=7A+QTs8{ z5zoVv^4D+i)G~QsYqnkw@CWa)G%$L49X%Mvl$4a%&eS-#hW#M;!y0xU{J(KQDp ztG>^1EO1YX_ax?e3WS;y%5WNH8)b%1cI_M_S6}eAobG(w-NlRGUFzp~edfK=0AtWe zr~-~A{7DEhPIVCs*m?HONIsob0MG+E!F7O@W1a;RInBVpR?v8zoOSU2(o4D$(LK6= z4Q109Cosy?X@Vq!(-9JAY+D~8%o;d8R-k3}o~*Ae7mz8fgl)G{4xQ{i8Qh3yms4Z5m==jMbC{Q90LWpW4QK;3l_Mp8RgDc3BSS&~zcAYh4#5_mSkd|?CS+}}v5H#dk! zz`|yA+7Kbr{Q02TtFXK60P@|Y0T=n{H{z)Xg|p@Nv)z-Ycj{kpD8#zA9S^!m&U9@; z#pCEMPA1zhi)U+6PXUjwK***71Olx*Vn0p)Tz_VTK!g^wV6KR(Jg1wGvL@!bHX%g} zk*A-Z*RC25@f!t{Q=XADA_R^=@E*nJxNj^|OhN9MT3QOXozDMCffL#(jKWvB)`7+tDkTu&z2`hLFJDIi7mLFC3&rhB?Rt zuCGQXT#GqLxBA4UKk$)2_!Wd)&kVRYT||=7X7@Sbl)&9Lxga_&)u?p0PhBof8t!w% z%)D;O6W~@z|HUWvbA3OQVJQ(c_f1$b8Z%W$#&_$JkM`UQg!fwM7rMaTq{kEWt>e9m zpZbYA6VZ*Hyru1(0+Q*z3ED)Cn-gU}IT$SzpEQxFTqng6v04w9sdm14eKV7h>&6JV zTKFEinC4o(bX=toUrUdU2|17?T6m6Pw556IKiSNKCr38`QEWe`=HqWvwW=CPGj!(@ zMX-V38B$y0kqBp@iYH%byNubYXj4da@vQkK z!YNmsAZb1_{D|x$Kw}!ZgC8YOQoy!1b|00IP;VlB>x}$TFH`_Rp#M;8nVM{Vsjitl z-hZVh%efJV!y!?@$T+G*s_LFd_Nu{q@ zmqd-zhXkf4PL`FyA)C&$!Tco7`8bzr{9MBHp|RO2>CRnI!d3wM#~G<~`#Qc-A!wX6 zrVPRgKIjFVd@<b!DGIevRfu>{`%o0#8a3O0DJk zLGP}tb56Z>H)6qVkTaRn!&#;GfM-9I2VABU5VGe#i$9P(L;fbm^|H)#mbV<-6gvZ^ zRjlME8gt9>E{o5PrSDy9rqbI}6veYj-I@+yRJuivo!*pAsLR8K&Z|Z=o0ApD*>)GR zpqJ@5l1LZ2TcA+84^!_IXwP`F$zjk!XHlZgaJ1*_k=qe!gV$@QDG!GTd6jI3E_UDS zb_+T+Is>^fLF9!OJk!%hS9xRIfB*Qf=~p%hfZ8sX5z{v zW^ukF+S#+^`JTP$LD+lrLq0~B%In9E9|P0z{{9~Oj<99_so5r#h;L}ho18{o_sCFM zpIkOl^dfu#{qt+_Pi)kbi*;XMFv0}JS{F`bxW!703L_E!VvTb&rfA7q?(`JMHqQv+miE69A+lzJe_zdtcxCb})2(%S zYD`(oY7L+^aO92Y?T!rfqFc)whE48|G55Nbz3U2gu(y5quxgjSF zCNM*v1aYQKyG;ub4ve0!GNs%Q)FaYqU+bt0Exi7|NdPAJmR$BJBM;&G%g9IyuSu{I*M)>CvN3XE6XAM z9x80Mb?OlC{c$%n-dP zf<>L*LU%-m+f?BA<1F-YB$QZrqR4|nQ#JY^Cx?d%etITCM<`TyXD{TnwZ~*(SO?t% zS{VYKpo$sJd7u&|OnNug+3R*`lhQ3PNQTbYn$)(=9M3pLnmo`DNxZ5=!+iB_h-`?T zr|8jxIayZ|)#pj^D!3$lhQO#a&uP0fU*CZZ@f(#k^7M)+4aeZL=m_0|j_vCkpFL!1 z%P$*_ZPxOp@7HoJ>$=2RGGJo+EKtJEB#ON>OOuTodN}7Ne;~rj{kL3aJ5LJdHr*w} zB&Tf3X^rR_v?y3mI$;K1YzinXrTZwf^do&DlH+?BoC%$8by%5-PaSPTQ@PN~&)o-% zIv)09feRs`e}gaLM0w^TW&r6=41F3CKnd%zu#UK@7SmM7tn`!-I7)56Ej?F=p4hwmdEE+F0mQ9IqRTUbf8&*$TX6m%ahaqQ_n*A+}(@O3~5vWMdcFWtX8rh7T)&tVM|%dc2f7kh6)S-!qYZMj|`N zJzO#g_Y5u2dWhc(`5v$QoMDMcGZNofz6Y4hF0-wj^5&$4wi#JSqHDRqlz8bxs!VzJ zV5$!+S?cZDpKDs?9qkftg|vBx+UNT@tW{NY49?T3SZdeK^AoS=)(${ zih|GRd#=al<`0x)yd_i=w?5Evh2E{P1|$2Hx59ICxm-~aoG%1dqEt7Uo9}~{=^MM* zbmi)1pTz_S>Z9MsH&ooT?nPP!)RItJaOf6?55K}Ovw>_L>>Kwoetft2^XGNcROPkZ zYI?Dk)OWo2WPUY)?E*E40sL|-Im1}VnWH(nhn{q`(cQeOvD3V#yDdl$+fJIvSXQQ! z%a31F!?8DYtHwa6&Vw2_A?Hj9cdDxAu_9n$8p>PLRO#+n4CUM1cjnVWM*^zEg)o0L zHUv1DT3=Gol{k>kkw@vm%cQ>8_%c cl!Uf@^v!5#GWKfV^%bIrs=6xWO6bu40!&iQN&o-= literal 0 HcmV?d00001 diff --git a/test/image/mocks/automargin-small-width.json b/test/image/mocks/automargin-small-width.json new file mode 100644 index 00000000000..4476fba6d67 --- /dev/null +++ b/test/image/mocks/automargin-small-width.json @@ -0,0 +1,21 @@ +{ + "data": [ + { + "type": "bar", + "x": [ "Montreal", "Tofu Bowl", "Tropical Beaches" ], + "y": [ 3, 1, 2 ] + } + ], + "layout": { + "margin": { + "l": 20, + "r": 0, + "b": 30, + "t": 15 + }, + "xaxis": { + "automargin": true + }, + "width": 150 + } +} From 9a665dbd1245ba9a0461696b44366cb004a7b7ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 25 Sep 2019 09:49:43 -0400 Subject: [PATCH 5/5] add extra ax.automargin condition for fixLabelOverlaps bypass --- src/plots/cartesian/axes.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plots/cartesian/axes.js b/src/plots/cartesian/axes.js index 77ee7a853dd..722e95e06a2 100644 --- a/src/plots/cartesian/axes.js +++ b/src/plots/cartesian/axes.js @@ -2597,10 +2597,10 @@ axes.drawLabels = function(gd, ax, opts) { var seq = [allLabelsReady]; - // N.B. during auto-margin redraw, if the axis fixed its label overlaps + // N.B. during auto-margin redraws, if the axis fixed its label overlaps // by rotating 90 degrees, do not attempt to re-fix its label overlaps // as this can lead to infinite redraw loops! - if(fullLayout._redrawFromAutoMarginCount && prevAngle === 90) { + if(ax.automargin && fullLayout._redrawFromAutoMarginCount && prevAngle === 90) { autoangle = 90; seq.push(function() { positionLabels(tickLabels, prevAngle);