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.

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

Top margin incorrect #12206

Closed
drmrbrewer opened this issue Oct 14, 2019 · 29 comments · Fixed by #17319
Closed

Top margin incorrect #12206

drmrbrewer opened this issue Oct 14, 2019 · 29 comments · Fixed by #17319

Comments

@drmrbrewer
Copy link

Take the following example:

https://jsfiddle.net/8wvkstxh/1/

The chart options include a y adjustment to the top xAxis labels, because, without it, the labels are too far from the axis itself (for my liking):

https://jsfiddle.net/8wvkstxh/2/

But with the y adjustment included (first example above), there is now a lot of dead space above the top x axis labels (I'm trying to create a chart layout which minimises dead margin space and maximises chart area, so I'm sensitive to this).

The top margin is seemingly "correct" for the default y position (without the y adjustment -- second example above), in that there is only a small gap between the labels and the edge.

But with the adjusted y position there is too much margin space.

It seems to me that the problem is that in order to set the y position to my liking (to move the labels down from their default position), I need a negative value for y.

But a negative y value actually should normally be pushing the labels up... so it seems that highcharts is thinking that more margin space will be required, and adds margin space to compensate for this, even though the labels are actually lower than without the negative adjustment (so that the opposite should have happened... the top margin should have been reduced, not increased).

Compare this with setting y explicitly to 0:

https://jsfiddle.net/sw36dqtp/

Therein lies the problem: setting y: 0 gives a very different result to not setting y at all. I need to set y to get the y position right, so I need to state a value, and I'm moving it up from y: 0 so I need to specify a negative value, which highcharts is apparently compensating for by adding more margin space which isn't needed. This leaves a lot of bloat in the top margin.

@pawelfus
Copy link
Contributor

Thanks for reporting the issue @drmrbrewer

In this particular design, you can set xAxis.labels.reserveSpace to false, take a look: https://jsfiddle.net/BlackLabel/rw1eag2h/

Internal note:
It looks like a bug in padding correction:

highcharts/ts/parts/Axis.ts

Lines 6651 to 6671 in e62d541

if (side === 0) {
lineHeightCorrection = -axis.labelMetrics().h;
} else if (side === 2) {
lineHeightCorrection = axis.tickRotCorr.y;
} else {
lineHeightCorrection = 0;
}
// Find the padded label offset
labelOffsetPadded = Math.abs(labelOffset) + titleMargin;
if (labelOffset) {
labelOffsetPadded -= lineHeightCorrection;
labelOffsetPadded += directionFactor * (
horiz ?
pick(
(labelOptions as any).y,
axis.tickRotCorr.y + directionFactor * 8
) :
(labelOptions as any).x
);
}

In short: http://jsfiddle.net/BlackLabel/0ou3rfa4/ - both (top/bottom) spacings are set to zero, yet only bottom labels are close enough to the border.

@drmrbrewer
Copy link
Author

drmrbrewer commented Oct 14, 2019

Thanks, @pawelfus. Is the reserveSpace trick an exact fix in my particular example, or does it just reduce the impact? Because my feeling is that I still want to be "reserving space" for the x axis labels at the top, just not quite so much as is the case at present! So setting reserveSpace to false seems wrong, even if it helps.

I also find that there is still too much blank space for my use case. Take the following example, with title added:

https://jsfiddle.net/5tb1xmvk/1/

Because I am trying to use every last bit of space, I have even reduced the line-height of the title. But the gap between title and chart is still too big for my liking. So I need to add a margin-bottom:-0.8em; to reduce it:

https://jsfiddle.net/5tb1xmvk/

Is the extra blank space in the first example (even with reserveSpace set to false) a consequence of this same issue, or is it just something I have to accept and compensate for through negative margin? Because if I remove the y adjustment to the title (without any negative margin), the gap seems a bit more appropriate:

https://jsfiddle.net/9dLqxga1/

@pawelfus
Copy link
Contributor

pawelfus commented Oct 15, 2019

It's a direct fix @drmrbrewer - instead of reserving space for labels automatically, you control it. Note that labels nicely fit in the space created by tickmarks. You can reduce extra space above by setting spacingTop (defaults to 10px) and spacingBottom (defaults to 15px), demo: https://jsfiddle.net/BlackLabel/m9uyvszf/

Extra space, in your demos with titles, is caused by title.margin (defaults to 15px). You can disable it, see: https://jsfiddle.net/BlackLabel/f93bn8ck/ - no need to play around with negative adjustments and line-height.

@drmrbrewer
Copy link
Author

labels nicely fit in the space created by tickmarks

@pawelfus aaah, I see. Yes, makes sense! Thanks.

Extra space, in your demos with titles, is caused by title.margin

ah yes, thanks... I really hated the negative margin kludge so I'm glad it's not necessary!

no need to play around with negative adjustments and line-height

but in my case I still need to reduce line-height because otherwise I find that the gap between title lines is too big, compare:

https://jsfiddle.net/p8z2476t/
https://jsfiddle.net/p8z2476t/1/

