Fix Zero pointBorderWidth still drawing border #5180
Conversation
jhaenchen
changed the title
|
The reason we have to do this is because of this line which tries to set the canvas context line width to 0, which is not a valid value and so is ignored. |
src/elements/element.point.js
Outdated
| @@ -79,7 +79,11 @@ module.exports = Element.extend({ | |||
| return; | |||
| } | |||
|
|
|||
| ctx.strokeStyle = vm.borderColor || defaultColor; | |||
|
|
|||
| ctx.strokeStyle = helpers.valueOrDefault(vm.borderWidth, defaults.global.elements.point.borderWidth) === 0 ? | |||
There was a problem hiding this comment.
I would only compute the line width once which improves performance and minification
|
Sorry about that @etimberg just want to put in a comment because I was confused when assigning 0 and seeing 1 remain. |
|
This should be good to merge. |
|
@jhaenchen would you be able to rebase this PR? thanks! |
|
@jhaenchen just a quick reminder that this PR will need to be rebased |
src/elements/element.point.js
Outdated
| ctx.strokeStyle = vm.borderColor || defaultColor; | ||
| ctx.lineWidth = helpers.valueOrDefault(vm.borderWidth, defaults.global.elements.point.borderWidth); | ||
| var finalLineWidth = helpers.valueOrDefault(vm.borderWidth, defaults.global.elements.point.borderWidth); | ||
| ctx.strokeStyle = finalLineWidth === 0 ? 'rgba(0,0,0,0)' : vm.borderColor || defaultColor; |
There was a problem hiding this comment.
@jhaenchen these lines don't line up. you have two spaces here where there should be a tab
src/elements/element.point.js
Outdated
| @@ -79,8 +79,9 @@ module.exports = Element.extend({ | |||
|
|
|||
| // Clipping for Points. | |||
| if (chartArea === undefined || (model.x >= chartArea.left && chartArea.right * errMargin >= model.x && model.y >= chartArea.top && chartArea.bottom * errMargin >= model.y)) { | |||
| ctx.strokeStyle = vm.borderColor || defaultColor; | |||
| ctx.lineWidth = helpers.valueOrDefault(vm.borderWidth, defaults.global.elements.point.borderWidth); | |||
| var finalLineWidth = helpers.valueOrDefault(vm.borderWidth, defaults.global.elements.point.borderWidth); | |||
There was a problem hiding this comment.
why do you need an intermediate finalLineWidth variable here? can you do this instead:
ctx.lineWidth = helpers.valueOrDefault(vm.borderWidth, defaults.global.elements.point.borderWidth);
ctx.strokeStyle = ctx.lineWidth === 0 ? 'rgba(0,0,0,0)' : vm.borderColor || defaultColor;
|
The PR description says "it changes the stroke of the point to be completely opaque". I'm guessing you meant "transparent" instead? Why draw it transparently instead of just not drawing it at all? |
|
@jhaenchen a reminder to take another look at this PR when you get a chance |
|
@benmccann I'll take a look tonight, sorry. |
Yep, sorry.
You cannot set a stroke width of zero hence the bug in the first place (stroke width zero is ignored), and so no matter what you have a stroke when drawing a point. Therefore, in order to draw the point, we also have to draw the stroke, so the next best thing is a completely transparent stroke. Also, I've fixed your other comments. Thank you for your patience! |
|
It seems like we would need this fix in |
|
@benmccann would you like me to add that to this PR? |
|
Yes, that would be great if you can do that |
There was a problem hiding this comment.
@jhaenchen I think the latest code doesn't work (see my comment), can you please verify? Ideally, we will also need unit tests.
| @@ -79,7 +79,7 @@ module.exports = Element.extend({ | |||
|
|
|||
| // Clipping for Points. | |||
| if (chartArea === undefined || (model.x >= chartArea.left && chartArea.right * errMargin >= model.x && model.y >= chartArea.top && chartArea.bottom * errMargin >= model.y)) { | |||
| ctx.strokeStyle = vm.borderColor || defaultColor; | |||
| ctx.strokeStyle = ctx.lineWidth === 0 ? 'rgba(0,0,0,0)' : vm.borderColor || defaultColor; | |||
There was a problem hiding this comment.
You are trying to access ctx.lineWidth before assigning it a value (next line), is that expected?
Though, I think @benmccann suggestion was wrong because according the MDN:
lineWidth: A number specifying the line width in space units. Zero, negative, Infinity and NaN values are ignored.
So ctx.lineWidth will never match ctx.lineWidth === 0 (because ignored) so I would keep your first approach and cache the lineWidth value (and check for lineWidth === null or undefined using !lineWidth):
// ...
var errMargin = 1.01;
var lineWidth;
//...
lineWidth = helpers.valueOrDefault(vm.borderWidth, defaults.global.elements.point.borderWidth);
ctx.strokeStyle = !lineWidth ? 'rgba(0,0,0,0)' : vm.borderColor || defaultColor;
ctx.lineWidth = lineWidth;
|
@jhaenchen would you be able to address Simon's comments? |
|
@jhaenchen just a reminder that there's a comment that will need to be addressed before we can merge |
|
@jhaenchen this PR will need to be rebased. Don't forget about Simon's comment either. We'd love to get this merged |
|
@jhaenchen we'd love to merge this, but haven't gotten any response, so are going to close this PR as inactive. Please let us know if you're able to find the time to revisit and we can reopen this and work with you to get it in |
This PR fixes the fact that a pointBorderWidth property of zero still draws a border around the point. It does this by checking if the pointBorderWidth is zero. If yes, it changes the stroke of the point to be completely transparent.
Before with a zero width red point border:
https://codepen.io/anon/pen/xpNbbN
After with a zero width red point border:
