Update to fix responsive chart size in IE11 when parent container has… #4620

andi-b · 2017-08-07T13:43:49Z

… padding as percentage

When the chart is responsive to the parent container, the calculations for padding assumes that the figure is in pixels so that 20% is taken to be 20 (pixels), which results in the chart exceeding the parent container.

Issue may be seen in IE11 here:
https://jsfiddle.net/digits/e8g4ux20/

… padding as percentage When the chart is responsive to the parent container, the calculations for padding assumes that the figure is in pixels so that 20% is taken to be 20 (pixels), which results in the chart exceeding the parent container. This appears to be an IE11 only issue.

andi-b · 2017-08-08T08:46:47Z

Codeclimate seems to lie :) Click on 'Details' and all appears fine

benmccann · 2017-09-19T21:27:09Z

Does this issue happen only on IE11?

andi-b · 2017-09-19T22:05:14Z

Strangely it does seem to only happen in IE11. Was fine in Firefox as uses computed value and think it was fine in Chrome and Edge.

benmccann · 2017-10-11T04:23:40Z

Perhaps you could add a helper function to cut down on code duplication:

function calculatePadding(padding, parentDimension) {
	if (padding.indexOf('%') > -1) {
		return parentDimension / parseInt(padding, 10);
	} else {
		return parseInt(padding, 10);
	}
}

etimberg

I think this needs some tests written as well

benmccann · 2018-01-16T06:27:54Z

@andi-b will you be able to respond to the comments on this PR?

andi-b · 2018-01-16T11:11:58Z

I have made the update locally - just need to test myself before submitting change.

Do I need to write tests and how do I go about doing this?

Thanks

Added calculatePadding function to reduce code duplication

Reduced code further by adding to calculatePadding function

Fixed due to code climate: Updated to inline if statement for single return Removed space;

benmccann · 2018-01-16T17:10:43Z

Yes, please add tests as well. You can add them to the file test/specs/core.helpers.tests.js. You can create a dom node and call the function on it and check if the results are as expected

benmccann · 2018-02-03T18:16:08Z

@andi-b do you plan on updating this PR?

andi-b · 2018-02-06T08:45:31Z

@benmccann Sorry, just need to find some time to figure out how to add a test case as this is new to me. I'll hopefully find some time this week.

Testing script added for calculatePadding function

Update code formatting as per Travis CI

simonbrunel · 2018-02-08T08:55:17Z

src/core/core.helpers.js