But this is really just particular to my use case where I'm trying to maximise actual chart size as far as possible.

@pawelfus
Copy link
Contributor

I still need to reduce line-height because otherwise I find that the gap between title lines is too big, compare:

Ah, in that case, yes, line-height is a good idea :)

@stale
Copy link

stale bot commented Jun 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

@stale stale bot added the Status: Stale This issue hasn't had any activity for a while, and will be auto-closed if no further updates occur. label Jun 3, 2021
@stale stale bot closed this as completed Jun 11, 2021
@drmrbrewer
Copy link
Author

drmrbrewer commented Apr 5, 2022

@pawelfus this issue was closed automatically but I don't see it as being completely resolved. I've been using the suggested solution, which has been working fine in general. Basically the solution was to set reserveSpace for the top xAxis labels to false, which seems to work only because some space is anyway being reserved for the ticks.

But now I'm in a situation where I want to make the format for the top axis labels customisable, so that I can no longer be sure how much space I need to reserve. Therefore, using the ticks to reserve space no longer works, but I also can't get the right amount of space to be reserved for the labels... or at least I can't adjust the y position of the labels without messing everything up.

Example: https://jsfiddle.net/y2pj7gmb/ (press Run multiple times to show the effect of different label height)

I have deliberately reduced tickLength so that it is insufficient to reserve enough space in any scenario.

@pawellysy pawellysy reopened this Apr 5, 2022
@stale stale bot removed the Status: Stale This issue hasn't had any activity for a while, and will be auto-closed if no further updates occur. label Apr 5, 2022
@pawelfus
Copy link
Contributor

Hey @drmrbrewer
Thank you for the update. In this case (customizable format), I agree you can't use reserveSpace = false and rely on ticks' created space. I have removed the code responsible for extra space, awaiting visual tests results (draft).

You can test this branch: <script src="https://github.highcharts.com/7ba376b/highcharts.js"></script>

Your demo without workarounds (negative title.offset and labels.y): https://jsfiddle.net/BlackLabel/7krvqmhx/

@drmrbrewer
Copy link
Author

drmrbrewer commented Apr 11, 2022

Thanks, @pawelfus. The demo seems to suffer from a similar problem to my original post, in that it seems to be impossible to adjust the y position of the labels, because setting an explicit value for y messes up the spacing.

I'm trying to reduce the gap shown here:

image

So I try to add a small positive y adjustment to your demo (the only change), and it results in the following:

https://jsfiddle.net/btx5qhwj/

I'm used to being able to adjust the x position of the labels on the yAxis and it seems to work (reserving the right amount of space, taking account of the adjustment), but here it doesn't.

@pawelfus
Copy link
Contributor

Right, back to square one... I think I got it: https://jsfiddle.net/BlackLabel/o5wcehyv/1/
Now y: 0 and y: undefined are the same, so any positive y-offset will push to the bottom and negative y-offset will push to the top. In the demo, I added title.margin = 0 to reduce the space between the title and xAxis labels.

@drmrbrewer
Copy link
Author

@pawelfus this is looking good 👍

@drmrbrewer
Copy link
Author

drmrbrewer commented Apr 12, 2022

@pawelfus there still seem to be some quirks.

You say that "Now y: 0 and y: undefined are the same"... this seems to be so for the top (opposite) xAxis... or mostly so (see comment right at bottom of this post).

But now I'm now also looking at the behaviour of the labels on the bottom xAxis.

Check the following example, which uses the same label setup for top and bottom:

https://jsfiddle.net/bqcj163u/

Hit 'Run' several times... this now changes the y for the labels randomly between 0 and undefined, and this does change the layout significantly at the bottom (see gap between labels and chart edge):

image

vs:

image

This (i.e. whether y of bottom labels is 0 or undefined) also seems to have a small effect on the layout at the top (see highlighted gap below, between labels and title):

image

vs:

image

The following is an example with fixed label format, and changing just y of top labels between 0 and undefined for each Run, and again there is a slight but clear shift:

https://jsfiddle.net/fm6zpcgL/

Sorry if this seems like pedantic pixel-peeping but in my use-case the positioning needs to be very precise (and predictable) because space is very limited and the chart features and axis labels etc are packed in as compactly as possible :-)

@drmrbrewer
Copy link
Author

@pawelfus any further thoughts on this?

@pawelfus
Copy link
Contributor

pawelfus commented May 17, 2022

Hi @drmrbrewer

My apologies for not replying sooner. I will try a different approach next week. It's a quite tricky case, because I don't want to cause any regressions (current PR has plenty of them..)

@drmrbrewer
Copy link
Author

Hi @pawelfus... no problem, and I completely understand that it is tricky for you, potentially with lots of unintended consequences for any change you might make. As I mentioned, adjusting the x position of the labels on the yAxis (or yAxes) seems to be very well behaved (somehow magically reserving the right amount of space regardless of how complicated my chart is), but with the y position of labels on the xAxis (or xAxes) it seems a little broken somehow.

@pawelfus
Copy link
Contributor

Hi @drmrbrewer
I have tried some different solutions and I must stick to the first idea (improved a little).

