Fix padding of horizontal axes when labels are rotated

[kurkle]

I noticed a issue with layout when working with label rotations.
The left/right padding for all horizontal scales are calculated in a "minimum height" phase. That causes excess padding if long labels are present (rotation would be zero or close to it).

Side effects of this same issue has been worked by @nagix in #5961 / calculateTickRotation comment

Master:

This PR:

The actual change is using chartHeight / 2 instead of horizontalBoxHeight in getMinimumBoxSize. Other changes are refactoring to make this function simpler.

[etimberg]

The reason this used horizontalBoxHeight is that the sum of the heights of all the horizontal boxes needs to be capped at chartHeight / 2 so that there is room for the chart. Is it possible now to have the axes take up more space because we measure them at a bigger size?

[kurkle]

Is it possible now to have the axes take up more space because we measure them at a bigger size?

Its been like that for a long time already (couldn't find the original change, more than 3 years ago from #1837)
Also visible in both of the attached images, axes take more space than the chart.

Because that horizontalBoxHeight is not actually used in the final layout:

Chart.js/src/core/core.layouts.js

Line 272 in abe74a6

box.update(box.fullWidth ? chartWidth : maxChartAreaWidth, chartHeight / 2, scaleMargin);

[benmccann]

It took me quite awhile to understand that minSize was an ILayoutItem and not a number. Maybe we could rename it to something clearer like minBox.

chartHeight / 2 seems wrong. What if there are multiple axes? Does this value actually represent the minimum height or is ignored? Could we fix the old horizontalBoxHeight rather than removing it?

[kurkle]

It took me quite awhile to understand that minSize was an ILayoutItem and not a number. Maybe we could rename it to something clearer like minBox.

I agree there's room for improvement in these names.

chartHeight / 2 seems wrong. What if there are multiple axes? Does this value actually represent the minimum height or is ignored? Could we fix the old horizontalBoxHeight rather than removing it?

It represents maximum height available for box, but I agree it seems wrong.
However, this was not something I was fixing, just the padding issue. I tried using horizontalBoxHeight instead in both places (makes more sense to me), but then issue #1766 reappears.

[kurkle]

Basically we are now figuring out the minimum size of the horizontal box by using same maximum height available as in the final fit (=chartHeight / 2). I'd rather not change the chartHeight / 2 now, unless there is a issue that relates to it.

[benmccann]

Should it be chartHeight / 2 / verticalBoxes.length though? I don't understand why we can start ignoring how many axes there are

[nagix]

I had the same concern as @benmccann, but the vertical boxes include the title and legend boxes, and they don't always require horizontalBoxHeight, so the calculation of the label rotation might not work as expected if we use horizontalBoxHeight as maxHeight in update. With the proposed solution, if there are multiple axes and each axis has long labels, the total height might exceed the chart height. I wonder if we can say it's an ignorable edge case.

[etimberg]

I think we could ignore that edge case for now. When I wrote this code the first time I wasn't considering the case of multiple horizontal axes and mostly wanted to support multiple vertical axes since that was a common use case. I think that also explains chartHeight/2 more because with 1 horizontal axis splitting the height in 2 seemed like a reasonable step.

In v3 I think I'd like to explore splitting the fit function into 2 separate methods: measure and arrange. This matches the patterns I've seen in a number of other systems, such as WPF and it means we can keep a difference between calling fit to get a size and calling fit to actually setup the internals.

If the split into 2 separate methods goes well and we could get measure performing well, we could rethink the core layout algorithm. Having 2 passes was a compromise between a slow iterative algorithm that kept looping until boxes didn't change much in size and a single pass algorithm that just assigned sizes based on heuristics.

[nagix]

That 2-pass approach makes a lot of sense to me. I'm fine with the solution in this PR for now, and the refactoring is so good.

[simonbrunel]

Didn't review this PR in depth (@etimberg knows this part better than me) so will just make a small comment about the form. Else, it looks really good to me :)