Fix axis line width, length, and positioning for coupled subplots #1854

alexcjohnson · 2017-07-06T00:18:34Z

@etpinard this is for the first bug we came across this afternoon, but I'm not going to fix it until you sort out the second one (re: joinLayer) because I expect the symptoms are likely to change with that fix - although I don't expect it will be fixed. Turns out that there are a bunch of related issues with the wrong line widths getting applied to different lines, and the wrong positioning of ticks & labels regardless of which width is correct. The mock changes below make all the axis lines semi-transparent so we can tell if ticks and counteraxes abut (as intended) or overlap.

Also to note (will be fixed in this PR): axis lines currently get stroke-width set both as a style and an attribute - with different values 🙀 . I'll remove this duplication and standardize on style.

no updated baseline yet because the behavior is broken

no functionality changes here, just simplifying and linting

alexcjohnson · 2017-07-13T06:20:35Z

@etpinard this is ready for a look. I ended up changing nearly all the baseline images, so let me explain (and please look closely at the new images, let me know if you think these changes are positive AND acceptable in terms of compatibility).

Fix overlaying axis layers #1855 did partially fix this, as I hoped it would: at least after that each axis received its own line width, but the positioning still was off.
The first few commits are a stab at cleaning up the ancient tangle of code that draws axis lines. The functional changes are all in eb48a9d
I started out fixing how axis lines are drawn, which was broken in a number of multiple-axis situations (interestingly there was a TODO about it dating from 4 years ago when we first added multiple axes!) - see test image 20 eb48a9d#diff-2355db501d595161704e15acbd3a9960 where I extended this to all combinations of anchored / free / mirror axes, all with different line widths, to make sure the lines all abut correctly at corners.
Along the way I discovered we were doing weird things with tick label and title positioning as a function of axis line width - which became blazingly obvious in mock 20 with its super fat lines. Rationalizing that behavior moved the ticks on a substantial fraction of our mocks.
Then since I was staring at label positions and had modified a bunch of the mocks, I thought it would be a good time to address Alignment of horizontal grid lines and tick text is off #1434 - not the part where we proposed making it configurable, just the misalignment. I suppose I could have made that a separate commit, but it's really just this line which is a bit of a magic number but looks good to me across a wide range of fonts and font sizes. I suppose a better way to handle it would be to use bounding boxes but... there are tons of edge cases around that with tick labels, and this is far simpler (and faster) for a result that will only be minutely different. I'll also note that if you look very closely, you'll see the tick labels are not quite consistent in how they align with the ticks. In fact their coordinates are consistently aligned, I believe, but the ticks are rendered with crispEdges so their positions get rounded while the text is antialiased instead. We could round the text positions manually, but we'd have to turn that off for export... big mess. Lets just leave it as is.
I only put the baseline for mock 20 in the code commit, as that's got the key features I was fixing. All the other baselines are in the next commit.

And an aesthetic question for you: Notice in mock 20 and the x line and y line comments about how we handle free axes: if a free axis is aligned with an opposite-letter anchored axis, I opted to extend its line out the same way as if both axes are anchored - but perhaps it would be better to leave free axes always ending at the edge of the subplot? That would mean for example that the green, yellow, purple, and brown axes in 20 would be shortened to the same span as the blue and red axes.

etpinard · 2017-07-13T14:17:42Z

Wow. Thanks for digging up #1434.

Were you aware of #484 when working on this branch?

Starting my review now.

etpinard · 2017-07-13T14:23:04Z

src/plot_api/subroutines.js

-        var ya = Plotly.Axes.getFromId(gd, subplot, 'y');
-        var xDomain = xa.domain;
-        var yDomain = ya.domain;
+        var xDomain = plotinfo.xaxis.domain;


Amazing. Note that references in plotinfo in are updated linkSubplots called during supplyDefaults.

etpinard · 2017-07-13T14:28:13Z

src/plot_api/subroutines.js

    var axList = Plotly.Axes.list(gd);
+    var hasSVGCartesian = fullLayout._has('cartesian');


To note, this is needed as gl2d now uses cartesian (SVG layers) subplots to handle selections.

Ah right - I figured out that that's the reason, which is why I added SVG to the var name, but perhaps I should add the full explanation in a comment. Will become important when we start using SVG axes with GL2D plots @dfcreative

comment added 6d41a37

alexcjohnson · 2017-07-13T14:47:46Z

Were you aware of #484 when working on this branch?

