Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix padding of horizontal axes when labels are rotated #6021

Merged
merged 6 commits into from Feb 2, 2019
Merged

Fix padding of horizontal axes when labels are rotated #6021

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5aa6c63
use same heigth for horizontal axes in step 4 thats used in steps 5/6
kurkle Jan 28, 2019
abe74a6
a little refactoring
kurkle Jan 28, 2019
5362045
renumbering, some minimization
kurkle Jan 29, 2019
0345c93
Better minification (and certainly performance)
kurkle Jan 29, 2019
075e61a
revert numbering, add a TODO in Step3
kurkle Jan 29, 2019
cd260d0
fix comment
kurkle Jan 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
170 changes: 67 additions & 103 deletions src/core/core.layouts.js
View file
Open in desktop
Expand Up @@ -26,6 +26,33 @@ function sortByWeight(array, reverse) {
});
}

function findMaxPadding(boxes) {
var maxPadding = {top: 0, left: 0, bottom: 0, right: 0};
kurkle marked this conversation as resolved.
Show resolved Hide resolved
helpers.each(boxes, function(box) {
if (box.getPadding) {
var boxPadding = box.getPadding();
maxPadding.left = Math.max(maxPadding.left, boxPadding.left);
maxPadding.right = Math.max(maxPadding.right, boxPadding.right);
maxPadding.top = Math.max(maxPadding.top, boxPadding.top);
maxPadding.bottom = Math.max(maxPadding.bottom, boxPadding.bottom);
}
});
return maxPadding;
}

function addHeightByPosition(boxes, size) {
helpers.each(boxes, function(box) {
size[box.position] += box.height;
});
}

function addWidthByPosition(boxes, size) {
helpers.each(boxes, function(box) {
size[box.position] += box.width;
});
}


defaults._set('global', {
layout: {
padding: {
Expand Down Expand Up @@ -142,6 +169,10 @@ module.exports = {
sortByWeight(topBoxes, true);
sortByWeight(bottomBoxes, false);

var verticalBoxes = leftBoxes.concat(rightBoxes);
var horizontalBoxes = topBoxes.concat(bottomBoxes);
var outerBoxes = verticalBoxes.concat(horizontalBoxes);

// Essentially we now have any number of boxes on each of the 4 sides.
// Our canvas looks like the following.
// The areas L1 and L2 are the left axes. R1 is the right axis, T1 is the top axis and
Expand Down Expand Up @@ -171,7 +202,6 @@ module.exports = {
// What we do to find the best sizing, we do the following
// 1. Determine the minimum size of the chart area.
// 2. Split the remaining width equally between each vertical axis
// 3. Split the remaining height equally between each horizontal axis
// 4. Give each layout the maximum size it can be. The layout will return it's minimum size
kurkle marked this conversation as resolved.
Show resolved Hide resolved
// 5. Adjust the sizes of each axis based on it's minimum reported size.
// 6. Refit each axis
Expand All @@ -183,13 +213,10 @@ module.exports = {
var chartWidth = width - leftPadding - rightPadding;
var chartHeight = height - topPadding - bottomPadding;
var chartAreaWidth = chartWidth / 2; // min 50%
var chartAreaHeight = chartHeight / 2; // min 50%

// Step 2
var verticalBoxWidth = (width - chartAreaWidth) / (leftBoxes.length + rightBoxes.length);
var verticalBoxWidth = (width - chartAreaWidth) / verticalBoxes.length;

// Step 3
var horizontalBoxHeight = (height - chartAreaHeight) / (topBoxes.length + bottomBoxes.length);

// Step 4
var maxChartAreaWidth = chartWidth;
Expand All @@ -201,7 +228,7 @@ module.exports = {
var isHorizontal = box.isHorizontal();

if (isHorizontal) {
minSize = box.update(box.fullWidth ? chartWidth : maxChartAreaWidth, horizontalBoxHeight);
minSize = box.update(box.fullWidth ? chartWidth : maxChartAreaWidth, chartHeight / 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@kurkle kurkle Jan 28, 2019 •
edited by benmccann

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

maxChartAreaHeight -= minSize.height;
} else {
minSize = box.update(verticalBoxWidth, maxChartAreaHeight);
Expand All @@ -215,37 +242,15 @@ module.exports = {
});
}

helpers.each(leftBoxes.concat(rightBoxes, topBoxes, bottomBoxes), getMinimumBoxSize);
helpers.each(outerBoxes, getMinimumBoxSize);

// If a horizontal box has padding, we move the left boxes over to avoid ugly charts (see issue #2478)
var maxHorizontalLeftPadding = 0;
var maxHorizontalRightPadding = 0;
var maxVerticalTopPadding = 0;
var maxVerticalBottomPadding = 0;

helpers.each(topBoxes.concat(bottomBoxes), function(horizontalBox) {
if (horizontalBox.getPadding) {
var boxPadding = horizontalBox.getPadding();
maxHorizontalLeftPadding = Math.max(maxHorizontalLeftPadding, boxPadding.left);
maxHorizontalRightPadding = Math.max(maxHorizontalRightPadding, boxPadding.right);
}
});

helpers.each(leftBoxes.concat(rightBoxes), function(verticalBox) {
if (verticalBox.getPadding) {
var boxPadding = verticalBox.getPadding();
maxVerticalTopPadding = Math.max(maxVerticalTopPadding, boxPadding.top);
maxVerticalBottomPadding = Math.max(maxVerticalBottomPadding, boxPadding.bottom);
}
});
var maxPadding = findMaxPadding(outerBoxes);

// At this point, maxChartAreaHeight and maxChartAreaWidth are the size the chart area could
// be if the axes are drawn at their minimum sizes.
// Steps 5 & 6
var totalLeftBoxesWidth = leftPadding;
var totalRightBoxesWidth = rightPadding;
var totalTopBoxesHeight = topPadding;
var totalBottomBoxesHeight = bottomPadding;
var outerBoxSizes = {top: topPadding, left: leftPadding, bottom: bottomPadding, right: rightPadding};

// Function to fit a box
function fitBox(box) {
Expand All @@ -254,10 +259,10 @@ module.exports = {
});

if (minBoxSize) {
if (box.isHorizontal()) {
if (minBoxSize.horizontal) {
var scaleMargin = {
left: Math.max(totalLeftBoxesWidth, maxHorizontalLeftPadding),
right: Math.max(totalRightBoxesWidth, maxHorizontalRightPadding),
left: Math.max(outerBoxSizes.left, maxPadding.left),
right: Math.max(outerBoxSizes.right, maxPadding.right),
top: 0,
bottom: 0
};
Expand All @@ -272,27 +277,12 @@ module.exports = {
}

// Update, and calculate the left and right margins for the horizontal boxes
helpers.each(leftBoxes.concat(rightBoxes), fitBox);

helpers.each(leftBoxes, function(box) {
totalLeftBoxesWidth += box.width;
});

helpers.each(rightBoxes, function(box) {
totalRightBoxesWidth += box.width;
});
helpers.each(verticalBoxes, fitBox);
addWidthByPosition(verticalBoxes, outerBoxSizes);
simonbrunel marked this conversation as resolved.
Show resolved Hide resolved

// Set the Left and Right margins for the horizontal boxes
helpers.each(topBoxes.concat(bottomBoxes), fitBox);

// Figure out how much margin is on the top and bottom of the vertical boxes
helpers.each(topBoxes, function(box) {
totalTopBoxesHeight += box.height;
});

helpers.each(bottomBoxes, function(box) {
totalBottomBoxesHeight += box.height;
});
helpers.each(horizontalBoxes, fitBox);
addHeightByPosition(horizontalBoxes, outerBoxSizes);

function finalFitVerticalBox(box) {
var minBoxSize = helpers.findNextWhere(minBoxSizes, function(minSize) {
Expand All @@ -302,8 +292,8 @@ module.exports = {
var scaleMargin = {
left: 0,
right: 0,
top: totalTopBoxesHeight,
bottom: totalBottomBoxesHeight
top: outerBoxSizes.top,
bottom: outerBoxSizes.bottom
};

if (minBoxSize) {
Expand All @@ -312,60 +302,34 @@ module.exports = {
}

// Let the left layout know the final margin
helpers.each(leftBoxes.concat(rightBoxes), finalFitVerticalBox);
helpers.each(verticalBoxes, finalFitVerticalBox);

// Recalculate because the size of each layout might have changed slightly due to the margins (label rotation for instance)
totalLeftBoxesWidth = leftPadding;
totalRightBoxesWidth = rightPadding;
totalTopBoxesHeight = topPadding;
totalBottomBoxesHeight = bottomPadding;

helpers.each(leftBoxes, function(box) {
totalLeftBoxesWidth += box.width;
});

helpers.each(rightBoxes, function(box) {
totalRightBoxesWidth += box.width;
});

helpers.each(topBoxes, function(box) {
totalTopBoxesHeight += box.height;
});
helpers.each(bottomBoxes, function(box) {
totalBottomBoxesHeight += box.height;
});
outerBoxSizes = {top: topPadding, left: leftPadding, bottom: bottomPadding, right: rightPadding};
addWidthByPosition(verticalBoxes, outerBoxSizes);
addHeightByPosition(horizontalBoxes, outerBoxSizes);

// We may be adding some padding to account for rotated x axis labels
var leftPaddingAddition = Math.max(maxHorizontalLeftPadding - totalLeftBoxesWidth, 0);
totalLeftBoxesWidth += leftPaddingAddition;
totalRightBoxesWidth += Math.max(maxHorizontalRightPadding - totalRightBoxesWidth, 0);
var leftPaddingAddition = Math.max(maxPadding.left - outerBoxSizes.left, 0);
outerBoxSizes.left += leftPaddingAddition;
outerBoxSizes.right += Math.max(maxPadding.right - outerBoxSizes.right, 0);

var topPaddingAddition = Math.max(maxVerticalTopPadding - totalTopBoxesHeight, 0);
totalTopBoxesHeight += topPaddingAddition;
totalBottomBoxesHeight += Math.max(maxVerticalBottomPadding - totalBottomBoxesHeight, 0);
var topPaddingAddition = Math.max(maxPadding.top - outerBoxSizes.top, 0);
outerBoxSizes.top += topPaddingAddition;
outerBoxSizes.bottom += Math.max(maxPadding.bottom - outerBoxSizes.bottom, 0);

// Figure out if our chart area changed. This would occur if the dataset layout label rotation
// changed due to the application of the margins in step 6. Since we can only get bigger, this is safe to do
// without calling `fit` again
var newMaxChartAreaHeight = height - totalTopBoxesHeight - totalBottomBoxesHeight;
var newMaxChartAreaWidth = width - totalLeftBoxesWidth - totalRightBoxesWidth;
var newMaxChartAreaHeight = height - outerBoxSizes.top - outerBoxSizes.bottom;
var newMaxChartAreaWidth = width - outerBoxSizes.left - outerBoxSizes.right;

if (newMaxChartAreaWidth !== maxChartAreaWidth || newMaxChartAreaHeight !== maxChartAreaHeight) {
helpers.each(leftBoxes, function(box) {
box.height = newMaxChartAreaHeight;
});

helpers.each(rightBoxes, function(box) {
helpers.each(verticalBoxes, function(box) {
box.height = newMaxChartAreaHeight;
});

helpers.each(topBoxes, function(box) {
if (!box.fullWidth) {
box.width = newMaxChartAreaWidth;
}
});

helpers.each(bottomBoxes, function(box) {
helpers.each(horizontalBoxes, function(box) {
if (!box.fullWidth) {
box.width = newMaxChartAreaWidth;
}
Expand All @@ -381,8 +345,8 @@ module.exports = {

function placeBox(box) {
if (box.isHorizontal()) {
box.left = box.fullWidth ? leftPadding : totalLeftBoxesWidth;
box.right = box.fullWidth ? width - rightPadding : totalLeftBoxesWidth + maxChartAreaWidth;
box.left = box.fullWidth ? leftPadding : outerBoxSizes.left;
box.right = box.fullWidth ? width - rightPadding : outerBoxSizes.left + maxChartAreaWidth;
box.top = top;
box.bottom = top + box.height;

Expand All @@ -393,8 +357,8 @@ module.exports = {

box.left = left;
box.right = left + box.width;
box.top = totalTopBoxesHeight;
box.bottom = totalTopBoxesHeight + maxChartAreaHeight;
box.top = outerBoxSizes.top;
box.bottom = outerBoxSizes.top + maxChartAreaHeight;

// Move to next point
left = box.right;
Expand All @@ -412,10 +376,10 @@ module.exports = {

// Step 8
chart.chartArea = {
left: totalLeftBoxesWidth,
top: totalTopBoxesHeight,
right: totalLeftBoxesWidth + maxChartAreaWidth,
bottom: totalTopBoxesHeight + maxChartAreaHeight
left: outerBoxSizes.left,
top: outerBoxSizes.top,
right: outerBoxSizes.left + maxChartAreaWidth,
bottom: outerBoxSizes.top + maxChartAreaHeight
};

// Step 9
Expand Down