Opened 7 years ago

Last modified 20 months ago

#16341 assigned defect

[patch][cla] charts don't render the right dimensions

Reported by: ben hockey Owned by: Eric Durocher
Priority: undecided Milestone: 1.14
Component: DojoX GFX Version: 1.8.1
Keywords: Cc: Patrick Ruzand
Blocked By: Blocking:

Description (last modified by ben hockey)

http://jsfiddle.net/neonstalwart/GRGjD/ shows a chart that i would expect to be rendered without any overflow at all - the dimensions have been set to fit exactly in the space provided yet the chart causes overflow.

when i looked at the code that renders the chart i found the background of a chart is composed of 2 rect elements - 1 for "fill" and 1 for "stroke". the "fill" rect is rendered 1px taller and 1px wider than the chart dimensions (http://bugs.dojotoolkit.org/browser/dojo/dojox/trunk/charting/Chart.js?rev=29694#L984) and the "stroke" rect is rendered 1px narrower and 1px shorter than the chart dimensions (http://bugs.dojotoolkit.org/browser/dojo/dojox/trunk/charting/Chart.js?rev=29694#L1027).

why the need to use anything other than the proper dimensions? there are no comments at all in the code to give any indication of why this is done.

i suspect that there could be more to it than just these few lines of code because in an attempt to tweak the dimensions so the chart will render how i'd expect i've found the height to be too tall by 3px.

Attachments (1)

patch16341.patch (498 bytes) - added by cjolif 6 years ago.
tenative gfx fix

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by ben hockey

Description: modified (diff)

comment:2 Changed 7 years ago by cjolif

maybe there is a relationship with #16068 (not sure).

comment:3 Changed 7 years ago by ben hockey

this would certainly contribute towards #16068. however, #16068 looks like it is primarily an issue of margin-box vs content-box (which i agree would be more intuitive to be content-box) and if we were to fix this issue i'd say that #16068 will still be an issue due to the box model being used to calculate the dimensions of the chart.

in the example i've provided, given that there are no borders, padding or margin, the content-box and margin-box are equal so there seems to be a problem that goes beyond just the box model. my main confusion is why the dimensions are being adjusted when creating the rect elements that represent the fill and stroke of the chart background. if we figure out why these numbers are being adjusted we should at least add some comments but to me it looks like they're just wrong and the actual dimensions should be used.

comment:4 Changed 6 years ago by cjolif

Owner: changed from Eugene Lazutkin to cjolif
Status: newassigned

If you look at the generated HTML+SVG it is something like the following:

<div id="chart">
  <svg overflow="hidden" width="500" height="270">
     <rect stroke="none" stroke-opacity="0" stroke-width="1" stroke-linecap="butt" stroke-linejoin="miter" stroke-miterlimit="4" x="0" y="0" width="501" height="271" ry="0" rx="0" fill-rule="evenodd"></rect>
     <rect fill="none" fill-opacity="0" stroke="rgb(181, 188, 199)" stroke-opacity="1" stroke-width="1" stroke-linecap="butt" stroke-linejoin="miter" stroke-miterlimit="4" x="0" y="0" width="499" height="269" ry="0" rx="0" stroke-dasharray="none" dojogfxstrokestyle="solid"></rect>
   </svg>
</div>

If you ignore the 1px pb you already mentioned the dimensions are ok! But still if you look at the computed size in Chrome debugger for the first rect you will see it is something 70px higher than the one set on the rect. There seem to be a mixup between the CSS height and the SVG height.

comment:5 Changed 6 years ago by cjolif

Actually I don't reproduce anymore the 70px difference I talked about above. That said I still reproduce the issue in pure SVG. I think the best explanation can be found here: http://stackoverflow.com/questions/4758638/why-is-there-a-vertical-scrollbar-on-my-svg-at-100. Apparently svg elements are by default "inline" and do reserve some additional spacing for descent that causes the issue. The attached dojox/gfx patch is fixing the issue by explicitly setting "block". I re-assign that ticket to Patrick so he can investigate more (my naive patch might not be good) as I think the issue (beside #16068) is really a gfx issue not a charting one.

Changed 6 years ago by cjolif

Attachment: patch16341.patch added

tenative gfx fix

comment:6 Changed 6 years ago by cjolif

Component: ChartingDojoX GFX
Owner: changed from cjolif to Patrick Ruzand

comment:7 Changed 6 years ago by cjolif

By the way the following code reproduces the issue in pure gfx:

<!DOCTYPE HTML>
<html lang="en">
	<head>
		<script src="../../../dojo/dojo.js" data-dojo-config="isDebug: true,parseOnLoad: true, async:1"></script>
		<script>
			require(["dojo/ready", "dojox/gfx", "dojo/dom"], function(ready, gfx, dom){
				ready(function(){
					gfx.createSurface(dom.byId("gfx"), 500, 270);
				});
			});
		</script>
		<style>
			#container {
			    height: 300px;
			    width: 500px;
			    overflow: auto;
			}

			#banner {
			    height: 30px;
			    line-height: 30px;
			    background-color: #ededed;
			}

			#gfx {
			    height: 270px;
			    background-color: red;
			}
		</style>
	</head>
	<body>
		<div id="container">
	    	<div id="banner">This is a banner</div>
		    <div id="gfx"></div>
		</div>
	</body>
</html>

comment:8 Changed 6 years ago by Eric Durocher

Owner: changed from Patrick Ruzand to Eric Durocher

comment:9 Changed 3 years ago by dylan

Milestone: tbd1.11
Summary: charts don't render the right dimensions[patch][cla] charts don't render the right dimensions

Is there a reason we never landed the one line patch?

comment:10 Changed 3 years ago by cjolif

Cc: Patrick Ruzand added

adding pruzand because he probably knows better why it did not get in.

comment:11 Changed 3 years ago by Patrick Ruzand

The reason why it was not merged is no the time to do proper testing (esp. on mobile devices, at that time SVG support was kind of flimsy).

comment:12 Changed 3 years ago by dylan

Milestone: 1.111.12

Seems like a very simple fix to try and test, but I'll move it to 1.12 then. Hopefully someone will have interest in landing this!

comment:13 Changed 3 years ago by dylan

Milestone: 1.121.13

Ticket planning... move current 1.12 tickets out to 1.13 that likely won't get fixed in 1.12.

comment:14 Changed 20 months ago by dylan

Milestone: 1.131.14
Note: See TracTickets for help on using tickets.