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

Asymmetric datalabel padding #6213

Closed
drmrbrewer opened this Issue Jan 6, 2017 · 10 comments

Comments

Projects
None yet
2 participants
@drmrbrewer

drmrbrewer commented Jan 6, 2017

Expected behaviour

With a bounding box around the data label, would expect uniform padding around the text.

Actual behaviour

More space below than above, at least in Chrome.

Live demo with steps to reproduce

http://jsfiddle.net/yp813csb/

Chrome (bad):
capture

IE (good):
captureie

I have found that enabling html improves the formatting in Chrome:

http://jsfiddle.net/yp813csb/2/

capturechome2

Affected browser(s)

See above. At least Chrome. And using phantomjs on the server... the same asymmetry is seen.

Possible solutions?

This is the same as closed issue #4153. But it's still really really bugging me that I can't get something as simple as a datalabel looking good, on what is surely a very common browser (Chrome, both on desktop and mobile)... particularly if you're developing for Android! The existing data labels just look really bad IMHO.

I tried the workaround mentioned in the above issue thread, but it got really messy. Instead I just enable html, which as mentioned above does improve the formatting... but it leads to another problem which is that the html floats above svg as mentioned at http://www.highcharts.com/docs/chart-concepts/labels-and-string-formatting#html).

Anyway I think that a messy workaround shouldn't be necessary for such a simple component.

If it's not possible for the highcharts code to determine the spacing properly, can you at least provide separate paddingTop, paddingRight, paddingBottom and paddingLeft options for the data label, rather than just a single padding option as at present? That way, the user can choose to use asymmetric padding to compensate, and thereby create a uniform padding for a particular environment?

Ideally, the separate padding options would allow you to specify a negative padding too, in case you want zero padding at top, left, right, and then you'd need negative padding at the bottom due to the extra padding there by default.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Jan 9, 2017

Collaborator

Hi, the problem is basically the same as #1101 (for IE), so I added Chrome to the same fix. See the demo with the fix applied.

Collaborator

TorsteinHonsi commented Jan 9, 2017

Hi, the problem is basically the same as #1101 (for IE), so I added Chrome to the same fix. See the demo with the fix applied.

@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer Jan 9, 2017

Is a separate fix required for rendering via phantomjs?

drmrbrewer commented Jan 9, 2017

Is a separate fix required for rendering via phantomjs?

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Jan 9, 2017

Collaborator

Apparently this doesn't happen with PhantomJS. See http://export.highcharts.com/ and Preview with the configuration object from your fiddle.
skjermbilde 2017-01-09 kl 10 40 40

Collaborator

TorsteinHonsi commented Jan 9, 2017

Apparently this doesn't happen with PhantomJS. See http://export.highcharts.com/ and Preview with the configuration object from your fiddle.
skjermbilde 2017-01-09 kl 10 40 40

@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer Jan 9, 2017

OK. I think that maybe the problem I'm still seeing when rendering with PhantomJS on the server is with specific font families, for example TeX Gyre Adventor (otf) specified in the js via chart.style.fontFamily. It's appearing too high in the label box:

capture

Is there any way I can move it down? This is perhaps where individual paddingTop, paddingRight, paddingBottom and paddingLeft options for the data label would be handy, so that I could e.g. specify zero padding for top, left, and right, and a negative padding for bottom.

Enabling useHtml for the label does make the text more central (vertically), which is what I'm using at present to fix this, but as mentioned before it does lead to the problem that the html text floats above the svg elements. Is the placement of label text within the label box treated differently for html compared to non-html... why is it better (for me) when using html?

drmrbrewer commented Jan 9, 2017

OK. I think that maybe the problem I'm still seeing when rendering with PhantomJS on the server is with specific font families, for example TeX Gyre Adventor (otf) specified in the js via chart.style.fontFamily. It's appearing too high in the label box:

capture

Is there any way I can move it down? This is perhaps where individual paddingTop, paddingRight, paddingBottom and paddingLeft options for the data label would be handy, so that I could e.g. specify zero padding for top, left, and right, and a negative padding for bottom.

Enabling useHtml for the label does make the text more central (vertically), which is what I'm using at present to fix this, but as mentioned before it does lead to the problem that the html text floats above the svg elements. Is the placement of label text within the label box treated differently for html compared to non-html... why is it better (for me) when using html?

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Jan 9, 2017

Collaborator

Is there any way I can move it down? This is perhaps where individual paddingTop, paddingRight, paddingBottom and paddingLeft options for the data label would be handy

It would probably be handy, but it would still only be a workaround to a browser issue - also, it would be wrong in Firefox that doesn't have the same problem with bounding boxes.

The workaround in the commit above is specific to the default font, so I think for a complete solution we would need some kind of extendable mapping for corrected bounding boxes. Something along these lines:

SVGRenderer.prototype.bBoxCorrections = {
   '"Lucida Grande", "Lucida Sans Unicode", Arial, Helvetica, sans-serif': [
      ['11px', 17, 14]
   ]
}

.... where 11px is the font size setting, 17 is the browser's reported box height and 14 is the corrected box height. Then you would be able to define your custom corrections like this:

SVGRenderer.prototype.bBoxCorrections['TeX Gyre Adventor'] = [
   ['11px', 16, 14],
   // ... etc for other font sizes
];
Collaborator

TorsteinHonsi commented Jan 9, 2017

Is there any way I can move it down? This is perhaps where individual paddingTop, paddingRight, paddingBottom and paddingLeft options for the data label would be handy

It would probably be handy, but it would still only be a workaround to a browser issue - also, it would be wrong in Firefox that doesn't have the same problem with bounding boxes.

The workaround in the commit above is specific to the default font, so I think for a complete solution we would need some kind of extendable mapping for corrected bounding boxes. Something along these lines:

