Fixes labelOffset not working for vertical axes

[suheb]

Signed-off-by: Suhaib Khan suheb.work@gmail.com

Fixes #3889
Example: https://codepen.io/anon/pen/EmEzKb

[etimberg]

@suheb would it be possible to add some tests for this?

[suheb]

@etimberg Not sure how to test this as it effects the position where labels are drawn on chart. Are there any existing tests which check something similar to this? Maybe that'll me on this.

[etimberg]

there are some tests in https://github.com/chartjs/Chart.js/blob/master/test/specs/plugin.filler.tests.js that compare images generated from the canvas. This would be a good place to use that kind of tests. The baseline images are stored in https://github.com/chartjs/Chart.js/tree/master/test/fixtures/plugin.filler

[simonbrunel]

Everything explained in #3988

[suheb]

@simonbrunel Thanks for the link, really helpful.
@etimberg I've added a basic test for this. Let me know if anything else is needed :)

[suheb]

@simonbrunel The test passes when I run the it using gulp unittest --watch --inputs=test/specs/{spec.name}.js. Not sure why it's failing in the travis build.

[simonbrunel]

@suheb it fails for me locally:

Most of the time it's because of the font rendering that differs between platform/browser.

Setting tolerance to 0.004 in label-offset-vertical-axes.json works for me:

{
    "tolerance": "0.004",
    "config": {
        // ...
}

[simonbrunel]

I would remove that options because it's not used by this test

[simonbrunel]

0.008 might be too high, did you verify locally that the test fails if you temporary change "labelOffset": 25 to 20? Maybe it worths a try to 0.005, the lower the better :)

@etimberg I'm curious to know if there is a font that can be consistently rendered on all platforms/browsers that we could use for our unit tests.

[etimberg]

@simonbrunel not sure about a specific font. Maybe something monospace?

[suheb]

@simonbrunel 0.008 is indeed too high. But the test does not fail (which it should) even at 0.004 if I change "labelOffset" from 25 to 20 for short labels like "1", "2", "3", "4".

[simonbrunel]

Not sure what to do then :\ Unit tests must fail if labels are not correctly positioned, if it doesn't fail with short labels, simply use long labels to make the test relevant. Ideally, tolerance should be 0 (default is 0.001), so pick the lowest value (with long labels) that passes on Travis but that easily fail if labelOffset changes.

[suheb]

@simonbrunel I tried using long labels but then the test was failing even at high tolerance due to different font rendering on browsers/platform. So I tried replacing text with unicode shapes instead and it's working fine for me locally on firefox and chrome (macOS 10.2.3) but still failing on travis. Moreover, I can't see the build log for the latest commit. :/

[etimberg]

@simonbrunel is this good to merge?

[simonbrunel]

It fails locally and it seems because of the aliasing of the grid lines. @suheb that's weird you don't get the same result. I suspect the calls to helpers.aliasPixel, may be better to also remove the grid line at zero?

[suheb]

@simonbrunel That's weird, seems to work fine on my local system. Anyway, I've removed the grid lines at zero to make it better. I hope this works fine now :)

[suheb]

@simonbrunel @etimberg Is this ready to be merged? Let me know if any other changes are needed.

[simonbrunel]

@suheb sorry for the delay, looks good to me, can you just remove: test/fixtures/core.scale/label-offset-vertical-axes.json~ (the one with the extra ~)?

[suheb]

@simonbrunel Ah! Not sure how that file got added. Just removed it :)

[simonbrunel]

Thanks!