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

RESOLVED FIXED in mozilla8

Status

()

Core
Printing: Output
--
minor
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Nick_Jenkins, Assigned: Michael Ventnor)

Tracking

Trunk
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 411803 [details]
Test image as an attachment, in case the URL stops working.

Updated

7 years ago
Component: General → Print Preview
Product: Firefox → Core
QA Contact: general → printing
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Summary: "Shrink to fit" print preview of an image crops the image instead of printing all of it. → Print / print-preview crops very tall images at page boundaries
Version: unspecified → Trunk
Component: Print Preview → Printing: Output
Whiteboard: DUPEME
(Reporter)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
Assignee: nobody → ventnor.bugzilla
Attachment #543880 - Flags: review?(roc)
(Assignee)

Comment 5

6 years ago
roc, is there any chance you can review this before the Fx7 merge?
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.
(Assignee)

Updated

6 years ago
Attachment #543880 - Flags: review?(roc)
(Assignee)

Comment 7

6 years ago
(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?
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.
(Assignee)

Comment 9

6 years ago
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.
Attachment #543880 - Attachment is obsolete: true
Attachment #544164 - Flags: review?(jst)
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 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.
Attachment #544164 - Flags: review?(jst) → review+
(Assignee)

Comment 12

6 years ago
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.
It does affect more than just image documents, yeah. I think we have bugs on that.
http://hg.mozilla.org/mozilla-central/rev/3a695801d014
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Whiteboard: DUPEME
innerHTML?
(Assignee)

Comment 16

6 years ago
(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.
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'.
(Assignee)

Comment 18

6 years ago
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.
Attachment #544688 - Flags: review?(roc)
Comment on attachment 544688 [details] [diff] [review]
Followup

Review of attachment 544688 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #544688 - Flags: review?(roc) → review+
(Assignee)

Comment 20

6 years ago
Comment on attachment 544688 [details] [diff] [review]
Followup

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

Comment 22

6 years ago
> 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?
> 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.
I think we should seriously consider undoing that part of the change (i.e. moving the style back into the head)...
Depends on: 677495
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
(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?
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>.
> 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>.

Updated

6 years ago
Depends on: 724516
You need to log in before you can comment on or make changes to this bug.