SVGRenderer.prototype.bBoxCorrections = {
   '"Lucida Grande", "Lucida Sans Unicode", Arial, Helvetica, sans-serif': [
      ['11px', 17, 14]
   ]
}

.... where 11px is the font size setting, 17 is the browser's reported box height and 14 is the corrected box height. Then you would be able to define your custom corrections like this:

SVGRenderer.prototype.bBoxCorrections['TeX Gyre Adventor'] = [
   ['11px', 16, 14],
   // ... etc for other font sizes
];
@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer Jan 9, 2017

Custom corrections like that would be awesome. Would it be possible to specify a correction that was applicable to all font sizes, rather than (or as an option instead of) having to do it for all possible px values... particularly since px values are not restricted to integer values where the font size is determined programatically (but I suppose that integer px values could be enforced in the code). For example just specifying -10 (%) would be equivalent to ['6px', 10, 9], ['8px', 12, 10.8] etc.

drmrbrewer commented Jan 9, 2017

Custom corrections like that would be awesome. Would it be possible to specify a correction that was applicable to all font sizes, rather than (or as an option instead of) having to do it for all possible px values... particularly since px values are not restricted to integer values where the font size is determined programatically (but I suppose that integer px values could be enforced in the code). For example just specifying -10 (%) would be equivalent to ['6px', 10, 9], ['8px', 12, 10.8] etc.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Jan 9, 2017

Collaborator

Yes a general formula would be great. It could potentially eliminate all pixel-level differences between browsers, except the glyphs themselves. I'll have to leave now, but here's something we can use for comparison: http://jsfiddle.net/highcharts/em37nvuj/

It seems like no two browsers report the same bounding boxes. Even Chrome and Safari on Mac are different.

Collaborator

TorsteinHonsi commented Jan 9, 2017

Yes a general formula would be great. It could potentially eliminate all pixel-level differences between browsers, except the glyphs themselves. I'll have to leave now, but here's something we can use for comparison: http://jsfiddle.net/highcharts/em37nvuj/

It seems like no two browsers report the same bounding boxes. Even Chrome and Safari on Mac are different.

@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer Jan 9, 2017

Hmm, interesting... that makes it easier to see why this is so tricky.

Times on Chrome:
chrome-times
Times on IE:
ie-times

So bbox dimensions are direct from the browser, untouched by highcharts? Chrome seems to stick to integer px heights, unlike IE. And (maybe as a result of rounding), Chrome gives the same bbox height for 13px and 14px... both can't be right! Or maybe it is using a different bbox height (float) to what it is reporting (rounded to int). Not sure how on Earth one can find a pattern in there, sufficient to derive a general formula!

Can highcharts not work out for itself what the text height is? Is this handled differently for html labels (which in my limited experience seems to render better more consistently), or does highcharts still rely on what the browser is telling it?

drmrbrewer commented Jan 9, 2017

Hmm, interesting... that makes it easier to see why this is so tricky.

Times on Chrome:
chrome-times
Times on IE:
ie-times

So bbox dimensions are direct from the browser, untouched by highcharts? Chrome seems to stick to integer px heights, unlike IE. And (maybe as a result of rounding), Chrome gives the same bbox height for 13px and 14px... both can't be right! Or maybe it is using a different bbox height (float) to what it is reporting (rounded to int). Not sure how on Earth one can find a pattern in there, sufficient to derive a general formula!

Can highcharts not work out for itself what the text height is? Is this handled differently for html labels (which in my limited experience seems to render better more consistently), or does highcharts still rely on what the browser is telling it?

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Jan 10, 2017

Collaborator

So bbox dimensions are direct from the browser, untouched by highcharts?

That's correct, in this jsFiddle it comes unmodified from the DOM.

I have added pure HTML boxes to the jsFiddle. The only difference here as far as I can see is that IE rounds off these numbers for the offsetHeight.

Can highcharts not work out for itself what the text height is?

We need to measure the actual box size to get label widths, but in theory we may be able to use logic to sort out the line heights instead of reading how the browser renders it. It would be great if we could have absolute consistency between browsers. The challenge then would be to compute the bounding box height of multiline text, but these too could probably be built by adding the heights of each tspan that the lines are made of. I did an implementation like this in the highcharts-jsdom experiment a couple of years ago (jsdom doesn't handle bounding boxes), but it should be noted it is risky business and by no means production ready.

Collaborator

TorsteinHonsi commented Jan 10, 2017

So bbox dimensions are direct from the browser, untouched by highcharts?

That's correct, in this jsFiddle it comes unmodified from the DOM.

I have added pure HTML boxes to the jsFiddle. The only difference here as far as I can see is that IE rounds off these numbers for the offsetHeight.

Can highcharts not work out for itself what the text height is?

We need to measure the actual box size to get label widths, but in theory we may be able to use logic to sort out the line heights instead of reading how the browser renders it. It would be great if we could have absolute consistency between browsers. The challenge then would be to compute the bounding box height of multiline text, but these too could probably be built by adding the heights of each tspan that the lines are made of. I did an implementation like this in the highcharts-jsdom experiment a couple of years ago (jsdom doesn't handle bounding boxes), but it should be noted it is risky business and by no means production ready.

@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer Jan 10, 2017

Look forward to developments on this. BTW I think your [to the jsFiddle] link needs updating to refer to the jsfiddle URL http://jsfiddle.net/highcharts/em37nvuj/ [now done].

drmrbrewer commented Jan 10, 2017

Look forward to developments on this. BTW I think your [to the jsFiddle] link needs updating to refer to the jsfiddle URL http://jsfiddle.net/highcharts/em37nvuj/ [now done].

TorsteinHonsi added a commit that referenced this issue Jan 10, 2017

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