Closed Bug 411382 Opened 12 years ago Closed 12 years ago

SVG document with unspecified height gets clipped at 150px when printed

Categories

(Core :: SVG, defect, P2)

x86
All
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: fantasai.bugs)

References

(Depends on 2 open bugs, )

Details

(Keywords: regression)

Attachments

(3 files, 5 obsolete files)

See url, when printing that document, almost nothing shows up as print output, only a few lines.
It just works fine on branch.
Flags: blocking1.9?
I also noticed that print preview looks much smaller on trunk than it is on branch, I filed bug 411383 for that.
Flags: in-testsuite?
Version: unspecified → Trunk
Assignee: nobody → dholbert
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Attached file ignore -- posted to wrong bug (obsolete) —
Comment on attachment 298607 [details]
ignore -- posted to wrong bug

oops -- sorry, wrong bug
Attachment #298607 - Attachment is obsolete: true
Attachment #298607 - Attachment description: megatest → ignore -- posted to wrong bug
Attached file printing results
This is the output I get from both Print and Print-Preview on WinVista and on Linux in trunk.  Just the top half of the butterfly is printed, to the base of its head.
OS: Windows Vista → All
Attached image testcase 1 (pure SVG)
Here's a somewhat reduced butterfly testcase, with just one of the <path> elements.
Here's the previous SVG testcase embedded as an object in an HTML document.

It looks identical to the printing output -- only the top half of the butterfly is visible.

Some DOM inspection shows that the <object> element is 150px tall.  Maybe we're assuming SVG elements are 150px tall unless a height is specified?

Note that we can hide the bug by adding a specified height (e.g. 300px) to either the SVG source or the <object> element.  So this is a bug with unspecified-width-SVG objects.
Attachment #298784 - Attachment description: testcase (html, no printing required) → testcase 2 (html, no printing required)
Attachment #298750 - Attachment description: testcase → testcase 1 (pure SVG)
In Firefox 2 and in builds up to the regression point, the HTML testcase (attachment 298784 [details]) rendered with...
 - height clipped at 150px
 - width clipped at 300px.

After the regression window, the width clipping is removed, so now just the height clipping remains.

Also -- these issues all occur with this testcase as well:
  http://www.croczilla.com/svg/samples/circles1/circles1.svg
That is, it's clipped to be 150px high when printed, and it's also clipped when included as an <object> in a HTML document.  As I said in my previous comment, this seems to be due to the fact that the testcase's SVG doesn't give a height.
> this is a bug with unspecified-width-SVG objects.

Unspecified width defaults to 100%, so this is rather a problem with percentage width/height during printing. When percentage width can't be resolved it defaults to 300px, and when percentage height can't be resolved it defaults to 150px (as per CSS 2.1 spec.). For some reason it looks like percentages for width/height on root elements can't be resolved (no viewport/containing block).
(In reply to comment #9)
> > this is a bug with unspecified-width-SVG objects.

Sorry -- s/width/height
Attached file testcase 3 (html, with table wrapper) (obsolete) —
This new testcase is the same as the previous one, but with a table wrapping around the SVG <object>.  The table just has a green border, and has no specified width/height.

Here's how we handle this testcase:
  Trunk Pre-411383 (and FF2):  SVG <object> clipped to 150px high, 300px wide
  Trunk Post-411383:           SVG <object> clipped to 150px high, 0px wide
                                 (i.e. only the green border is visible)

So it looks like we're now returning 0px as the default width, not 300px.
(In reply to comment #11)
> So it looks like we're now returning 0px as the default width, not 300px.

(this width issue may be worthy of a separate bug -- just posting it here for its relevance to this bug and in case it's easily fixed as part of this bug)
Component: Printing → SVG
Yeah, we don't set the initial containing block correctly for print, see bug 381497.
Depends on: 381497
Comment on attachment 298804 [details]
testcase 3 (html, with table wrapper)

(In reply to comment #11)
> Created an attachment (id=298804) [details]
> testcase 3 (html, with table wrapper)
> 
> This new testcase is the same as the previous one, but with a table wrapping
> around the SVG <object>.  The table just has a green border, and has no
> specified width/height.

Yup, that's bug 382325. See the third attachment (attachment 289669 [details]) on that bug.
Attachment #298804 - Attachment is obsolete: true
Comment on attachment 298784 [details]
testcase 2 (html, no printing required)

> Created an attachment (id=298784) [details]
> testcase (html, no printing required)

This is exactly as CSS 2.1 requires. What happens is that the SVG has an intrinsic width and height of 100%. Since the containing block for the <object> does not have a fixed height, the intrinsic height of  100% cannot be resolved, and it falls back to 150px. (Essentially the height of the containing block and the height of the <object> depend on each other, so something has to give.)
Attachment #298784 - Attachment is obsolete: true
Summary: Printing SVG document almost doesn't print anything → SVG document with unspecified height gets clipped at 150px
Summary: SVG document with unspecified height gets clipped at 150px → SVG document with unspecified height gets clipped at 150px when printed
Ok -- from talking to jwatt and fantasai in IRC a bit, I think the correct way to fix this bug is to squash bug 381497.  (see comment 13 -- essentially, when printing, the 100% height on SVG content isn't getting resolved correctly because we're using the wrong containing block.)

Nominating that bug for blocking.
Reassigning to jwatt as he knows our SVG code & SVG expected behavior much better than I do. :)

Note that AFAIK, this bug mostly depends on bug 381497 being fixed, but if we can find a workaround for this in the meantime, that'd definitely be helpful.
Assignee: dholbert → jwatt
Attached patch patch (obsolete) — Splinter Review
This fixes the first two testcases in bug 381479, and also this one. Fixing 381479 properly would probably be pretty difficult and risky and not something we want to tackle for 1.9.
Assignee: jwatt → fantasai.bugs
Status: NEW → ASSIGNED
Attachment #301386 - Flags: superreview?(bzbarsky)
Attachment #301386 - Flags: review?(bzbarsky)
Wouldn't it make more sense to fix ComputeSize() for page frames to just use the right computed height?  Or would that misbehave somehow?
Attached patch patch v2 (obsolete) — Splinter Review
(And by 381479 I meant bug 381497.)

Updated patch using ComputeSize(). Thanks for the tip, bz. :)
Attachment #301386 - Attachment is obsolete: true
Attachment #302086 - Flags: superreview?(bzbarsky)
Attachment #302086 - Flags: review?(bzbarsky)
Attachment #301386 - Flags: superreview?(bzbarsky)
Attachment #301386 - Flags: review?(bzbarsky)
Comment on attachment 302086 [details] [diff] [review]
patch v2

Looks fine, though it might make sense to store this number in mPD somewhere, since the page nsPageFrame ends up doing the same exact calculation....
Attachment #302086 - Flags: superreview?(bzbarsky)
Attachment #302086 - Flags: superreview+
Attachment #302086 - Flags: review?(bzbarsky)
Attachment #302086 - Flags: review+
Comment on attachment 302086 [details] [diff] [review]
patch v2

>Index: layout/generic/nsPageContentFrame.cpp
>+  nscoord height = (mPD && mPD->mReflowSize.height == NS_UNCONSTRAINEDSIZE)
>+                   ? (NS_UNCONSTRAINEDSIZE)
>+                   : (mPD->mReflowSize.height - 

Er, I just realized there's a weirdness here.  Why null-check mPD?  If you feel that you must, wouldn't it make more sense to do:

  (!mPD || mPD->mReflowSize.height == NS_UNCONSTRAINEDSIZE)

for the conditional?

And remove the parens around NS_UNCONSTRAINEDSIZE after the '?', please.
Attached patch patch v3Splinter Review
Fixed null-check and added an assert, because I don't think we're supposed to be getting a null mPD here (but I'm not sure).
Attachment #302086 - Attachment is obsolete: true
Attachment #302096 - Flags: superreview?(bzbarsky)
Attachment #302096 - Flags: review?(bzbarsky)
Comment on attachment 302096 [details] [diff] [review]
patch v3

Looks good.
Attachment #302096 - Flags: superreview?(bzbarsky)
Attachment #302096 - Flags: superreview+
Attachment #302096 - Flags: review?(bzbarsky)
Attachment #302096 - Flags: review+
Excellent. Thanks a lot for taking this fantasai!
Fix checked in. Thanks, bz.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 1033149
You need to log in before you can comment on or make changes to this bug.