Closed Bug 966419 Opened 10 years ago Closed 10 years ago

Shrink-to-fit doesn't work when there's more than one page that scales

Categories

(Core :: Printing: Output, defect)

Product:
Core
Component:
Printing: Output
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

Details

(Keywords: testcase)

Attachments

(3 files, 2 obsolete files)

Testcase (plain text)
1.16 KB, text/plain
Details
Testcase (html)
1.24 KB, text/html
Details
fix+tests
7.62 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Attached file Testcase (plain text) — 
STR:
1. load the attached testcase
2. select View -> Page Style -> Basic Page Style
3. Print Preview or Print (with Shrink-to-fit enabled)

ACTUAL RESULTS
The long line on page 1 is clipped.

EXPECTED RESULTS
No line is clipped.
Attached file Testcase (html) — 
This is for paper size A4.
The bug does not occur if I remove the long line on page 2.

It appears we scale all pages based on the page which needs the
least amount of scaling?
Attached patch wip (obsolete) — — Splinter Review 
We set the document scale to fit the last reflowed page that overflows.
Assignee: nobody → matspal
Attached patch fix+test (obsolete) — — Splinter Review 
This calculates the scaling needed for each page (that overflows)
individually and stores the minimum value of all pages.

It should make shrink-to-fit not overflow horizontally, as intended.
(I find it rather surprising that no-one has discovered this bug
until now, because GetSTFPercent dates back to 2002 or so.)

https://tbpl.mozilla.org/?tree=Try&rev=17a3c1b6ea16
Attachment #8368786 - Attachment is obsolete: true
Attachment #8369044 - Flags: review?(dholbert)
Comment on attachment 8369044 [details] [diff] [review]
fix+test

>Bug 966419 - Update the global ScaleToFit value when the current page overflows and requires additional scaling to fit.  r=dholbert

Probably worth mentioning "shrink to fit" and "print" in the commit message somewhere, for easier regression-hunting in case this regresses something.

>diff --git a/layout/generic/nsPageContentFrame.cpp b/layout/generic/nsPageContentFrame.cpp
>-        mPD->mPageContentXMost =
>-          xmost +
>-          kidReflowState.mStyleBorder->GetComputedBorderWidth(NS_SIDE_RIGHT) +
>-          padding.right;
>+        nscoord widthToFit = xmost + padding.right +
>+          kidReflowState.mStyleBorder->GetComputedBorderWidth(NS_SIDE_RIGHT);

Nit: The old addition ordering made slightly more sense. It had:
 contents + border + padding
and you're changing it to:
 contents + padding + border

Maybe just leave this chunk as-is, and only do s/mPD->mPageContentXMost/nscoord widthToFit/

>+        float scaleToFit = float(maxSize.width) / widthToFit;

We should probably assert that the new scaleToFit value here is >= 0.0 and < 1.0, as a sanity-check that we're dividing the right quantities (and only running this code when we really need to).

>+  aSTFPercent = mPageData->mScaleToFit;

It's a bit confusing to have "STF" overloaded, for both "Shrink to fit" and "scale to fit". (which sound like they could mean different things, when really they're effectively the same. (one is the name of the print-mode, the other is the exact scale value))

Maybe rename the new mScaleToFit variable to something like "mShrinkToFitScale" or "mSTFScale" or something else like that, so the "STF" abbreviation keeps a clear meaning?

>+++ b/layout/generic/nsSimplePageSequence.h
>+  // The scale we need to apply to make all pages fit horizontally.  It's the
>+  // minimum "ComputedWidth / OverflowWidth" ratio of all page content frames
>+  // that overflows.  It's 1.0 if none overflowed horizontally.
>+  float mScaleToFit;
> };

Grammar nit: mixed pluralization in "frames that overflows".

s/overflows/overflow/  (or "overflowed")

>+++ b/layout/reftests/printing/966419-ref.html
>@@ -0,0 +1,13 @@
>+<!DOCTYPE html>
>+<html class="reftest-print">
>+<head><meta charset="utf-8">
>+<style type="text/css">
>+@page { size:5in 3in; margin:0.5in; }

This gives me "Unknown property 'size'.  Declaration dropped." in my error console.  Probably remove it?
( https://developer.mozilla.org/en-US/docs/Web/CSS/@page doesn't mention 'size' or any similar property being settable in @page)

Also: I don't understand why the reftest should pass right now. The testcase has an 8-inch-by-1em div, whereas the reference case has a 7-inch-by-1-em div. Both will be shrunk, of course, but I'd expect they shrink by different scales (due to the 8in vs. 7in discrepancy), so I'd expect they'd render differently.  Perhaps you meant to make them both the same size? Or maybe I'm just missing something. (The testcase's 6in hidden div on the 2nd page does make sense, of course.)