When I first saw the title of that issue "Axis labels on very tall graph don't reach end of axis" I thought it might be related to the axis line being (sometimes) longer than the subplot (and therefore you can't have ticks/labels all the way to the end) - see my "aesthetic question" in #1854 (comment). That's one way in which it actually isn't a purely aesthetic consideration, you could argue that axis lines for free axes should map exactly to the extent of the plot area, to help the viewer map far-away data points onto the axis.

I think I buy that argument actually, so I'd be inclined to make this change. Any objection?

etpinard · 2017-07-13T14:56:25Z

src/plot_api/subroutines.js

+        var ylw = Drawing.crispRound(gd, ya.linewidth, 1);
+
+        function findMainAxis(ax) {
+            return ax.overlaying ? Plotly.Axes.getFromId(gd, ax.overlaying) : ax;


Non- ⛔ : might be nice to stash references to overlaying and anchor axes in plotinfo during makeSubplotData and 🔪 those Axes.getFromId

good idea. Actually makeSubplotData didn't really seem the place for this, it duplicated a lot of effort that should have just been done once per axis. I put this all into plots.linkSubplots. 274526a

etpinard · 2017-07-13T15:00:01Z

src/plot_api/subroutines.js

+            return ax.overlaying ? Plotly.Axes.getFromId(gd, ax.overlaying) : ax;
+        }
+
+        function findCounterAxes(ax) {


Would it be too much to ask to move those new find* functions outside of the subplotSelection.each scope? I don't think compilers can optimize that still meaning these functions are redefined for every subplot.

ah, I actually meant to un-nest these, thanks for reminding me. b31e60d

etpinard · 2017-07-13T15:05:42Z

src/plots/cartesian/axes.js

@@ -1792,7 +1795,9 @@ axes.doTicks = function(gd, axid, skipTitle) {
        }
        else {
            flipit = (axside === 'right') ? 1 : -1;
-            labely = function(d) { return d.dy + d.fontSize / 2 - labelShift * flipit; };
+            labely = function(d) {
+                return d.dy + d.fontSize * 0.35 - labelShift * flipit;


Non- ⛔ (might be better suited for #1434 - the part where we proposed making it configurable) but this 0.35 would look really nice side-by-side LINE_SPACING.

hah, I found the 0.35 value empirically, but polar had the same one, and it turns out there's a rational basis for exactly this value. 16bae9c

indeed! just out of curiosity, would alignment-baseline: central be an alternative, or there's something that would be lost with it? (tweenability comes to mind, smooth with pixels but not nice with discrete levels, though probably tweening isn't applicable here)

would alignment-baseline: central be an alternative

Quite possibly! I recall some baseline properties that I tried to use in the past not having sufficient cross-browser support, so we would need to test it. Also I'm not sure how this would work with pseudo-html (subscript/superscript, changing font sizes, multiple lines...) so I'm not going to do it now but we could investigate later.

If you run into such a thing I'm interested as the upcoming table uses it. IIRC dominant-baseline had some inconsistencies but don't now recall an issue with alignment-baseline. We'll see, I just bought a machine for real-life testing which as MS Edge on it.

etpinard · 2017-07-13T15:07:35Z

src/plot_api/subroutines.js

                    'M0,0')
-                .attr('stroke-width', ylw + 'px')
+                .style('stroke-width', ylw + 'px')


Good catch. Is .attr('stroke-width', /* */) a thing?

Yes, a lot of properties in SVG you can use either .attr or .style, with slightly different rules but also very different precedence - see https://css-tricks.com/presentation-attributes-vs-inline-styles/ - attributes are about the lowest, while inline styles are nearly the highest.
@monfera and I had a discussion about this a few days ago, I think for our purposes .style should be preferred.

Ha, interesting. It's something we might to standardize in strict-d3.js.

I think stroke-width as an attribute doesn't need the 'px' but also, the question kept brewing in my mind and I like the (user-experienced) flexibility of using attributes where possible, as it's easy to restyle them. With D3 the typical usage is inline styling of the element, which is for our current context, the highest priority, ie. the user would have a hard time overriding it (there are two main ways: 1) user reselects with D3 and changes the inline style; 2) user specifies !important styling in the style sheet. So, using attr would be more convenient for users if they choose to restyle via CSS.

... also, there's the thing that Microsoft browsers (IE, Edge) don't have full CSS support, so if the decision is to go with style as much as we can, then we still must use attr for transform and possibly some more presentation attributes, ie. some style->attr switch may need to happen during MS testing

Right, transform should stay as an attr for MS support. I think even some things like x and y that we've always provided as attributes can be styles. I think my own proclivity is to treat positioning as attributes and everything else as style, but I'm sure I've broken that rule. Might be worthwhile to create a list of property names we use and whether each should be used as a style or attribute? And then yes, we could enforce that in strict-d3.

I'm sympathetic to the idea of giving users more control, but in the end I think it would be a mistake to encourage overriding internals of the plot with CSS - we should push people to use the plot JSON instead - for three reasons:

Portability: with outside CSS the figure JSON no longer fully describes the plot. Even if you don't care about sharing a plot to another server, you might care about the image server or - depending on how the user applied their styling - even in-browser downloading could be affected.

Accidental overrides: we use a lot of generic class names like .point, .xtitle, that could get mangled easily if the page does something else with those classes.

Forward compatibility: obviously anyone can peek inside our elements and find selectors for whatever elements they'd like to style, but we've never published this as part of the API, and I don't want to give up the right to refactor our structures and rename classes.

Yes these points are all true. We don't currently go out of our way to dissuade scenegraph class based styling - we don't generate mangled or one-off class names or otherwise actively dissuade external styling. Also on the forum, SO and gh questions sometimes our answer is that a particular wish isn't supported but can be done with a d3.selectAll('whatever').style(...) and our scenegraph class is human friendly so I over-inferred 😄 That and the upside of not having to have heuristics for when to switch to attr. Btw. as a developer either way is fine as long as there's a guideline like what you mentioned above @alexcjohnson

Not sure if the emerging linter solution already covers it, I think this is the list of attributes (ie. all presentation attributes) that can go either way.

Unfortunately it's quite Venn-diagrammey and common stuff like x, dx, r etc. ie. drivers of the most basic geometry are not on it so even in the future, those things won't be able to use CSS transitions/animations. (Maybe interesting is that the SMIL deprecation note got retracted.)

etpinard · 2017-07-13T15:09:56Z

src/plot_api/subroutines.js

-        if(showfreex) { freefinished.push(xa._id); }
-        if(showfreey) { freefinished.push(ya._id); }
+        if(showFreeX) freeFinished[xa._id] = 1;
+        if(showFreeY) freeFinished[ya._id] = 1;


Smart. I usually set keys like this as true. But 1 is truthy and three characters shorter!

etpinard · 2017-07-13T15:20:47Z

src/plot_api/subroutines.js

+         * free x lines are not excluded - they don't necessarily *meet* the
+         * y lines, but it still looks good if the x line reaches to the ends
+         * of the y lines, especially in the case of a free axis parallel to
+         * an anchored axis, like this:


I agree 100%. Thanks for documenting.

etpinard · 2017-07-13T15:22:33Z

src/plot_api/subroutines.js

+         * y lines do not get longer when they meet x axes, because the
+         * x axis already filled that space and we don't want to double-fill.
+         * BUT they get longer if they're free axes, for the same reason as
+         * we do not exclude x axes:


mock 20 looks amazing.

etpinard · 2017-07-13T15:39:14Z

you could argue that axis lines for free axes should map exactly to the extent of the plot area, to help the viewer map far-away data points onto the axis.

I think I buy that argument actually, so I'd be inclined to make this change. Any objection?

Hmm. By extent of the plot area, you mean spanning exactly axis.domain?

alexcjohnson · 2017-07-13T15:47:49Z

Hmm. By extent of the plot area, you mean spanning exactly axis.range?

domain, but yeah - the idea is to have it exactly match the x or y pixel range on which we display data.

etpinard · 2017-07-13T20:23:14Z

@alexcjohnson here's what mpl does:

https://gist.github.com/etpinard/55254d82e1fd48d96a920bb306506238#file-mpl-free-axis-line-span-ipynb

Looks like free axes do span the whole axis domain regardless of their sister axis line width.

All in all, we'll probably need some new attribute governing this behavior in the future. I'll let you decide the best default value for now.

We could probably use these in other places too, to reduce getFromId calls

etpinard · 2017-07-13T20:31:40Z

And for those you (like me), that had issues understanding the implications of

you could argue that axis lines for free axes should map exactly to the extent of the plot area, to help the viewer map far-away data points onto the axis.

Here's the effect as of eb48a9d when relayout'ing xaxis.linewidth:

alexcjohnson · 2017-07-13T23:06:05Z

you could argue that axis lines for free axes should map exactly to the extent of the plot area, to help the viewer map far-away data points onto the axis.

I tried this out, and I think it both looks good and best represents the data. 739c998

Also I included margin.pad in that test image. It's not shown this way, but cliponaxis: false #1861 suddenly makes margin.pad a much more useful feature, since you can use it to set the axes back from the plot area (to give extra room so these non-clipped markers will not cover lines and labels) without increasing the axis ranges. cc @geocosmite

etpinard · 2017-07-14T14:24:35Z

src/plots/polar/micropolar.js

@@ -295,7 +296,7 @@ var µ = module.exports = { version: '0.2.2' };
            angularAxisEnter.append('text').classed('axis-text', true).style(fontStyle);
            var ticksText = angularAxis.select('text.axis-text').attr({
                x: radius + axisConfig.labelOffset,
-                dy: '.35em',
+                dy: MID_SHIFT + 'em',


🏆 for patching the polar code.

etpinard · 2017-07-14T14:28:32Z

test/image/mocks/20.json

@@ -291,6 +291,6 @@
        },
        "width": 750,
        "height": 600,
-        "margin": {"r": 200, "l": 50, "t": 50, "b": 50}
+        "margin": {"r": 200, "l": 50, "t": 50, "b": 50, "pad": 5}


🏆 for bringing margin.pad back to life.

etpinard · 2017-07-14T14:45:41Z

src/plots/plots.js

+    // while we're at it, link overlaying axes to their main axes and
+    // anchored axes to the axes they're anchored to
+    var axList = Plotly.Axes.list(mockGd, null, true);
+    for(i = 0; i < axList.length; i++) {


Much better. I'd glad linkSubplots is getting more attention. Updating all those refs early during supplyDefaults sounds like the way to go.

One small non-:no_entry: comment, that we could easily address later: Axes.list is now called twice during Plots.supplyDefaults here and after doAutoMargin here. Maybe we could call it once and stash the result in fullLayout instead?

etpinard · 2017-07-14T14:52:52Z

💃 solid work.

😄 to see that cliponaxis: false made us discover (and fix) a bunch of little axis bugs.

I thought it would be a good time to address #1434 - not the part where we proposed making it configurable, just the misalignment.

Make sure to either close #1434 and open up a new issue about making tick label alignment configurable OR update the #1434 issue title accordingly.

alexcjohnson added status: in progress bug something broken labels Jul 6, 2017

etpinard mentioned this pull request Jul 6, 2017

Fix overlaying axis layers #1855

Merged

alexcjohnson added 6 commits July 13, 2017 01:20

updated mock 20 to test axis line width, length, and positioning

829cd6e

no updated baseline yet because the behavior is broken

lint var statements in subroutines.lsInner

fe1615b

plotinfo already has xaxis/yaxis stashed

5b495e6

code cleanup in lsInner

543d6b0

no functionality changes here, just simplifying and linting

fix axis line length logic and tweak label positioning

eb48a9d

update all other baselines for the tweaked label positioning

aaba530

alexcjohnson force-pushed the axis-lines branch from 0da3b77 to aaba530 Compare July 13, 2017 05:21

alexcjohnson changed the title ~~[WIP] Fix axis line width, length, and positioning for coupled subplots~~ Fix axis line width, length, and positioning for coupled subplots Jul 13, 2017

alexcjohnson added status: reviewable and removed status: in progress labels Jul 13, 2017

update gl2d mocks with colorbars for tick label position tweak

27d2ca2

etpinard reviewed Jul 13, 2017

View reviewed changes

comment on hasSVGCartesian

6d41a37

un-nest new lsInner helper functions

b31e60d

stash _mainAxis and _anchorAxis in fullLayout axis objects

274526a

We could probably use these in other places too, to reduce getFromId calls

etpinard added this to the 1.29.0 milestone Jul 13, 2017

alexcjohnson added 3 commits July 13, 2017 17:03

make a constant for font midline alignment

16bae9c

Merge branch 'master' into axis-lines

893588d

do not lengthen disconnected axes, also test margin.pad positioning

739c998

etpinard reviewed Jul 14, 2017

View reviewed changes

alexcjohnson merged commit 700c088 into master Jul 14, 2017

alexcjohnson deleted the axis-lines branch July 14, 2017 15:32

alexcjohnson mentioned this pull request Jul 14, 2017

Alignment of horizontal grid lines and tick text is off #1434

Closed

alexcjohnson mentioned this pull request Jul 24, 2017

colorbar draws outside the bounds #970

Closed

etpinard mentioned this pull request Feb 2, 2018

Mixed up axes assignments in subplots #1312

Closed

		var axList = Plotly.Axes.list(gd);
		var hasSVGCartesian = fullLayout._has('cartesian');

Fix axis line width, length, and positioning for coupled subplots #1854

Fix axis line width, length, and positioning for coupled subplots #1854

Conversation

alexcjohnson commented Jul 6, 2017

alexcjohnson commented Jul 13, 2017

etpinard commented Jul 13, 2017

etpinard Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Jul 13, 2017 • edited Loading

alexcjohnson commented Jul 13, 2017

etpinard commented Jul 13, 2017

etpinard commented Jul 13, 2017

alexcjohnson commented Jul 13, 2017

Choose a reason for hiding this comment

etpinard Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Jul 14, 2017

etpinard Jul 13, 2017 •

edited

Loading

etpinard Jul 13, 2017 •

edited

Loading

etpinard Jul 13, 2017 •

edited

Loading

etpinard commented Jul 13, 2017 •

edited

Loading

etpinard Jul 14, 2017 •

edited

Loading