Allow configuration of borderWidth as object

[kurkle]

Allow object as borderWidth, false / null (etc) to borderSkipped.
Examples:

borderWidth: 2,
borderSkipped: false

borderWidth: { right: 2, bottom: 2},

Pen (817970c)
Pen (66d84ec)
Pen (40b4327)
Pen (700ae2c)

Fixes: #5565
Fixes: #5071
Fixes: #5709
Fixes: #4681
Related: #5413
Related: #3293 (fixed in master already)

[benmccann]

Also related is #5262 which implements the 'none' option as well by checking if borderSkipped === null

[benmccann]

Could we use an array for specifying multiple borders instead? So instead of 'left right' we could do ['left', 'right']. It looks like right now it would accept 'leftrighttop' as a valid value, but I think it might be nice to be slightly stricter

[simonbrunel]

I can't remember the ticket(s) but I'm sure we already had this debate about borderSkipped: this options should be deprecated in favor of supporting per side borderWidth, for example borderWidth.left: 0 would be equivalent to skip the left border.

I agree with @benmccann about not using a string but using an array doesn't make sense either because it's not possible to skip a border more than one time (i.e. ['left', 'left'] is not valid). And in both cases, a search is required.

If we really want to enhance this option (instead of borderWidth), what about supporting an object instead:

borderSkipped: false | null   // instead of 'none'
borderSkipped: {
    left: boolean,
    right: boolean,
    top: boolean,
    bottom: boolean
}

It's consistent with the other "per side" options (e.g. layout.padding), it works better with the merging process, it's easier to use and doesn't involve searching a string or array (better performances).

[kurkle]

Updated per comments

[benmccann]

Do we still need to make borderSkipped an object if we're also making borderWidth an object? It seems like it'd be simpler to only make the latter an object

[kurkle]

Do we still need to make borderSkipped an object if we're also making borderWidth an object? It seems like it'd be simpler to only make the latter an object

I think we should probably just deprecate borderSkipped, as everything can be done with only borderWidth. But didn't want to remove it just yet.

[kurkle]

A little more cleanup and tests for different directions.

[etimberg]

Just one minor comment

[kurkle]

One thing that might arise from this, is the fact that top/bottom for vertical and left/right for horizontal are actually start/end.
Changing that would be a breaking change - so probably another thing that should be corrected in the next major.

[benmccann]

I think it's confusing to be able to set borderSkipped as an object, but not be able to set multiple borders skipped. I agree we should deprecate, borderSkipped, but I'm still not sure why we need to expand it with new functionality in the meantime. We can document in the borderSkipped documentation that you should use borderWidth instead as it's more flexible and allows for skipping multiple borders

[kurkle]

refactored quite heavily to make it simpler (imo), better or not?

[simonbrunel]

After talking a bit more with @kurkle, it seems that the following makes sense (at least for v2):

• do not deprecate borderSkipped but keep supporting only one side - inverted when negative
• add support for borderSkipped: false to display all rectangle borders
• add support for borderWidth as object - not inverted (top is top, always)
• always apply borderSkipped (inverted) on top of borderWidth (not inverted)

Thoughts?

---

Here are some use cases:

1. Users who want to hide border at origin should keep using borderSkipped: 'bottom' (default)
2. Users who want to show only the "end" border (e.g. Border only top of barchart #5071) would use :

borderSkipped: 'bottom',          // e.q. origin
borderWidth: {top: 2, bottom: 2}  // left/right implicitly 0

3. Users who want to show all borders but with different style (not inverted for negative value:

borderSkipped: false,
borderWidth: {top: 4, bottom: 2}

4. Users who want to show a border differently based on the current value:

borderSkipped: false,
borderWidth: function(ctx) {
    var neg = ctx.dataset.data[ctx.dataIndex] < 0;
    return {top: neg ? 1 : 4, bottom: neg ? 1 : 4};
}

The last use case (4) may not be common, so I don't think it's an issue to ask for a scriptable option. Also, the condition (< 0) is really up to the user.

[benmccann]

That all sounds reasonable to me

[kurkle]

Updated to comply #6047 (comment)
@benmccann can you come up with something better to docs? 😄

[nagix]

Are these glitches due to antialiasing?

test/fixtures/controller.bar/borderWidth/object.png

test/fixtures/controller.bar/horizontal-borders.png

[nagix]

Maybe we can fake the border using a path if the border widths are uneven.

ctx.beginPath();
ctx.rect(/* outer rect */);
ctx.rect(/* inner rect */);
ctx.fill('evenodd');

[kurkle]

Good catch, @nagix! I did not notice those.
Filling between rectangles does not work for skipped border (in chrome). But I'll try outer rectangle +manual path and fill. So lets not merge this yet.

[kurkle]

Issue found by @nagix was not that simple to fix. With no skipped border, rectangle approach works like a charm and is a really compact solution. It does not work with skipped borders however.

I tried using a rectangle + path, but that same issue lets the fill leak in some cases.

So here is a new solution. While at it, added borderRadius as it was a natural fit.

This got way bigger than my intention originally was, but at least I have explored some options :)
(description updated with a new pen)

[nagix]

@kurkle What happens with the rectangle approach with skipped borders? Does zero-width border still remain visible?

[kurkle]

@kurkle What happens with the rectangle approach with skipped borders? Does zero-width border still remain visible?

Firefox is fine, Chrome draws a faint line there.

[nagix]

I see a problem when borderRadius is greater than half of the bar width. Also, the inner shape is a rectangle even if borderRadius is greater than borderWidth.

If we don't take the arc approach, another option would be to use a clip in the same way as the arc element (link). If you still see a faint line on the edge, you can shift that section of the border a little bit outward so that it is completely outside the clip area.

ctx.save();
ctx.beginPath();
ctx.rect(/* outer rect */);
ctx.clip();
ctx.beginPath();
ctx.rect(/* outer rect minus half of the thickest border width but shift outward based on the width of each section*/);
ctx.lineWidth = /* thickest border width */;
ctx.stroke();
ctx.restore();

[kurkle]

If we don't take the arc approach, another option would be to use a clip in the same way as the arc element (link). If you still see a faint line on the edge, you can shift that section of the border a little bit outward so that it is completely outside the clip area.

I'll do another PR using that approach later.

[kurkle]

Closing in favor of #6077

[simonbrunel]

@nagix about the clipping approach, according the MDN:

The CanvasRenderingContext2D.clip() method of the Canvas 2D API turns the current or given path into the current clipping region. It replaces any previous clipping region.

We are already clipping the canvas to the chart area, so it may not work in this case.

[nagix]

CanvasRenderingContext2D.save() and CanvasRenderingContext2D.restore() save/restore the current clipping region, so I think it should be no problem.

[kurkle]

The outer clip was introduced in #3658 to clip overshooting bars / lines. So this inner clipping will ignore that. I'll see if there is a easy workaround.

[nagix]

Ah, right, the problem is the solution in my comment will ignore the outer clip...