Make legend layouting respect padding at the start of columns

[jtagscherer]

The draw function of chart legends introduces a vertical padding at the very beginning of each column:

Chart.js/src/plugins/plugin.legend.js

Lines 420 to 424 in 5816770

	cursor = {
	x: me.left + labelOpts.padding,
	y: me.top + labelOpts.padding,
	line: 0
	};

Before individual items of a legend are being drawn, the fit function performs some layouting. This includes the calculation of how many columns of items will be needed to fit all items into a vertical legend. However, this calculation does not consider the aforementioned initial column padding. Instead, all new columns are assigned a height of 0:

Chart.js/src/plugins/plugin.legend.js

Lines 272 to 303 in 5816770

	var vPadding = labelOpts.padding;
	var columnWidths = me.columnWidths = [];
	var totalWidth = labelOpts.padding;
	var currentColWidth = 0;
	var currentColHeight = 0;
	var itemHeight = fontSize + vPadding;
	helpers.each(me.legendItems, function(legendItem, i) {
	var boxWidth = getBoxWidth(labelOpts, fontSize);
	var itemWidth = boxWidth + (fontSize / 2) + ctx.measureText(legendItem.text).width;
	// If too tall, go to new column
	if (currentColHeight + itemHeight > minSize.height) {
	totalWidth += currentColWidth + labelOpts.padding;
	columnWidths.push(currentColWidth); // previous column width
	currentColWidth = 0;
	currentColHeight = 0;
	}
	// Get max width
	currentColWidth = Math.max(currentColWidth, itemWidth);
	currentColHeight += itemHeight;
	// Store the hitbox width and height here. Final position will be updated in `draw`
	hitboxes[i] = {
	left: 0,
	top: 0,
	width: itemWidth,
	height: fontSize
	};
	});

In some edge cases with specific amounts of legend items and chart heights, this lead to overflow bugs as seen in #5491. This pull request fixes this issue.

Fixes #5491

[simonbrunel]

I think I understand the issue but I'm not sure about the fix: currentColHeight should start from 0 because when there is no item, the height should be 0. However, it seems that we don't check currentColHeight against the correct layout item (maximum) height.

What about these changes:

var maxHeight = minSize.height - vPadding;

// ...

helpers.each(me.legendItems, function(legendItem, i) {
    // ...

    if (currentColHeight + itemHeight > maxHeight) {
       //...

Does that fix the issue?

[jtagscherer]

Going into this direction would work as well. However, rather than adding the padding to the maxHeight, we would need to subtract it to account for the initial column padding. Therefore, the first line from your comment would need to be changed to:

var maxHeight = minSize.height - vPadding;

@simonbrunel Reverting my previous changes and going with this approach fixes this issue as well. What do you think? If you think we should go with this approach, I will commit the necessary changes.

[simonbrunel]

@jtagscherer you right, I fixed my previous comment, it should be minSize.height - vPadding;

I would go with the approach I suggested. Do you confirm that it fixes #5491?

[jtagscherer]

@simonbrunel Thank you for your suggestions, I have changed the code accordingly. Using a slightly adjusted version of the chart provided by the issue's author for testing, I can confirm that these changes fix #5491.

[simonbrunel]

@jtagscherer can you try to add a unit test for this case to prevent regression?

[jtagscherer]

@simonbrunel Sure thing! Should I add the unit test to this pull request or should I create a new one?

[simonbrunel]

In this PR but you will need to wait a bit since GitHub have some issues.

[jtagscherer]

Unfortunately, upon further testing and trying to create a minimal example for the unit test, this issue turned out to be of a larger scale than I expected. My changes did not fix the problem entirely. I'll try to outline it and offer some possibilities.

The draw function actually uses the legend's top and bottom attribute to decide whether a new column with legend items should be started. The fit function tries to get away with using the minSize for the same purpose, which does not work out in some cases. On top of that, the top and bottom attributes are different when accessed within the fit function. I strongly suspect that that is the case because of the function's position within the lifecycle, meaning that final chart layouting results are not available yet when fit is called.

Is there any reason why we have to calculate the amount of columns twice, once within fit and once within draw? Could we not do this just once within the draw function, since final layouting results are available here?

Alternatively, we could move the fit function to another position within the lifecycle and use top, bottom, left, and right attributes instead of the minSize. Doing so, we could guarantee that the column calculation results of both functions do not diverge. Calling fit from the afterLayout callback would work.

@simonbrunel What do you think? How should we go about this?

[simonbrunel]

I'm really not familiar with the layout code, maybe @etimberg have more ideas on this issue?

[nagix]

I was originally trying to solve different legend layout issues in #5816, but realised that this PR is addressing a similar one. My solution to the fit/draw issue in that PR is to use minSize in the draw function. As recalculating the legend box every time the chart area size is changed will introduce more complexity, I just used the same values used in the fit function. The final result may not be perfect, but it is unnoticeable in most cases because of padding in the legend.

[jtagscherer]

@nagix You are right, that seems to be the root cause for both of our issues, while my changes only addressed the symptoms. I tested the version of your pull request against my two test cases, and it fixed the issue in both of them. Therefore, I'd suggest that we go with your approach and fix this issue at its root.

Before that, it would be great if a maintainer that is familiar with the layout code could step in and explain why the current architecture with two diverging layout routines was chosen. Maybe there's a good reason for this, and our solution could cause even more problems.

Finally, I think it would be a good idea to add another test case against the specific issue that I was addressing. That way, we can ensure that there will be no regression. @nagix I already have a local finished test case that I could add. I could commit it if you give me write access to your pull request's forked repository, or I could create a pull request for your repository with my changes. Whatever works best for you.

[nagix]

I think the calculation in fit is needed because the height of the legend box is determined by the size of other boxes but the width can change depending on the legend items (in case of 'left' and 'right' positions). The problem is that the result of the fit function also affects the positions and sizes of other boxes, so we need to stop calculation at some point.

@jtagscherer Can you create a pull request for my repository? Then, I will merge it.

[simonbrunel]

@jtagscherer @nagix can we close that PR in favor of #5816?

[jtagscherer]

@simonbrunel Sorry for the delay. Yes, you can close this PR in favour of #5816. I would have added my unit test for the specific use case that the author of #5491 had an issue with to #5816 today, but I see that you have already merged it. That's not too bad though, since @nagix already added test cases for legends with multiple columns. We just won't have a regression test against that specific edge case in #5491.

[simonbrunel]

@jtagscherer thanks for the update (and your contribution) :)

I totally forgot about the extra unit tests :\ sorry for that. @nagix, does your PR include regression tests for case described in #5491? If not and if it makes sense, @jtagscherer feel free to rebase and update this PR with these tests.

[jtagscherer]

@simonbrunel No worries! I'll rebase this PR to include my regression tests covering the specific situation from #5491 and then we can decide whether they are worth the addition. I'd say that you can never have enough regression tests :)

[jtagscherer]

@simonbrunel I have added a regression test for a simplified version of the issue in #5491. I have checked and can confirm that this test fails when run against the old version without #5816, but succeeds on the fixed version.

[nagix]

Thanks @jtagscherer, this looks good to me.

[simonbrunel]

Thanks @jtagscherer