528046 - Print / print-preview crops very tall images at page boundaries

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Nick_Jenkins]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091109 Ubuntu/9.04 (jaunty) Shiretoko/3.5.5
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091109 Ubuntu/9.04 (jaunty) Shiretoko/3.5.5

"Shrink to fit" print preview of an image crops the image instead of printing all of it.

Reproducible: Always

Steps to Reproduce:
* Open the image URL (will also try to attach the image). Note: happens will many images, this example image is just being used to illustrate.
* Go File -> Print Preview
* Change the scale to some percentage setting (e.g. 40%)
* Change the scale to "shrink to fit".
Actual Results:  
Observe that the bottom approx 20% of the image is cut off.

Expected Results:  
The image should be resized so that it is as large as possible, whilst all fitting onto the page.

Comment 1

[Nick_Jenkins]

Attachment: Test image as an attachment, in case the URL stops working.

Comment 2

[Daniel Holbert [:dholbert]]

This actually isn't a "Shrink To Fit" problem.  I think you're (reasonably) misunderstanding what that option is for -- it's a bit misleadingly named -- it specifically refers to shrinking the page content until its **width** fits - NOT its height.

Height isn't considered for shrink-to-fit, because content below the 1st page generally just ends up on the 2nd page (or 3rd page, etc.)

However, in this case, you are hitting a real bug, because the tall image doesn't* end up on the second page -- it's just cropped. I bet there's a bug filed on that already, but just in case there isn't, I'm updating this bug's summary to describe that real issue.

Comment 3

[Nick_Jenkins]

Thank you for the explanation, it's very helpful.

> it specifically refers to shrinking the page content until
> its **width** fits - NOT its height.

Okay - and I agree that makes perfect sense for a web page, where the content is typically a long rectangle (for example, maybe 3 or 4 times as long as it is wide, and where you want to be able to read the text).

If you're looking at just an image however (e.g. just a JPEG, GIF, or PNG image), maybe "shrink to fit" could look at height, as well as width. Because a person who is printing an image is (in the vast majority of cases) going to want the whole image, as large as possible, on a single piece of paper - most especially if they have chosen "shrink to fit".

Under the current definition, "shrink to fit" for an image is not very useful (or rather: it is useful if you happen by sheer luck to be printing an image whose width/height ratio is the same as or higher than the printer's width/height page ratio; otherwise it's useless). If looking at height of images is not feasible, maybe images could have a "fit to single page" mode? Certainly, I think that's what most people want as reasonable defaults ("shrink to fit" for web pages, and fitting onto a single page for images).

> you are hitting a real bug, because the tall image doesn't*
> end up on the second page -- it's just cropped. I bet there's a bug

I agree that sounds like a bug, and that ideally all bugs should be fixed. But, from a purely personal viewpoint, rather than improving the option I won't use, I would prefer if there was an option that did something like what's described above.

Comment 4

[Michael Ventnor]

Attachment: Patch

I've decided to take this not only because it might help pdf.js, but also because I too noticed this bug while browsing long comics on Reddit.

The problem is that, at least in an image document, the image is the child of a line layout. Images already have the ability to paginate themselves, but only if they know what the size of the page is (using availableHeight). Line layouts break this by unconditionally setting the available height to unconstrained before reflowing the image.

We must use the line layout block's availableHeight, as others (including ComputedHeight()) are also unconstrained even in paginated mode.

Comment 5

[Michael Ventnor]

roc, is there any chance you can review this before the Fx7 merge?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This won't work. By setting an available height for inline children, you're claiming that they can break vertically and you'll reflow their continuations on the next page. But you don't.

Comment 7

[Michael Ventnor]

(In reply to comment #6)
> This won't work. By setting an available height for inline children, you're
> claiming that they can break vertically and you'll reflow their
> continuations on the next page. But you don't.

But this is the case, isn't it? The parent block frame reflows the continuations.

Is there something extra I need to do? For example call ReflowOverflowContainerChildren() if we have a PrevInFlow?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, it's far more complicated than that.

If we allowed lines to break vertically and horizontally at the same time, a line could have two kinds of overflow: inline elements that broke at the end of the line, with continuations that should be placed on the next line, and inline elements that broke vertically, with continuations that belong on the same line but on the next page. So in fact we'd have two kinds of "next line": the normal next line like we currently have, and a "vertical continuation line" that comes before the normal next line and contains the bottom halves of some parts of the line.

None of this infrastructure exists, and it would a ton of work to build it, and it's not worth it.

Comment 9

[Michael Ventnor]

Attachment: Patch 2

OK, I managed to figure out more of this.
Block lineboxes do respect available height, inline lineboxes don't. Whether it's block or inline depends on the child frame, in this case the image. Images are inline by default. By changing it to block, we get the page sizes passed through properly, allowing the image frame to create continuations.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 544164 [details] [diff] [review]
Patch 2

Review of attachment 544164 [details] [diff] [review]:
-----------------------------------------------------------------

Nice fix, I hadn't noticed that this bug was only about image documents.

::: content/html/document/src/ImageDocument.cpp
@@ +397,5 @@
>  
> +void
> +ImageDocument::UpdateStyle(const nsAString& aStyleCSS)
> +{
> +  // Bug 528046

Don't clutter the source up with bug numbers. We can get the bug number from hg annotate.

@@ +406,5 @@
> +  // the size of the paper and cannot break into continuations along
> +  // multiple pages.
> +  nsAutoString style;
> +  style.AssignLiteral("display: block; ");
> +  style.Append(aStyleCSS);

Instead of doing this, why not just insert a <style> element
into the document in nsImageDocument::CreateSyntheticDocument which contains "img{display:block}"? Then you wouldn't have to juggle dynamic changes to the style attribute.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 544164 [details] [diff] [review]
Patch 2

This fix looks good, but I like roc's comments as well. r=jst with those applied.

Comment 12

[Michael Ventnor]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3a695801d014

I thought it would affect more than just image documents, but image documents is where this is most common, I guess.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It does affect more than just image documents, yeah. I think we have bugs on that.

Comment 14

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/3a695801d014

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

innerHTML?

Comment 16

[Michael Ventnor]

(In reply to comment #15)
> innerHTML?

Yes. It's a virtual method that the style element overrides to properly make sure all notifications are sent and style data is updated.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Using innerHTML is overkill in that it would be more direct to just create a text node with the contents you want, and insert that as a child of the style element. However, it doesn't matter for performance here, and using SetTnnerHTML is a little less code. SetTextContent would be a little better though since it doesn't require the HTML parser, and the same amount of code.

I'd actually like to see a followup patch that uses SetTextContent instead of SetInnerHTML, and also instead of calling GetHeadElement you could just make the style element a child of 'body'.

Comment 18

[Michael Ventnor]

Attachment: Followup

Well, innerHTML is overridden by the style element to basically do just that, so the HTML parser shouldn't get involved, but I went ahead and changed it anyway.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 544688 [details] [diff] [review]
Followup

Review of attachment 544688 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 20

[Michael Ventnor]

Comment on attachment 544688 [details] [diff] [review]
Followup

http://hg.mozilla.org/integration/mozilla-inbound/rev/f10db0620790

Comment 21

[Marco Bonardo [:mak]]

merged followup http://hg.mozilla.org/mozilla-central/rev/f10db0620790

Comment 22

[Joe Wilson]

> Expected Results:  
> The image should be resized so that it is as large as possible, whilst all
> fitting onto the page.

It gets resized for me so that it uses 2 pages, instead of 1. Is it correct behavior?

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

> and also instead of calling GetHeadElement you could just make the
> style element a child of 'body'.

Which happened to break user styles: bug 677495.

Comment 24

[Boris Zbarsky [:bzbarsky]]

I think we should seriously consider undoing that part of the change (i.e. moving the style back into the head)...

Comment 25

[Boris Zbarsky [:bzbarsky]]

Especially because it changes web-facing behavior that used to be interoperable across browsers afaik and is explicitly called for in http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#read-image

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky (:bz) from comment #24)
> I think we should seriously consider undoing that part of the change (i.e.
> moving the style back into the head)...

That violates the spec too, though. So does our existing "style" attribute on the <img>... Where do we draw the line?

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh,
> User agents may add content to the head element of the Document, or attributes
> to the img element, e.g. to link to a style sheet or an XBL binding, to provide
> a script, to give the document a title, etc.

I'm not sure why the DOM interop line is drawn here, but OK. We just have to back out the change to put the <style> in the <body>.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> I'm not sure why the DOM interop line is drawn here

I think because that's where UAs currently have interop....  A lot of HTML5 is like that: specify the cases that are currently interoperable, leave more leeway for the ones that are not.

But yea, we just need to put the <style> in the <head>.