[BUG] Tick text extents not calculated correctly

[mMerlin]

Expected Behaviour

No space should be included in the chart layout for scaleLabel axis content, when no scaleLabel is being displayed.

Current Behaviour

Since PR #4268 (Time axis ticks formatting for base and senior unit) from @hurskiy-andriy, the chart body is being positioned on the canvas as if axis scaleLabels where being displayed. Untested, but this may only occur when using a time type axis.

Possible solution

I have looked at the code changes introduced by the PR, but so far have not seen anything obvious that introduced the new result.

Steps to reproduce

• Fiddles using the same code to create the Chart, but builds from different master branch commits
  • fiddle with build from 3a2884f (previous to Time axis ticks formatting for base and senior unit #4268)
  • fiddle with build from Time axis ticks formatting for base and senior unit #4268 2d7c1f0
  • fiddle with build from latest master branch commit f16d8a3
• A simple scatter plot with time axis

var graphPoints = [
  {x: 1496998541000, y: 0},
  {x: 1496998541000, y: 7.5},
  {x: 1497007069000, y: 15},
  {x: 1497009140000, y: 22.5},
  {x: 1497009255000, y: 30},
  {x: 1497009300001, y: 37.5}
];

var sensorGraph = new Chart(document.getElementById("myCanvas").getContext("2d"), {
    type: "scatter",
    data: {
        datasets: [{
            data: graphPoints
        }]
    },
    options: {
        scales: {
            xAxes: [{
                type: "time",
            }]
        }
    }
});

Context

2d7c1f0 #4268 introduced a problem with layout for scale labels on Jun 15 14:20:16 2017 . The space needed to place them is being reserved, even when they are not displayed. This is true if no Axis scaleLabel option information is included, or if that is explicitly set to display: false. It looks correct if a scaleLabel is displayed, and seems to use exactly the same amount of space that is being left blank without the scaleLabel.

Environment

• Chart.js after v2.6.0
• Firefox 54.0 (64-bit) on Fedora 24

[andig]

@mMerlin I've had a quick look at 2d7c1f0 and can find absolutely nothing in that commit that should cause this. Are you positive that's the relevant commit?

[mMerlin]

I could not see anything in the diffs that would cause the symptom either. But that is where my binary search regression tests lead me.

The relevant from the following is:

2d7c1f0 Time axis tick formatting with major and minor units (#4268)
3a2884f Fixed tiny typo in title.md

2 of the fiddles are referencing chartjs builds from those 2 commits, and that is where I see the difference show up. Unless of course I managed to mess up my checkout, build, copy that created the .js files used with the fiddles.

$ git status
On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working directory clean
$ git log --oneline v2.6.0..
f16d8a3 Fix links in documentations (#4477)
463a8dd Simplify formulas based on code review
56050dc Move easing effects in separate file + unit tests
e8ebb46 Update link to documentation for previous versions
6e54bc5 Clip chart area before filling - Add clip and unclip around doFill() call - This fixes #4450
9ec78ce Add a note on how to use getElementAtEvent in a click handler
7d60857 Use proper reverse option in radial linear scale
cc9e88a Support an array for line chart pointBorderWidth
225bfd3 Rewrite the clone and merge helpers (#4422)
6f31713 Clamp radius when drawing rounded rectangle (#4448)
e5a431e Remove `.js` extensions when requiring a file (#4427)
ecca337 Increase ESLint complexity and add config for tests (#4421)
0eedec3 replace self closing script tag with open and closing tags
7f15beb ticks.padding option applies to both vertical and horizontal axes
ccb2898 When all datasets are hidden, the linear scale defaults to a range of 0 - 1. If `ticks.min` was set this would not set the range correctly. Added a test to cover this case as well
548edc6 Fix non-passive event listener warning in Chrome
7fa6052 Update scatter chart default config to hide lines
8834bab Ensure deprecated unitStepSize property of time scale is respected (#4401)
5d95280 Change `valueAtIndexOrDefault` behavior (#4423)
5196e05 Cleanup and reorganize core and canvas helpers
6c82c93 Fix error when legend label options are not defined (#4402)
e543f87 Update Display Format table - Up to date with the latest code - Added Example column
2a7df85 Add aria-hidden=true attribute to hidden iframe for resizing (#4400)
18707cf Line height setting for scale titles. The text is centered within the line height, so setting the line height to a size greater than the font size moves it away from the axis edge.
9619110 Fix arguments in plugin interface description * Fixed arguments in IPlugin#before/afterDatasetUpdate description * Fixed arguments in IPlugin#before/afterDatasetDraw description
bef44cc Ensure that chart dimensions are always >= 0
b548d1a Add description on new dataset update and draw plugin hooks
46c0455 Wording error
de0ea5c Multiple lines of text in the chart title
7a02d93 Add note regarding non-existant fonts
28da154 HighCharts is not open source (#4383)
2d7c1f0 Time axis tick formatting with major and minor units (#4268)
3a2884f Fixed tiny typo in title.md
009ae4d Support hover animation duration during updates (#4300)
5e58114 Fix filling between datasets of different lengths
9e6a611 Fix Tiny Typo in Labelling.md
7423c48 Fix round option for time scales
9b8c24c Make sure that the border width of the tooltip color box is always correct
2a9a57e Use the latest 6.x LTS release for Travis builds (#4346)
7ee8da9 Fix broken link on animations page #4324
a930830 Fix vertical alignment of legend labels (#4318)
fd49ac9 Provide a blank default global layout option (#4319)
8eee124 Added 'devicePixelRatio' option to override the window's DPR setting (#4270)
0d1f68c Fix incorrect unitStepSize option. It should be stepSize (#4320)
fa6be2f Fix inconsistent aspect ratio
f7f177f Implemented aligment by senior unit in time axis. (#4267)
dab0a7f Fix onHover event not being triggered (#4297)
394382b Add tooltip textLabelColor callback (#4199)
2258199 Add hard coded integer constants for *_SAFE_INTEGER which are not available on IE
7579198 Upgrade dependencies
1cbba83 Refactor time scale methods into a common location
04ab523 Upgrade dependencies (#4272)
ea725c0 Fix code climate badge and link (#4277)
6ecad0b Attempt to fix the failing deploy step

[mMerlin]

Looks like my process was good.

$ git checkout 3a2884f
Note: checking out '3a2884f'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at 3a2884f... Fixed tiny typo in title.md
$ gulp build
[12:10:35] Using gulpfile ~/development/repos/Chart.js/gulpfile.js
[12:10:35] Starting 'build'...
[12:10:59] Finished 'build' after 24 s
$ diff dist/Chart.bundle.js ../../projects/libLinks/chartjs-regression/Chart.bundle-3a2884f.js 
$ git checkout 2d7c1f0
Previous HEAD position was 3a2884f... Fixed tiny typo in title.md
HEAD is now at 2d7c1f0... Time axis tick formatting with major and minor units (#4268)
[phil@caracal Chart.js]$ gulp build
[12:13:03] Using gulpfile ~/development/repos/Chart.js/gulpfile.js
[12:13:03] Starting 'build'...
[12:13:27] Finished 'build' after 24 s
$ diff dist/Chart.bundle.js ../../projects/libLinks/chartjs-regression/Chart.bundle-2d7c1f0.js

The only thing I saw that was at all suspicious, was the scale.mergeTicksOptions(); added to core.controller.js, but I see nothing in the code that should change the layout calculations.

[etimberg]

Ok, this problem isn't the scale label it's the tick strings. There must be some longer string that is measured and then not used but is used when calculating the length of the longest tick label. If I force the rotation to 0 then it goes away https://jsfiddle.net/w9c5ta7u/

[mMerlin]

That should narrow where to look, but it still seems to have been introduced by this PR. If I change the .chartWrapper css width to 470px in the fiddle of the 3a2884f (before) commit, it rotates the ticks while still keeping a reasonable spacing. Maybe a little more margin than needed on the bottom. But reasonable. Looking at the visual result, I will guess that the vertical space was calculated based on 45° angle for the tick, but actual placement was less. That might be true for the left margin spacing as well. The start of the first tick text is VERY close to the left edge of the canvas. I do not know if it was intended to get that close.

#4268 looks like it is doing the calculations, again assuming a 45° degree placement, but as you say, with an inappropriate string length. I see references to maximum string length. For the horizontal axis, and the left margin, that should really be the length of the first tick string. That is the one that extends the furthest to the left.

[mMerlin]

That unknown long string is probably why my fiddles were rotating the tick labels in the first place. The actual tick label text for the new 2d7c1f0 format is actually shorter than for 3a2884f, and I had to increase the css div width from 500 to 602 before it goes back to 0°. When you forced it to zero, it also changed from ticks every 30 minutes, to ticks every hour.

Without that maxRotation: 0, and with width: 601px;, it looks like very close to double the space actually needed, in both the horizontal and vertical directions.

[etimberg]

I see why this is happening now. It's very very subtle. On two spots in the core scale we measure the longest tick:
https://github.com/chartjs/Chart.js/blob/master/src/core/core.scale.js#L261
https://github.com/chartjs/Chart.js/blob/master/src/core/core.scale.js#L342

With #4268, the ticks array contains objects instead of strings so now we measure each tick as being the length of '[object object]' which is obviously incorrect.

@benmccann @hurskiy-andriy any thoughts on this?

[hurskiy-andriy]

I see two ways tolve this problem:

• make array of strings to pass to helpers.longestText() in core.scale.js at line 261
  OR
• in core.helpers.js helpers.longestText method (line 560) we can add check if thing is object -> extract string from there

[benmccann]

Perhaps an orthogonal issue, but it looks to me like core.scale.js is using raw strings, which is probably a bit suspect since it'd be ignoring all the formatting options that the user might specify. It looks to me like it In scale.time.js something similar is done that's probably a bit closer to what we'd want:

var exampleLabel = me.tickFormatFunction(moment(exampleTime), 0, []).value;
var tickLabelWidth = me.getLabelWidth(exampleLabel);

tickFormatFunction has a decent amount of logic in it and I'm not sure if it's safe for core.scale.js to not use it?

[etimberg]

It may make sense to have all scales use an object inside of ticks however that would definitely be a v3.0 kind of breaking change. That way we could also support different styles for each tick.

[mMerlin]

If no one else is picking this up, I can have a closer look. Now that I know what to look for and at. It might take some time though, depending.