168961 - [FIX]ShrinkToFit doesn't correctly handle overflow:scroll

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[rods (gone)]

Comment 1

[rods (gone)]

Attachment: patch v1

The solution is:
STF does 2 reflows one to figure out how big it wants to be and the 2nd to
shrink it. The problem with this is the first reflow is not being done
unconstrained, so therefore the reflow mechanism isn't really giving us the
answer we want. And when we traverse the frames looking for the XMost frame we
aren't taking clipping into account. We really need to do an unconstrained
reflow first and then a constrained one 2nd (if needed)

The patch does:
1) Enables the PageContentFrame to do a unconstrained reflow and have it
remember what it "would" have been reflowed at constrained.

2) Have the nsPrintEngine tell the nsPrintOptions object when we should do/not
do a constrained reflow, this is then picked by the PageSequenceFrame when it
is created. (This is really the only meachism to communicate to the page
sequence frame)

3) Make available to the PageContentFrame the PageSeq's "SharedData" object.
This way it can use this SharedData object to cache the uncontrained value.
This is exactly what the SharedData object should be used for. Previously the
Page ContentFrame did not need to have access to the object so it was passed on
down to it.

4) Some minor general cleanup in nsPrintEngine, there were some places where we
were getting some objects we already had access to. So a few of the method args
were changed

5) Create forward decls for some of the debugging methods that are only turned
on for extended debugging. Without these it won't build.

Comment 2

[dcone (gone)]

Comment on attachment 99695 [details] [diff] [review]
patch v1

r=dcone

Comment 3

[kinmoz]

Comment on attachment 99695 [details] [diff] [review]
patch v1

sr=[e-mail]

With the following addressed:


==== We check |mPrt->mPrintOptions| when setting unconstrained reflow to
PR_TRUE, but not when we set it back to false. Will that cause a problem if it
is ever null? Also is there any possibility of returning early in code between
the 2 places that call |SetDoUnconstrainedReflow()|? If so, we need to reset to
PR_FALSE before returning right?


+  // If we are doing "shrink to fit" then request that the first
+  // reflow is constrained
+  if (mPrt->mShrinkToFit && mPrt->mPrintOptions && !ppIsShrinkToFit) {
+    mPrt->mPrintOptions->SetDoUncontrainedReflow(PR_TRUE);
+  }


