[WIP] Vertically center multi-line labels on Y axis

[darabos]

Fixes #1137. The second commit shows the before and after images from the test.

[darabos]

Oops, sorry. I was trying to follow the contribution guidelines and send this PR against my fork, but I messed up. Well anyway, as long as I'm here already, let me know what you think. I guess splitting on <br> is not the most elaborate way to do this. Maybe I should add a handy function in svg_text_utils.js instead?

[alexcjohnson]

@darabos thanks for the PR and no problem, we can discuss it here.

The new mock and baseline are great, thanks!

The test failures come because we haven't always made sure that d.text is actually a string, sometimes it's a number (try loading mock 14, that's the one that failed first in the image tests).

But three problems I see make me think we need a different solution:
First, transfn is used for ticks too, which is not what you want:

Second, I don't think splitting on <br> is the right way to do it anyway - we also accept \n for example, and in case there are others now or later I wouldn't want to hard-code this here.

Third, we need to consider how this interacts with axis.tickangle. It's not as common for y axes as for x, but it is supported, and moreover x axis labels at 90 degrees may also want this correction. Here's how it looks on your branch at 90 degrees - OK:

but at -90 not so good:

I think the solution is going to need to be in positionLabels because that's after we've called svgTextUtils.convertToTspans on the text, so we can use svgTextUtils.lineCount.

[darabos]

Thanks for the careful advice! I'll switch to svgTextUtils.lineCount in a moment. For now I've moved the code to positionLabels (it fixed the ticks!) and made sure it works correctly for all sides and all angles. At least for the -90/0/90 angles. For slanted angles the correct behavior is less clear, but the results seem okay to me.

But what a terrible bloated code I ended up with! Sorry. I'll try to phrase it more compactly or move it to a separate function if I cannot.

[darabos]

I've switched to svgTextUtils.lineCount and moved the logic to a separate function. I don't see a nice way to unify this into one formula, but if you look at it case-by-case, it's not too complex.

I have expanded the test and you can see a before-after for the baseline image in 3adcdc9. Thanks!

[alexcjohnson]

@darabos I haven't looked at the code yet, but from the tests that failed I like it already - four of our test images have multiline y axis labels that changed for the better, so we need to update the baseline images. If you look in the artifacts tab https://circleci.com/gh/plotly/plotly.js/7559#artifacts/containers/0 you can see which ones failed.

If you've got our image tester set up you can run npm run baseline -- benchmarks etc to generate the new ones, if not you can just download the relevant ones from the artifacts -> test_images directory and overwrite the version in test/image/baselines.

(side note: world-cals also changed in my latest PR #2437 - whichever of these is merged last will have to regenerate the baseline again so it gets both changes...)

[alexcjohnson]

"y 1"

[darabos]

"y 1"

Oops, fixed.

[alexcjohnson]

Very nice, I don't think this is overly long at all, it has pretty much exactly the complexity required.

The new mock is fantastic, clearly covers all the cases we care about. The only question in my mind is the cutoff angle - why did you choose 20 degrees? It looks a little funny to me in the 45-degree cases where the x and y axis labels are essentially symmetric (look at x5/y5, and rotate the image 90 degrees CCW), but the x axis labels are vertically centered while the y axis labels are aligned to the first line. Symmetry would say the logic for an x axis at angle angle should match the logic for a y axis at angle 90 - angle.

Thinking also about cases where this could cause problems: the main issue I see is if you have a hard edge on the plot (either axis.showline or something like bars or a heatmap flush with the edge) particularly when you're not showing ticks. In order for multiline labels to not overlap the plot in that case, we need to make sure the angle between the vertical text and the axis is fairly small before we vertically center the text. So that would argue for a fairly small cutoff in the y case (could be 20 as you have it, or maybe 30?) and a correspondingly large cutoff in the x case (70 or 60).

[darabos]

I had no particular reason for 20 degrees. I think I ended up with the asymmetric setup because it counteracts an existing asymmetry a bit. But that's not a good reason. 🙂 I've switched to your recommendation. (30° cutoff for y, 60° cutoff for x.)

You can see the asymmetry I mean if you look in the top right on the current baseline. Both place the anchor at the start of the third line, but the gap between the tick and the text is greater on the top than on the right. Also the gap for x 2 is greater than for y 1.

I suppose this is unrelated to this pull request though, and does not really bother my eye.

[alexcjohnson]

💃 Great work @darabos - thanks!

[etpinard]

@darabos Incredible first PR. Thanks very much 🥇