@@ -466,14 +466,20 @@ module.exports = function(Chart) {
 	helpers.getConstraintHeight = function(domNode) {
 		return getConstraintDimension(domNode, 'max-height', 'clientHeight');
 	};
+	helpers.calculatePadding = function(container, padding, parentDimension) {


We don't want this method to be part of the public API.

Can you prefix the method with _ and add a private comment:

/** * @private */ helpers._calculatePadding = function(...

simonbrunel · 2018-02-08T08:59:11Z

src/core/core.helpers.js

 	helpers.getMaximumWidth = function(domNode) {
 		var container = domNode.parentNode;
 		if (!container) {
 			return domNode.clientWidth;
 		}

-		var paddingLeft = parseInt(helpers.getStyle(container, 'padding-left'), 10);
-		var paddingRight = parseInt(helpers.getStyle(container, 'padding-right'), 10);
+		var paddingLeft = helpers.calculatePadding(container, 'padding-left', container.clientWidth);


We should cache clientWidth since it's used multiple times:

var clientWidth = container.clientWidth; var paddingLeft = helpers.calculatePadding(container, 'padding-left', clientWidth ); var paddingRight = helpers.calculatePadding(container, 'padding-right', clientWidth ); var w = clientWidth - paddingLeft - paddingRight; // ...

simonbrunel · 2018-02-08T08:59:35Z

src/core/core.helpers.js

@@ -484,8 +490,9 @@ module.exports = function(Chart) {
 			return domNode.clientHeight;
 		}

-		var paddingTop = parseInt(helpers.getStyle(container, 'padding-top'), 10);
-		var paddingBottom = parseInt(helpers.getStyle(container, 'padding-bottom'), 10);
+		var paddingTop = helpers.calculatePadding(container, 'padding-top', container.clientHeight);


Same here (var clientHeight = container.clientHeight;)

simonbrunel · 2018-02-08T09:03:01Z

test/specs/core.helpers.tests.js

@@ -742,6 +742,41 @@ describe('Core helper tests', function() {
 		expect(canvas.style.width).toBe('400px');
 	});

+	it ('Should get padding of parent as number (pixels) when defined as percent (returns incorrectly in IE11)', function() {


Ideally, we would need a unit test that doesn't check internals but creates a chart inside a container with percent padding and verify the chart dimensions (something similar to this test)

I've removed call to now private function calculatePadding.
I attempted to write test with chart but require a parent DIV to the wrapper so I can set the width as the padding percentage is taken from this and not from the wrapper.

it('**********Should calculate padding of parent container in pixels if as percent (IE11)', function(done) { var chart = acquireChart({ type: 'pie', options: { responsive: true, maintainAspectRatio: true } }, { canvas: { style: 'width: 300px; height: 300px; position: relative' }, wrapper: { style: 'padding: 10%; width: 300px; height: 300px;' } }); expect(chart).toBeChartOfSize({ dw: 240, dh: 240, rw: 240, rh: 240, }); });

Added variable for clientWidth and clientHeight; Made function calculatePadding private

Removed calls to private function calculatePadding

Removed unused variable container

andi-b · 2018-02-14T17:17:49Z

codeclimate complaining about an existing function - should this be ignored?

etimberg · 2018-02-15T02:22:55Z

The code climate stuff about an existing function can be ignored.

chtheis · 2018-11-18T19:54:51Z

I think the calculation of padding in % is wrong, it just divides the size by the padding in percent.
Instead line 502 in src/core/core.helpers.js should read:

return padding.indexOf('%') > -1 ? parentDimension * parseInt(padding, 10) / 100 : parseInt(padding, 10);

I have a padding of 2% of the surrounding div, IE (Edge) returns just that value and the old code would return half the width as padding, thus the canvas would have 0 width.

chtheis · 2018-11-18T19:59:45Z

And in the test you use 10% as padding. In this case, and only this case, the old code would return a correct result. Maybe use something like 5% as the test case.

simonbrunel · 2018-11-19T07:02:10Z

@chtheis you are right, I'm surprised that nobody noticed that error in this PR.

When the chart is responsive to the parent container, the calculations for padding assumes that the figure is in pixels so that 20% is taken to be 20 (pixels), which results in the chart exceeding the parent container. This appears to be an IE11 only issue.

andi-b added 4 commits August 7, 2017 13:44

Updated code formating

3d4fe3a

Updated code formatting

5528a61

Updated spaces to tabs

26a56d7

etimberg requested a review from simonbrunel September 29, 2017 11:07

etimberg requested changes Dec 31, 2017

View reviewed changes

andi-b added 3 commits January 16, 2018 12:02

Added calculatePadding function

f31af0e

Added calculatePadding function to reduce code duplication

Reduced code further

52cdcce

Reduced code further by adding to calculatePadding function

Updated to inline if statement

083175b

Fixed due to code climate: Updated to inline if statement for single return Removed space;

benmccann added the status: needs test case label Jan 27, 2018

benmccann added the status: stale label Feb 3, 2018

andi-b added 2 commits February 7, 2018 17:26

Testing script added for calculatePadding function

4bfe92d

Testing script added for calculatePadding function

Merge branch 'master' into patch-1

23b863c

benmccann removed status: needs test case status: stale labels Feb 7, 2018

Update code formatting

cd00af3

Update code formatting as per Travis CI

etimberg previously approved these changes Feb 7, 2018

View reviewed changes

simonbrunel requested changes Feb 8, 2018

View reviewed changes

Added variables and made function private

fedda5b

Added variable for clientWidth and clientHeight; Made function calculatePadding private

andi-b dismissed etimberg’s stale review via fedda5b February 9, 2018 10:19

Removed calls to private function

ec79652

Removed calls to private function calculatePadding

Removed unused variable

76d0c0c

Removed unused variable container

etimberg approved these changes Feb 15, 2018

View reviewed changes

simonbrunel approved these changes Apr 22, 2018

View reviewed changes

simonbrunel merged commit 73b8cee into chartjs:master Apr 22, 2018

andi-b deleted the patch-1 branch April 30, 2018 13:47

chtheis mentioned this pull request Nov 18, 2018

Correct calculation of padding in percent #5846

Merged

vpetersson mentioned this pull request Mar 7, 2020

[Snyk] Upgrade chart.js from 2.7.3 to 2.9.3 WoTTsecurity/api#764

Merged

snyk-bot mentioned this pull request Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to fix responsive chart size in IE11 when parent container has… #4620

Update to fix responsive chart size in IE11 when parent container has… #4620

andi-b commented Aug 7, 2017

andi-b commented Aug 8, 2017

benmccann commented Sep 19, 2017 •

edited

Loading

andi-b commented Sep 19, 2017

benmccann commented Oct 11, 2017

etimberg left a comment

benmccann commented Jan 16, 2018

andi-b commented Jan 16, 2018

benmccann commented Jan 16, 2018

benmccann commented Feb 3, 2018

andi-b commented Feb 6, 2018

simonbrunel Feb 8, 2018

simonbrunel Feb 8, 2018

simonbrunel Feb 8, 2018

simonbrunel Feb 8, 2018

andi-b Feb 9, 2018

andi-b Feb 9, 2018

andi-b commented Feb 14, 2018

etimberg commented Feb 15, 2018

chtheis commented Nov 18, 2018

chtheis commented Nov 18, 2018

simonbrunel commented Nov 19, 2018

Update to fix responsive chart size in IE11 when parent container has… #4620

Update to fix responsive chart size in IE11 when parent container has… #4620

Conversation

andi-b commented Aug 7, 2017

andi-b commented Aug 8, 2017

benmccann commented Sep 19, 2017 • edited Loading

andi-b commented Sep 19, 2017

benmccann commented Oct 11, 2017

etimberg left a comment

Choose a reason for hiding this comment

benmccann commented Jan 16, 2018

andi-b commented Jan 16, 2018

benmccann commented Jan 16, 2018

benmccann commented Feb 3, 2018

andi-b commented Feb 6, 2018

simonbrunel Feb 8, 2018

Choose a reason for hiding this comment

simonbrunel Feb 8, 2018

Choose a reason for hiding this comment

simonbrunel Feb 8, 2018

Choose a reason for hiding this comment

simonbrunel Feb 8, 2018

Choose a reason for hiding this comment

andi-b Feb 9, 2018

Choose a reason for hiding this comment

andi-b Feb 9, 2018

Choose a reason for hiding this comment

andi-b commented Feb 14, 2018

etimberg commented Feb 15, 2018

chtheis commented Nov 18, 2018

chtheis commented Nov 18, 2018

simonbrunel commented Nov 19, 2018

benmccann commented Sep 19, 2017 •

edited

Loading