Also: It'd probably be good to include a second reftest where the widest thing is on the *second* page instead of the first page.
Attached patch fix+tests — — Splinter Review 
(In reply to Daniel Holbert [:dholbert] from comment #5)
> Probably worth mentioning "shrink to fit" and "print" in the commit message
> somewhere, for easier regression-hunting in case this regresses something.

Fixed.

> Nit: The old addition ordering made slightly more sense. It had:
>  contents + border + padding
> and you're changing it to:
>  contents + padding + border

In CSS Box Model terms, going from innermost and outward gives you
content then padding then border (then margin).  And viewing it from
the outside, from the parent POV, you have margin/border/padding/
content.  I don't see how the order content/border/padding makes sense.

> We should probably assert that the new scaleToFit value here is >= 0.0 and <
> 1.0, as a sanity-check that we're dividing the right quantities (and only
> running this code when we really need to).

Fixed.

> Maybe rename the new mScaleToFit variable to something like
> "mShrinkToFitScale" or "mSTFScale" or something else like that, so the "STF"
> abbreviation keeps a clear meaning?

I renamed it mShrinkToFitRatio. (to somewhat match mShrinkRatio in
nsPrintEngine)

> s/overflows/overflow/  (or "overflowed")

Fixed. (overflowed)

> >+++ b/layout/reftests/printing/966419-ref.html
> This gives me "Unknown property 'size'.  Declaration dropped." in my error
> console.  Probably remove it?

Yeah, this is boilerplate from some layout/reftests/w3c-css/submitted/ test
which requires that rule to set the page size for print tests.
I removed it.

> Also: I don't understand why the reftest should pass right now.

Hmm interesting... I developed this test without the fix applied using
normal Print Preview in the browser and made sure it failed, then applied
the fix and ran "mach reftest" to make sure it passed and it did.
Problem is, it appears reftest-print tests don't actually do shrink-to-fit
scaling at all...  I don't know why, maybe this:
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt#487

That's a reftest framework problem, and I'm not going to fix that here.
The tests should work as intended when the framework runs them properly.

> Also: It'd probably be good to include a second reftest where the widest
> thing is on the *second* page instead of the first page.

Fixed.

https://tbpl.mozilla.org/?tree=Try&rev=21b9d50e622e
Attachment #8369044 - Attachment is obsolete: true
Attachment #8369044 - Flags: review?(dholbert)
Attachment #8369730 - Flags: review?(dholbert)
(In reply to Mats Palmgren (:mats) from comment #6)
> In CSS Box Model terms, going from innermost and outward gives you
> content then padding then border (then margin).  And viewing it from
> the outside, from the parent POV, you have margin/border/padding/
> content.  I don't see how the order content/border/padding makes sense.

Oops, you're right, sorry. I just confused myself. Your new ordering is better, in that case. :)

> That's a reftest framework problem, and I'm not going to fix that here.
> The tests should work as intended when the framework runs them properly.

Agreed.

Would you mind filing a bug on that, and including a comment above this test saying something like:
 # NOTE: The tests 966419-* don't yet rigorously test what they're
 # trying to test (shrink-to-fit behavior), due to bug NNNN

That way, if & when someone comes along and fixes the bug, if it makes these tests fail (due to a bug in the tests that we've missed), it'll be clearer what's going on. (The tests would be failing because it'd be the first time they'd been fully tested, not because the fix "broke" them.)
(RE that followup bug -- I'd expect we should be able to do "ps.shrinkToFit = true;" inside of setupPrintMode(), IFF we have e.g. <html class="reftest-print-stf"> instead of "reftest-print":
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#163
nsIPrintSettings IDL file for reference: http://mxr.mozilla.org/mozilla-central/source/widget/nsIPrintSettings.idl
)
(and CC dbaron on the new bug - IIRC think he prefers to review reftest harness modifications himself)
(And if the followup harness bug is as simple to fix as I think it should be, it might be worth holding off on landing this & bug 960822 until it's fixed, so that the automated tests will be functional from the get-go & won't need a big caveat "NOTE" comment per comment 7.)
I added the suggested note in bug 960822.  I don't think we should block these
bug fixes on reftest framework improvements for the tests.
Comment on attachment 8369730 [details] [diff] [review]
fix+tests

Sounds good. Thanks!
Attachment #8369730 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/0b8fdcee7a26
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 1102791
No longer depends on: 1102791
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: