960822 - Print preview of the google.com source is effectively blank, with (default) Shrink-to-Fit scale behavior, due to absurdly long line

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P4)

Description

[Daniel Holbert [:dholbert]]

STR:
 Load https://www.google.com/
 View source (Ctrl + U)
 Print preview

ACTUAL RESULTS:
Blank page in print preview.

(NOTE: I can print-preview the source of other pages correctly,  e.g. the source of http://www.mozilla.org/en-US/foundation/licensing/website-content/ works fine. )

Comment 1

[Daniel Holbert [:dholbert]]

Ah, it's because the source contains an absurdly-long line, and the (default) shrink-to-fit behavior makes things so small as to be invisible, in order to fit that line entirely on the page.

D'oh.

Still, I wonder if there's anything we can do to make this less busted. (Maybe we should cap how much shrink-to-fit is allowed to shrink things)

Comment 2

[Mats Palmgren (inactive)]

This bug doesn't occur for plain text documents though (with View->Page Style
set to "Basic Page Style").  At first glance, it appears we scale it slightly
but not enough to actually fit, with the result that long lines are clipped.
I don't know if it's intentional or just a bug in shrink-to-fit for plain text.

Comment 3

[Mats Palmgren (inactive)]

Adding a cap seems like a good idea, but perhaps we should exclude some cases?
Like SVG and image documents?

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix

This limits the scaling to 25% of the desired width.  It's configurable
with the pref "print.shrink-to-fit.scale-limit-percent".

Comment 5

[Mats Palmgren (inactive)]

Attachment: screenshot (View Source of Google.com)

This is what View Source of Google.com looks like in Print Preview
with the patch.  Maybe I should have made it smaller still to make it
clear that Shrink-To-Fit is a bad option for this content...

Comment 6

[Mats Palmgren (inactive)]

Attachment: same thing with a 20% limit

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix

I think I prefer 20% actually, since the intended purpose is to make it clear
that shrink-to-fit is a bad choice.  Also, it makes more content fit vertically
on the first page which makes it even more obvious.

https://tbpl.mozilla.org/?tree=Try&rev=7d1dce4f40bc

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8369076 [details] [diff] [review]
fix

>+    nsIDocument* doc = aPO->mPresShell->GetDocument();
>+    nsAutoString contentType;
>+    doc->GetContentType(contentType);

'doc' is only used once. Maybe just drop its decl and inline it?

aPO->mPresShell->GetDocument()->GetContentType(contentType);

>+    contentType.SetLength(5);
>+    if (contentType.EqualsLiteral("text/")) {

s/SetLength/Truncate/, to convey that we're intending to *shorten* the string.

We probably should check that the length is >= 5 before calling this, too, to be on the safe side. (since Truncate only actually checks the length in debug builds)

>+      int32_t limitPercent = 
>+        mozilla::Preferences::GetInt("print.shrink-to-fit.scale-limit-percent", 20);

Drop the "mozilla::" prefix. (This file already has "using namespace mozilla".)

>+      float minShrinkRatio = float(limitPercent > 0 ? limitPercent : 0) / 100;
>+      aPO->mShrinkRatio = std::max(aPO->mShrinkRatio, minShrinkRatio);

(So we're enforcing a hard lower-limit of 1% (effectively) on minShrinkRatio here. Do we really need that (as opposed to letting it be 0)? I suppose it increases the likelihood that we'll print *something*, which is probably useful.)

We should make sure minShrinkRatio isn't greater than 100. (Otherwise someone could end up setting a "lower limit" of 200% scale, which would then *seriously* bust their shrink-to-fit behavior.)

Maybe something like this (assuming you want to keep your 1% lower-limit):
 limitPercent = std::max(1, limitPercent);
 limitPercent = std::min(100, limitPercent);

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js
>+// Print/Preview Shrink-To-Fit scales at most to 20% for text/* documents.
>+pref("print.shrink-to-fit.scale-limit-percent", 20);

"scales at most" sounds a bit odd, for a lower-limit. Maybe "won't shrink below"?

r=me with the above. Thanks for fixing this!

Comment 9

[Daniel Holbert [:dholbert]]

Also: this should be able to include a test.

For 20% shrink on a 5-inch-wide reftest-page, you'd need some content > 25 inches wide.

Maybe check that a div whose border-box is 30-inches wide (with a solid thick border) renders the same as a div whose border-box is 25 inches wide?

Comment 10

[Daniel Holbert [:dholbert]]

er, sorry -- that won't quite work. I meant to compare two both-overflowing divs (and the 25-inch div wouldn't quite overflow).

Better:  a 30-inch-wide div should render the same as a 100-inch-wide div. (Both should be shrunk down to 20%, and then overflow off the right side of the page.)

Comment 11

[Mats Palmgren (inactive)]

Attachment: fix+test

(In reply to Daniel Holbert [:dholbert] from comment #8)
> s/SetLength/Truncate/, to convey that we're intending to *shorten* the
> string.

Fixed.

> We probably should check that the length is >= 5 before calling this, too,
> to be on the safe side. (since Truncate only actually checks the length in
> debug builds)

Fixed.

> Drop the "mozilla::" prefix. (This file already has "using namespace
> mozilla".)

Fixed.

> >+      float minShrinkRatio = float(limitPercent > 0 ? limitPercent : 0) / 100;
> >+      aPO->mShrinkRatio = std::max(aPO->mShrinkRatio, minShrinkRatio);
> 
> (So we're enforcing a hard lower-limit of 1% (effectively) on minShrinkRatio
> here.

Not really, 'minShrinkRatio' would be zero when the pref is zero or negative
which means the ratio is unlimited.  But your suggested "valid percentage"
check is clearer.  Does this look OK?

> >+// Print/Preview Shrink-To-Fit scales at most to 20% for text/* documents.
> >+pref("print.shrink-to-fit.scale-limit-percent", 20);
> 
> "scales at most" sounds a bit odd, for a lower-limit. Maybe "won't shrink
> below"?

Fixed.

Also, text/* probably doesn't match all documents we want - for example
application/xhtml+xml, which I've added.  Do you know other that are
commonly used?
(I don't think it's super-important to be exhaustive here though)

I've added a test, with the caveat noted in bug 966419 that these
test aren't very useful at the moment.

https://tbpl.mozilla.org/?tree=Try&rev=21b9d50e622e

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #11)
> > >+      float minShrinkRatio = float(limitPercent > 0 ? limitPercent : 0) / 100;
> > >+      aPO->mShrinkRatio = std::max(aPO->mShrinkRatio, minShrinkRatio);
> > 
> > (So we're enforcing a hard lower-limit of 1% (effectively) on minShrinkRatio
> > here.
> 
> Not really, 'minShrinkRatio' would be zero when the pref is zero or negative

Ah right -- I was focusing on the fact that you were clamping for anything less than 1, but I suppose you were clamping those things to 0.

In any case: yup, clearer now.

> Also, text/* probably doesn't match all documents we want - for example
> application/xhtml+xml, which I've added.  Do you know other that are
> commonly used?

Not offhand.

> (I don't think it's super-important to be exhaustive here though)

Agreed.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8369740 [details] [diff] [review]
fix+test

># HG changeset patch
># Parent 73b0aac446d1fc011c57aa2103c911f40017584e
># User Mats Palmgren <[e-mail]>
>Bug 960822 - Limit the Shrink-To-Fit scaling for documents with a text-ish content type so that the content is still visible with extremely long lines.  r=dholbert
>
>diff --git a/layout/printing/nsPrintEngine.cpp b/layout/printing/nsPrintEngine.cpp
>--- a/layout/printing/nsPrintEngine.cpp
>+++ b/layout/printing/nsPrintEngine.cpp
>@@ -2028,16 +2028,35 @@ nsPrintEngine::UpdateSelectionAndShrinkP
>+    // Limit the shrink-to-fit scaling for some text-ish type of documents.
>+    nsAutoString contentType;
>+    aPO->mPresShell->GetDocument()->GetContentType(contentType);
>+    if (contentType.Length() > 21) {
>+      contentType.Truncate(21);
>+    }
>+    bool applyLimit = contentType.EqualsLiteral("application/xhtml+xml");

I'd probably skip the Truncate(21) chunk there. I think that only would "help" in case some new mimetype is created that has application/xhtml+xml as a prefix. That doesn't seem to merit having a special-case for here. (and even if that were to happen, we don't know for sure that we'd want to apply this logic to that new mimetype).

>+    if (contentType.Length() > 5) {
>+      contentType.Truncate(5);
>+    }
>+    applyLimit = applyLimit || contentType.EqualsLiteral("text/");

Seems like this should all be inside of "if (!applyLimit)".

But, it'd be even better if we refactored all of this string-munging into a helper-function (which maybe takes aPO and returns a bool).  That function can then simply "return true" early in all of the match cases. (so we can do away with "applyLimit" entirely.

Also: Have you verified that this doesn't impact image documents? (e.g. JPG)  Just want to be sure, because IIRC we render images in "fake" HTML pages these days (for background & centering purposes), and I could imagine that throwing off your mimetype check. It'd be nice to include a test for that, too, though it won't be very useful without the ability to force shrink-to-fit behavior.

Comment 14

[Mats Palmgren (inactive)]

Attachment: fix+test

(In reply to Daniel Holbert [:dholbert] from comment #13)
> I'd probably skip the Truncate(21) chunk there. 

Fixed.

> Seems like this should all be inside of "if (!applyLimit)".

Meh, it's not worth it from a performance POV and it doesn't make
the code clearer IMO; rather it's just more code, so I left it
as is.

> Also: Have you verified that this doesn't impact image documents? (e.g. JPG)

I verified Print Preview is using image documents.

> Just want to be sure, because IIRC we render images in "fake" HTML pages

Yep, good point. I suspect that a whole lot of tests will start
failing if that happens though so it doesn't seem worth the hassle
of writing a test.

I filed bug 967311 on the reftest framework and added the note
you suggested in the other bug.

Comment 15

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #14)
> Yep, good point. I suspect that a whole lot of tests will start
> failing if that happens though 

Actually, now that I think about it, the reftest framework doesn't
support testing raw images or text files etc, since it needs
class="reftest-print" on the document element. (AFAIK)

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #14)
> > Also: Have you verified that this doesn't impact image documents? (e.g. JPG)
> 
> I verified Print Preview is using image documents.

Right -- but do we run this code for those documents? (i.e. do we have contentType="text/html", or "image/jpeg" for those documents?)

If it's the former, then this might be problematic, since it could break shrinking of huge images (unless we're smart enough to auto-size the image in image-documents in ways that don't require STF).

(In reply to Mats Palmgren (:mats) from comment #15)
> Actually, now that I think about it, the reftest framework doesn't
> support testing raw images or text files etc, since it needs
> class="reftest-print" on the document element.

Ah, right. I guess that means you can't easily include a "print this large image" reftest (even aside from the inability to do shrink-to-fit). OK. Still worth manually checking the mimetype before landing this, though.

Comment 17

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #16)
Yes, I checked the document content type is image/jpeg in Print Preview.
That's what I meant by "image document" (the class is named ImageDocument).

Comment 18

[Daniel Holbert [:dholbert]]

Ah, great. Thanks!

Comment 19

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53489b3e14f1

Comment 20

[[e-mail]]

Comment on attachment 8369761 [details] [diff] [review]
fix+test

>+    bool applyLimit = contentType.EqualsLiteral("application/xhtml+xml");
>+    if (contentType.Length() > 5) {
>+      contentType.Truncate(5);
>+    }
>+    applyLimit = applyLimit || contentType.EqualsLiteral("text/");
>+    if (applyLimit) {
FYI I would have written this as
if (contentType.EqualsLiteral("application/xhtml+xml") ||
    StringBeginsWith(contentType, NS_LITERAL_STRING("text/"))) {

Comment 21

[[e-mail]]

We also appear to have a _mimeTypeIsTextBased function in findbar.xml, I wonder whether that's the sort of thing we should have more globally available.

Comment 22

[Mats Palmgren (inactive)]

Attachment: Neil's suggestion

(In reply to [e-mail] from comment #20)
> FYI I would have written this as
> if (contentType.EqualsLiteral("application/xhtml+xml") ||
>     StringBeginsWith(contentType, NS_LITERAL_STRING("text/"))) {

Yeah, that's much better, thanks!

I was actually looking for some method like that in the header
file but couldn't find it, since uh, it's not a method.  I keep
forgetting that some of that API is still C-oriented.

Comment 23

[Mats Palmgren (inactive)]

(In reply to [e-mail] from comment #21)
> We also appear to have a _mimeTypeIsTextBased function in findbar.xml, I
> wonder whether that's the sort of thing we should have more globally
> available.

Assuming everyone agrees on which mime types to include, yes.
"\+xml$" seems overly broad - wouldn't that match valid SVG mime types?
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#650

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/53489b3e14f1

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #22)
> I was actually looking for some method like that in the header
> file but couldn't find it, since uh, it's not a method.

(Same here. Should've just asked in IRC. Thanks, Neil!)

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #23)
> "\+xml$" seems overly broad - wouldn't that match valid SVG mime types?

(Yeah - but that's arguably correct for "find-in-page" behavior, which I'm assuming is what "findbar.xml" is about. Find-in-page works nicely e.g. on this demo:
 http://svg.kvalitne.cz/worldlandmarks/worldlandmarks.svg

Anyway, I agree that we probably don't want this 20% shrink-limit to apply to SVG, so we probably don't want to share the findbar's definition of what's text-ish.)

Comment 27

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62a1d33b9fe0

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62a1d33b9fe0