Hit 'Run' several times... this now changes the y for the labels randomly between 0 and undefined, and this does change the layout significantly at the bottom (see gap between labels and chart edge):

Fixed, my changes don't affect the bottom xAxis anymore

This (i.e. whether y of bottom labels is 0 or undefined) also seems to have a small effect on the layout at the top (see highlighted gap below, between labels and title):

I can't change it. After digging further, I consider this is a bug with undefined, which miscalculates available space by ~3px. However, "fixing" it generates ~1k failing tests because since ~2015 we assumed this ~3px offset is correct.

Current solution: https://jsfiddle.net/BlackLabel/h6twmcr1/11/

@drmrbrewer
Copy link
Author

@pawelfus the y positioning of the bottom axis label is a bit strange now isn't it?

See this example: https://jsfiddle.net/yjhq836o/

It's the same as your recent example, but using the same labels object for top and bottom (not just top). Look at the y position of the bottom labels now for column 1 vs 2 vs 3.

@pawelfus
Copy link
Contributor

@pawelfus the y positioning of the bottom axis label is a bit strange now isn't it?

I see no difference between the current version and my PR. My PR should change rendering top xAxis labels only, all other axes should remain unchanged

@drmrbrewer
Copy link
Author

@pawelfus OK fair enough, in which case it seems like a big improvement 👍... but is a fix also required for the bottom xAxis labels, in that case?

@pawelfus
Copy link
Contributor

I might be missing something, but in the top labels, the problem was with the unpredictable height of the labels, right? As a result of this issue, using labels.y was problematic with multiline texts.

As for the bottom xAxis. default labels.y adapts to the font size of the label (see docs).

pawelfus added a commit that referenced this issue May 26, 2022
@drmrbrewer
Copy link
Author

@pawelfus maybe I'm missing something myself, but my understanding is that the effect of labels.y should be the same for both top and bottom xAxis... after all they are both the same type of axis, with one being opposite.

But for the bottom xAxis this is what we see for labels.y = 0 (middle column):

image
image
image

... and this is what we see for labels.y = undefined (right-most column):

image
image
image

So for the bottom xAxis, the y positioning of the labels is different for labels.y = 0 (middle column) and labels.y = undefined (right-most column).

But for the top xAxis, the y positioning of the labels is the same for labels.y = 0 (middle column) and labels.y = undefined (right-most column):

image
image
image

So why does labels.y behave differently for top and bottom?

@drmrbrewer
Copy link
Author

drmrbrewer commented May 31, 2022

@pawelfus it's looking pretty good so far! There's still a problem I'm struggling with though, and it may or may not be related to this particular issue, but in any case I wonder whether you had some insight:

https://jsfiddle.net/mt7fbkpy/

In the above example, the third argument to your render() function now controls the font-size used for the xAxis labels (not the labels.y value as before).

If you look across each row (font size increasing) you can see that the gap between the labels and the xAxis is increasing, and the gap between the labels and the title is decreasing:

image
image
image

Ideally I'd like the axis labels to be positioned with vertical gaps above/below that are constant, or at least consistent (i.e. both increasing/decreasing with font size at the same rate)... in my use case the font size is variable and therefore it needs to look right regardless of font size.

At the moment the only way I can see to make positioning consistent across different font sizes is to add a horrible correction to labels.y and title.margin based on font size (the corrections applied in the example below are just determined by trial and error):

https://jsfiddle.net/492hwqxj/

It looks a little better, but is there a way to make this positioning better, out of the box?

@drmrbrewer
Copy link
Author

@pawelfus just wondering if you have any thoughts on my last couple of comments?

@raf18seb
Copy link
Contributor

raf18seb commented Jun 9, 2022

Hi @drmrbrewer, sorry for the late reply.

The y offset doesn't take the font size into consideration because the labels have CSS styles without useHTML flag. What about this? https://jsfiddle.net/BlackLabel/16np3gq9/

@drmrbrewer
Copy link
Author

@raf18seb so it was literally just a case of adding useHTML: true?

@raf18seb
Copy link
Contributor

raf18seb commented Jun 9, 2022

It looks like that's the case :)

@drmrbrewer
Copy link
Author

The margins and positioning of the axis labels are now behaving really well, and consistently... thanks for the fix @pawelfus :)

@drmrbrewer
Copy link
Author

drmrbrewer commented Jun 10, 2022

@raf18seb is there any way to change things so that the y offset does take the font size into consideration, even without useHTML: true? Setting useHTML: true breaks something else in my code (see here in case you're interested!).

It's possible I can work around it but I thought I'd just check if there was any way that the core highcharts could position the labels more accurately in this situation even without useHTML: true? Seems that it would be ideal if the positioning was correct, whether useHTML is set to true or false?

@raf18seb
Copy link
Contributor

@drmrbrewer, unfortunately, you're setting the different CSS font-size styles on your span elements in the xAxis.labels.format so, to make it work properly, your labels should be flagged as HTML elements.

I'd also go for the getBBox() as suggested on the Forum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants