Last Comment Bug 528046 - Print / print-preview crops very tall images at page boundaries
: Print / print-preview crops very tall images at page boundaries
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Printing: Output (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla8
Assigned To: Michael Ventnor
:
Mentors:
http://images.domain.com.au/img/20091...
Depends on: 677495 724516
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-11 14:36 PST by Nick_Jenkins
Modified: 2012-02-06 05:00 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test image as an attachment, in case the URL stops working. (109.32 KB, image/jpeg)
2009-11-11 14:38 PST, Nick_Jenkins
no flags Details
Patch (2.53 KB, patch)
2011-07-04 23:05 PDT, Michael Ventnor
no flags Details | Diff | Review
Patch 2 (3.31 KB, patch)
2011-07-06 00:42 PDT, Michael Ventnor
jst: review+
Details | Diff | Review
Followup (2.15 KB, patch)
2011-07-07 20:22 PDT, Michael Ventnor
roc: review+
Details | Diff | Review

Description Nick_Jenkins 2009-11-11 14:36:18 PST
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 2009-11-11 14:38:00 PST
Created attachment 411803 [details]
Test image as an attachment, in case the URL stops working.
Comment 2 Daniel Holbert [:dholbert] (largely AFK until June 28) 2010-07-09 16:16:26 PDT
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 2010-07-11 18:38:36 PDT
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 2011-07-04 23:05:32 PDT
Created attachment 543880 [details] [diff] [review]
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 2011-07-04 23:09:36 PDT
roc, is there any chance you can review this before the Fx7 merge?
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-04 23:12:43 PDT
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 2011-07-04 23:16:14 PDT
(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) (Exited; email my personal email if necessary) 2011-07-05 14:48:07 PDT
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 2011-07-06 00:42:14 PDT
Created attachment 544164 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-07-06 15:51:54 PDT
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, jst@mozilla.com) 2011-07-06 18:19:38 PDT
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 2011-07-06 21:06:59 PDT
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) (Exited; email my personal email if necessary) 2011-07-06 21:22:57 PDT
It does affect more than just image documents, yeah. I think we have bugs on that.
Comment 14 Marco Bonardo [::mak] 2011-07-07 03:34:18 PDT
http://hg.mozilla.org/mozilla-central/rev/3a695801d014
Comment 15 :Ms2ger 2011-07-07 04:56:41 PDT
innerHTML?
Comment 16 Michael Ventnor 2011-07-07 05:50:45 PDT
(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) (Exited; email my personal email if necessary) 2011-07-07 15:22:08 PDT
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 2011-07-07 20:22:45 PDT
Created attachment 544688 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-07-07 20:28:16 PDT
Comment on attachment 544688 [details] [diff] [review]
Followup

Review of attachment 544688 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 21 Marco Bonardo [::mak] 2011-07-08 06:00:31 PDT
merged followup http://hg.mozilla.org/mozilla-central/rev/f10db0620790
Comment 22 Joe Wilson 2011-08-09 06:59:28 PDT
> 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 2011-08-09 07:23:37 PDT
> 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 [:bz] (Out June 25-July 6) 2011-08-09 09:13:52 PDT
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 [:bz] (Out June 25-July 6) 2011-08-09 09:18:21 PDT
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) (Exited; email my personal email if necessary) 2011-08-09 16:23:14 PDT
(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) (Exited; email my personal email if necessary) 2011-08-09 16:25:01 PDT
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 [:bz] (Out June 25-July 6) 2011-08-10 10:36:40 PDT
> 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>.

Note You need to log in before you can comment on or make changes to this bug.