....


   if (mPrt->mShrinkToFit && !ppIsShrinkToFit) {
+    // Turn off the request for unconstrained reflow
+    if (mPrt->mPrintOptions) {
+      mPrt->mPrintOptions->SetDoUncontrainedReflow(PR_FALSE);
+    }


==== Didn't review nsPrintObject.cpp and this since they aren't part of the
fix:


-
+  PRBool		       mDidAddRefDocViewer;



==== There's no need to check |mPD->mPageContextSizeUC != 0| because if we ever
do get to checking the 2nd expression we already know its non-zero.


+	 if (mPD->mPageContextSizeUC == 0 ||
+	     (mPD->mPageContextSizeUC != 0 && mPD->mPageContextSizeUC <
aDesiredSize.width)) {


==== Fix indentation:


+NS_IMETHODIMP 
+nsPrintOptions::SetDoUncontrainedReflow(PRBool aDoUncontrainedReflow)
+{
+ mDoUncontrainedReflow = aDoUncontrainedReflow;
+ return NS_OK;
 }

Comment 4

[rods (gone)]

Attachment: patch (with kin's suggestions)

Comment 5

[rods (gone)]

Comment on attachment 99872 [details] [diff] [review]
patch (with kin's suggestions)

bringing r/sr forward for final patch

Comment 6

[karnaze (gone)]

It looks like this patch was checked in on 9/19/2 and breaks the printing of a
lot of pages. Now, by not reflowing frames with a constrained width, most tables
do not balance and size properly, most tables do not split, other elements which
base their size on the viewport don't size properly.

The page frame must do a constrained width reflow on its children. 

This could have been detected by running the regression tests located in
layout/html/tests/block/printing and layout/html/tests/table/printing. Please
start running these tests.

Comment 7

[rods (gone)]

The last patch has been checked in, but we have discovered that reflowing
unconstrained is very bad. Although I did do a lot of testing, my tests didn't
cover some table issues.

Comment 8

[rods (gone)]

Attachment: patch v3

This partially backs out the previous patch. We no longer need to set/get
anything in the nsIPrintOptions.

What this does is:
1) Reflows the first time constrained and remembers mOverflowArea.XMost() of
the HTML frame and then later uses that for the calculation of the ratio.
2) Renames some variables
3) This also changes the the maximum STF ratio from 0.5 to 0.3 to match the
dropdown in PP

Comment 9

[Roland Mainz]

Minor nit:
-- snip --
       // Clamp Shrink to Fit to 50%
-      mPrt->mShrinkRatio = PR_MAX(mPrt->mShrinkRatio, 0.5f);
+      mPrt->mShrinkRatio = PR_MAX(mPrt->mShrinkRatio, 0.30f);
-- snip --

The comment should be fixed, too...

Comment 10

[dcone (gone)]

Comment on attachment 100436 [details] [diff] [review]
patch v3

r=dcone

Comment 11

[kinmoz]

Comment on attachment 100436 [details] [diff] [review]
patch v3

==== Is NOTYETIMPLEMENTED the right warning to be printing here? These methods
will only return false if the values weren't previously calculated and cached?


+      if (!kidReflowState.mStyleBorder->GetBorder(border)) {
+	 NS_NOTYETIMPLEMENTED("percentage border");
+      }
+      if (!kidReflowState.mStylePadding->GetPadding(padding)) {
+	 NS_NOTYETIMPLEMENTED("percentage border");
+      }


==== Shouldn't these be initialized somewhere?

+  nscoord	mPageContentXMost;	// xmost size from Reflow(width)
+  nscoord	mPageContentSize;	// constrained size (width)


     I ask because |mPageContentXMost| is *only* conditionally set here:


+      // First check the combined area
+      if (NS_FRAME_OUTSIDE_CHILDREN & kidState) {
+	 // The background covers the content area and padding area, so check
+	 // for children sticking outside the child frame's padding edge
+	 nscoord paddingEdgeX = aDesiredSize.width - border.right -
padding.right;
+	 if (aDesiredSize.mOverflowArea.XMost() > aDesiredSize.width) {
+	   mPD->mPageContentXMost =  aDesiredSize.mOverflowArea.XMost() +
border.right + padding.right;
	 }
       }


==== As Roland pointed out earlier, this needs to be changed to say 30%:

       // Clamp Shrink to Fit to 50%
-      mPrt->mShrinkRatio = PR_MAX(mPrt->mShrinkRatio, 0.5f);
+      mPrt->mShrinkRatio = PR_MAX(mPrt->mShrinkRatio, 0.30f);

Comment 12

[rods (gone)]

Attachment: patch v4

They should be initialized (but they would never be called if a reflow hadn't
been done first which would set the values) I initilized them

I decided to not even check the return values, if the border and padding ends
up being zero it won't greatly affect the output and having it print is better
than having it fail.

This:
+      mPD->mPageContentSize  = aReflowState.availableWidth;

Does get set before the other.

Comment 13

[rods (gone)]

Comment on attachment 100626 [details] [diff] [review]
patch v4

minor changes from Kin's comments bringing don's r= forward

Comment 14

[rods (gone)]

Attachment: patch v5

wrong file, this should be right

Comment 15

[rods (gone)]

Comment on attachment 100661 [details] [diff] [review]
patch v5

bring r= forward

Comment 16

[kinmoz]

Comment on attachment 100661 [details] [diff] [review]
patch v5

sr=[e-mail]

Comment 17

[rods (gone)]

fixed