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

Axis labels removed by the auto skip functionality are still considered for sizing purposes #3694

Closed
ste2425 opened this issue Dec 8, 2016 · 10 comments · Fixed by #8627
Closed

Comments

@ste2425
Copy link

ste2425 commented Dec 8, 2016

Expected Behavior

When padding the bottom of a chart to make space for xAxis labels it should only pad for rendered labels, not all labels.

Current Behavior

If a dataset has a long xAxis label, but the chart is small and that label not rendered the chart has extra padding on the bottom to accommodate that label. Results in potentially a large amount of white space under the chart.

Possible Solution

Not entirely sure how the amount of padding needed is calculated but it appears it is performed before the list of labels to render is filtered down. Potentially move this process to after?

Steps to Reproduce (for bugs)

Fiddle

The top chart does not occupy the full height of its container. The bottom chart shows it's the label 'Autoenrolment' that it is accounting for, even though it is not rendered in the top chart.

Context

Chartjs is being used within Angular-Gridster, so the charts resize to their container, those containers are not always square so we turn off maintainAspectRatio.

The issue is the grid has to fit within a relatively small space (No matter how much we try this requirement cannot change) Which means you could potentially make rather small charts. (As big as the small example in the fiddle). Chartjs does a great job of managing this, except for the padding used to accommodate the Axislabels.

Environment

  • Chart.js version: 2.3.0 & 2.0.0 (Used in fiddle)
  • Browser name and version: Chrome latest, IE 11, FF latest

EDIT: Also present in the latest master

Workaround

As a workaround i have had to truncate my labels so that they are at a more uniformed length, using the ticks callback. Then use the tooltip callbacks to present the user with the full length tooltip.

@etimberg
Copy link
Member

etimberg commented Dec 8, 2016

@ste2425 is it possible to try the latest code from master? For v2.5 we've made some fixes to the padding calculations that might help alleviate this.

@ste2425
Copy link
Author

ste2425 commented Dec 9, 2016

@etimberg Sadly it still exhibits the same behavior fidle

@chron0
Copy link

chron0 commented Dec 28, 2016

I'm fighting with similar issues (up to 2.4.0) on the xAxis, where the labels are cut in half. This changes only when the labels start to rotate. As soon as they leave the default position, the padding is applied correctly. See screenshot in #3749.

I've used up any CSS/wrapping means in my arsenal but as it seems to be originating within the canvas, external fixing options are pretty limited. Any other ideas or just wait until 2.5.0 and try again?

@etimberg
Copy link
Member

The problem that is happening here is that the x axis is measured for size asssuming all the labels will display. The auto skipper runs during drawing (ie. once all measurement is finished) and hides the long labels.

Some options:

  1. disable the auto skipper
  2. change the auto-skipper to work before all measurement is finished and return the size of the axis only taking visible ticks into account.

Option 2 seems to be the best long-term, but it is obviously much more complex

@desean1625
Copy link

Fix for this. core.scale.js change the fit function

function() {
  var me = this;
  // Reset
  var minSize = me.minSize = {
    width: 0,
    height: 0
  };

  var labels = labelsFromTicks(this._autoSkip(me._ticks)).filter(l => undefined !== l);
  var length = labels.length;
  var opts = me.options;
  var tickOpts = opts.ticks;
  var scaleLabelOpts = opts.scaleLabel;
  var gridLineOpts = opts.gridLines;
  var display = opts.display;
  var isHorizontal = me.isHorizontal();

  var tickFont = parseFontOptions(tickOpts);
  var tickMarkLength = opts.gridLines.tickMarkLength;

  // Width
  if (isHorizontal) {
    // subtract the margins to line up with the chartArea if we are a full width scale
    minSize.width = me.isFullWidth() ? me.maxWidth - me.margins.left - me.margins.right : me.maxWidth;
  } else {
    minSize.width = display && gridLineOpts.drawTicks ? tickMarkLength : 0;
  }

  // height
  if (isHorizontal) {
    minSize.height = display && gridLineOpts.drawTicks ? tickMarkLength : 0;
  } else {
    minSize.height = me.maxHeight; // fill all the height
  }

  // Are we showing a title for the scale?
  if (scaleLabelOpts.display && display) {
    var scaleLabelLineHeight = parseLineHeight(scaleLabelOpts);
    var scaleLabelPadding = helpers.options.toPadding(scaleLabelOpts.padding);
    var deltaHeight = scaleLabelLineHeight + scaleLabelPadding.height;

    if (isHorizontal) {
      minSize.height += deltaHeight;
    } else {
      minSize.width += deltaHeight;
    }
  }

  // Don't bother fitting the ticks if we are not showing them
  if (tickOpts.display && display) {
    var largestTextWidth = helpers.longestText(me.ctx, tickFont.font, labels, me.longestTextCache);
    var tallestLabelHeightInLines = helpers.numberOfLabelLines(labels);
    var lineSpace = tickFont.size * 0.5;
    var tickPadding = me.options.ticks.padding;

    if (isHorizontal) {
      // A horizontal axis is more constrained by the height.
      me.longestLabelWidth = largestTextWidth;

      var angleRadians = helpers.toRadians(me.labelRotation);
      var cosRotation = Math.cos(angleRadians);
      var sinRotation = Math.sin(angleRadians);

      // TODO - improve this calculation
      var labelHeight = (sinRotation * largestTextWidth) +
        (tickFont.size * tallestLabelHeightInLines) +
        (lineSpace * (tallestLabelHeightInLines - 1)) +
        lineSpace; // padding

      minSize.height = Math.min(me.maxHeight, minSize.height + labelHeight + tickPadding);

      me.ctx.font = tickFont.font;
      var firstLabelWidth = computeTextSize(me.ctx, labels[0], tickFont.font);
      var lastLabelWidth = computeTextSize(me.ctx, labels[labels.length - 1], tickFont.font);

      // Ensure that our ticks are always inside the canvas. When rotated, ticks are right aligned
      // which means that the right padding is dominated by the font height
      if (me.labelRotation !== 0) {
        me.paddingLeft = opts.position === 'bottom' ? (cosRotation * firstLabelWidth) + 3 : (cosRotation * lineSpace) + 3; // add 3 px to move away from canvas edges
        me.paddingRight = opts.position === 'bottom' ? (cosRotation * lineSpace) + 3 : (cosRotation * lastLabelWidth) + 3;
      } else {
        me.paddingLeft = firstLabelWidth / 2 + 3; // add 3 px to move away from canvas edges
        me.paddingRight = lastLabelWidth / 2 + 3;
      }
    } else {
      // A vertical axis is more constrained by the width. Labels are the
      // dominant factor here, so get that length first and account for padding
      if (tickOpts.mirror) {
        largestTextWidth = 0;
      } else {
        // use lineSpace for consistency with horizontal axis
        // tickPadding is not implemented for horizontal
        largestTextWidth += tickPadding + lineSpace;
      }

      minSize.width = Math.min(me.maxWidth, minSize.width + largestTextWidth);

      me.paddingTop = tickFont.size / 2;
      me.paddingBottom = tickFont.size / 2;
    }
  }

  me.handleMargins();
  me.width = minSize.width;
  me.height = minSize.height;
  if (labelsFromTicks(this._autoSkip(me._ticks)).filter(l => undefined !== l).length !== length) {
    this.fit();
  }
}

@desean1625
Copy link

Run the autoskip on the labels before we calculate the size of the container and only measure the text of the labels to be displayed.

@desean1625
Copy link

Updated fiddle with the patch showing the correct padding on the top chart https://jsfiddle.net/0b04gr9z/

@etimberg
Copy link
Member

etimberg commented Jan 1, 2020

@kurkle @benmccann looks like this still occurs with v3: https://jsfiddle.net/ro9b8n0c/

@benmccann
Copy link
Contributor

It's not quite as simple as moving autoSkip before fit as @desean1625 suggested because fit updates _length (i.e. the width or height depending on isHorizontal), which autoSkip depends on.

@benmccann
Copy link
Contributor

We could split fit into two functions. We could determine the width first, then do autoSkip, then determine the height based only on showing labels. Any thoughts about that as a solution @kurkle? We should think about a solution in tandem with #6791 in case there's some way to rework this whole section of code that's cleaner

@etimberg etimberg changed the title [BUG] Incorrect chart offset for xAxis labels Axis labels removed by the auto skip functionality are still considered for sizing purposes Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants