153080 - [FIX] Print (Preview): Scale (Shrink To Fit, or nn%) broken !?

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Roland Mainz]

I have the "feeling" that shrink-to-fit is somehopw broken. Printing URLs like
http://www.yahoo.co.jp or http://www.top500.org/list/2002/06/ always draws over
the right border...

Comment 1

[Roland Mainz]

I forgot the build info: 2002-06-13-08-trunk on Solaris and Linux.

Trying to reduce the scale to 50% does not work either... ;-(

Comment 2

[rods (gone)]

Attachment: testcase

The problem in the testcase is the "font-size" style set to 10pt. If that is
removed it does STF fine. I think the issue here is that the style is cached or
it font size isn't being recalculated.

Comment 3

[rods (gone)]

Attachment: patch

The FontSize needs to be scaled or zoom depending on whether we are printing/PP
or in Galley mode. At this time they are mutually exclusive.

I changed the helper function to be able to do zoom or scale depending on the
above. The DocumentViewer calculates the scale and sets it into the PresContext
if scaling is needed. The RuleNode then retreives the scale from the
PresContext to scale the font size accordingly.

The helper function also now supports un-doing the scale/zoom for where it is
needed.

Comment 4

[dcone (gone)]

Comment on attachment 89258 [details] [diff] [review]
patch

r=dcone

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

At a quick glance (I haven't tested yet), I'm guessing that you're now going to
be double-scaling any font sizes specified in px units.  I think the correct
solution to this problem is a little more complicated, and it will also serve as
a usable solution for a "full zoom" feature.

In particular, I think what you really want to do is give the document viewer
(or pres context or whatever) getters and setters for both canonical-pixel-scale
and zoom (or scaling or whatever).  Then you want to give it a getter that
returns the product of the two, which is what most callers of
GetCanonicalPixelScale should use, except for those that are doing unit
conversions from pixels to twips rather than those doing final rendering.  Or
something like that.  I need to think it through in a little more detail, and
look at the patch more closely.

Comment 6

[rods (gone)]

Attachment: better testcase

Comment 7

[rods (gone)]

Attachment: better patch

Yes, all I need to use is the CanonicalPixelScale from the DC, then I just need
to know when NOT to do the scaling. No changes are needed in the PresCOntext or
DV.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: screenshot of attachment 89415, printed, both before (above) and with (below) patch

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, a full review this time:


At a syntactic complexity level the patch would be greatly simplified if
you changed the the signature to:

  static float
  FontZoomFor(nsIPresContext* aPresContext)

and then made changes to the callers that were like:

       fontData->mSize = fontData->mFont.size =
-          ZoomFont(mPresContext, fontData->mFont.size);
+          nscoord(fontData->mFont.size * FontZoomFor(mPresContext));


-      nsCOMPtr<nsIDeviceContext> dc;
-      aPresContext->GetDeviceContext(getter_AddRefs(dc));
-      float textZoom;
-      dc->GetTextZoom(textZoom);

-      nscoord parentSize = nscoord(aParentFont->mSize / textZoom);
+      nscoord parentSize = nscoord(aParentFont->mSize / GetZoomFor(aPresContext));


However, I'm still a bit worried about correctness.  Using the canonical
pixel scale seems like it could do weird things to printing, as opposed
to print preview.  I tested printing your testcase with and without the
patch, and both sets of output are incorrect, but in different ways.
I've attached, as attachment 89699 [details], a screenshot of attachment 89415 [details],
printed, both before and after the patch.  A more thorough testcase
might be http://www.w3.org/Style/CSS/Test/CSS1/current/test526.htm , and
it's certainly important to test printing in addition to print preview
since the canonical pixel scale becomes something other than 1.

So what's wrong?  The basic problem is that you want to do scaling while
printing.  We already have a bunch of things involved in scaling:

 1. We have text zoom, which only scales text, so it's not sufficient or
    really even related.

 2. We have the pixels to twips value.  This defines the conversion
    between physical lengths (used internally) and a reference pixel, as
    described in section 4.3.2 of CSS2, which describes the meaning of a
    pixel unit in CSS or pixel-sizes in HTML or the intrinsic sizes of
    images.  This prevents sizes specified in pixels from "shrinking" by
    a factor of 5 when going from a 120ppi screen display to a 600ppi
    printer.

    nsDeviceContextPS sets p2t so that a reference pixel is 1/72 of a
    point.

    On some interfaces pixels are called dev units.

 3. We have the app units to dev units value.  This is the same as the
    pixels to twips value, except that for printing, we use the value
    from the non-printing context.

 4. We have the canonical pixel scale.  This is typically the ratio of
    the previous two values.  In other words, if the display is 96ppi
    and the reference pixel on the printer is 72ppi, then this value is
    ??? (which way?).  [Before today, I seriously misunderstood what the
    canonical pixel scale value was.  I'm still only basing my
    understanding on a quick reading of the code and not any testing to
    look at the actual values when printing.]

Based on these descriptions, we have three values representing only two
independent pieces of information:
 1. the ratio of reference pixels and physical lengths, where the native
    units of the display are either one or the other
 2. the ratio of those two things to twips.
[Note that physical pixels aren't involved here at all.  I used to think
the canonical pixel scale involved physical pixels]

These three values could represent a third thing, an overall scale, and
there does need to be a third value for overall scale, since documents
have lengths map to both reference pixels (px units, intrinsic sizes of
images, HTML unitless numbers meaning pixels, default font sizes in
prefs) and to physical lengths (CSS units for physical lengths, default
font sizes for printing (I hope).  However, this would require clear
definitions of when all these values should be used, and I don't see
those in nsIDeviceContext.h, and I somehow doubt they're used
consistently across the app.

So, that said, I'd like to see clear comments in nsIDeviceContext.h
defining the model through which we can use these three values for
scaling so that we define the "right" way to fix scaling bugs.  (It
would also be nice if we, at some point, renamed some of the functions
to use consistent terminology (pixels and dev units; twips and app
units).)  Without such rules, it's a little hard to know which changes
are correct and which aren't, since the values don't seem to be the
logical ones to pick if one were designing a system that allowed for
such conversions. (For example, in an ideal system, I might want to have
three constants for:
 1. converting reference pixels to app units (which would be points for
    printing or fractions of pixels for display)
 2. converting physical lengths to app units, and
 3. an overall scale.)

The screenshot I attached also seems to show bugs in scaling of font
sizes even without any special shrink-to-fit.  It might be useful to fix
those bugs first before attempting to fix this one, since those are
problems that show up even when these three values represent only two
independent concepts and the third can be computed from the other two.

Or am I misunderstanding something here?

Comment 10

[dcone (gone)]

What you said is correct.. but a few clarifications.
1.)  DevUnitsToAppUnits.. .. it is PixelsToTwips.. Dev (Device) units are Pixels
and App (Application) units are twips.  These should be renamed and/or we should
have just one set of methods to convert these.  Right now we have a variety of
methods that do the same thing.

2.)  The canonical pixel scale maps a DevUnit from one DeviceContext to another
DeviceContext  I think the original author made it for converting from pixels to
pixels depending for things like scroll bars.. or things that were based on a
pixel scale not on a twips scale.  For example.. windows have scroll bars and
can return the width of scroll bars.  Printer have no such thing, how do I get
easily get this measurment to the printer?   DO not use this value for
converting twips based on one device to twips for another device.


3.)  Name needs to be changed on the canonical.. this is so misleading.. or
confusing.  No one gets it right or can even pronounce this.

Comment 11

[dcone (gone)]

So.. for a start we should

1.)  consolidate the mTwipsToPixels and mAppUnitsToDevUnits.  
2.)  consolidate the mPixelsToTwips and mDevUnitsToAppUnits.

From what I can see.. these are always the same values.

3.) rename above to something like CoreUnitsToDevUnits and DevUnitsToCoreUnits.

4.) rename the Canonical to DeviveToDeviceRatio.

To follow.. my ideas on what should be done to fix the scaling problem..

Comment 12

[rods (gone)]

Attachment: full patch

This patch has the DV set the current scale into the PresContext.
When the font is Points it uses only the scale to change its size, otherwise it
uses the ZoomScaleFont method that uses the CanonicalPixelScale after
converting the twips to Pixels.

Comment 13

[dcone (gone)]

Comment on attachment 91153 [details] [diff] [review]
full patch

r=dcone

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 91153 [details] [diff] [review]
full patch

This patch:
 1. doesn't address my comments on the ZoomFont function in comment 9.
 2. Doesn't explain why you're doing what you're doing, which isn't 
    obvious at all and which needs to be explained.
 3. Makes the |aDoZoom| cases asymmetric for the printing case, which 
    seems incorrect.
 4. Ignores the text zoom parameter for printing, which someone may want 
    to use at some point.  We have no reason to want to break it.

Comment 15

[rods (gone)]

Overview:
First off, the "text zoom" is not used when printing or in print preview. I
deliberately turn all text zoom (set it 1.0). In order to a "real" zoom for PP
we need to step back an look at the entire problem of zoom the entire
presentation of the page and may be a transformation matrix. The way text zoom
works is to actually reflow the document. One might want to argue that we should
somehow convert the text zoom value into a meaningful 'scale" value for Printing
and PP.

As it stands, the size of the font has already been converted to twips and we
need to convert it back to pixels and use the canaonical pixel scale to get the
right value. That is unless the twips values for the font size came from points
and then we want to use the current "scale" to directly convert the value.

The canonical pixel scale is a scaling value between two devices, the screen and
the printer. For scaling in Printing and PP we take that value and multiply it
times the user's desired scale value (in a way it is kinda of a hybred value). 

So at this point in the execution the only way to know what the true scale
percentage is, is to add a new method to set/get it from the PresContext, which
is what I have done.

The aDoZoom don't mean mean do zoom or do-scale it means zoom or unzoom, scale
or unscale. Look at the various ZoomScaleFont calls and one of them needs to
undo the zoom/scale.

The only "special case" I have added in all this code is when we discover that
the font is a "point" value which is always converted exactly to the same twips
value no matter what the canonical pixel scale is or what device we are going
to. Therefore we want to use only the true scale for it.

I also don't want to use the suggestion of ZoomFontFor because that just
involves more code everywhere else where we woul have to do all the converting
to and from pixels.

I should add more comments and will put up another patch with those, but this is
the best approach for now until we want to address a full zoom feature which
would have more to do with "magnification" than text zooming.

Comment 16

[rods (gone)]

Attachment: An even more complete test

Comment 17

[rods (gone)]

Attachment: patch

The nsRuleNode is responsible for zooming and scaling fonts. Right now in
Printing/PP we cache the current Zoom value and then set it to 1.0 and then
allow for the scaling of the presentation.

The RuleNode has all Zooming and Scaling on font size computed in
ZoomScaleFont. It checks to see if we are in Paginated mode and then does
scaling. ZoomScaleFont is also capable of un-zooming or un-scaling.

The trick is that all fonts with sizes defined in pixels already have the scale
built into their value. This is because, before doing reflow the DocumentViewer
(DV) calculates the scale (or it is set by the user) and then multiples it by
the CanonicalPixelScale and resets the CanonicalPixelScale's value. So from the
statr when everything is created they use the CanonicalPixelScale with the
scale already built into it. 

So this means as pixel sized fonts are created and they convert their size to
twips they are doing the scaling at that time. Point size fonts would typically
get directly converted over to twips via a constant, but now they are
multiplied by the scale.

------- Summary ---------
Zooming:
Zooming only effects the size of fonts and nothing else, that's why it is
referred to as "text zooming". It never takes into account the
CanonicalPixelScale.

Scaling:
Scaling scales everything. It does this by setting the CanonicalPixelScale that
most all calculates use in one form or another for calculating there size for
the dislpay. The bug is really that the Point size fonts don't get there size
correctly scaled. This is because of the direct conversion from Points to
twips.

So what this patch does is the following:
It scales all Point size font when Printing/PP. It changes the function
ZoomFont to ZoomScaleFont. It makes sure that when Printing/PP pixel sized
fonts are not scale (because they already have been) and that point sized fonts
are.

The changes to the DocumentViewer and PresContext are for getting the correct
scale value and storing it in the PresContext so it can be used by the
RuleNode.

Note: I have tested Printing, Print Preview and Text Zooming. This patch
actually fixes a couple of problems with text zooming also.

Comment 18

[dcone (gone)]

Comment on attachment 92776 [details] [diff] [review]
patch

r=dcone

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: screenshot before and after patch

This is a screenshot of what PostScript printing of attachment 89415 [details] looks like
before (without) and after (with) the patch in attachment 92776 [details] [diff] [review].  This looks
similar to my previous screenshot and still seems like an unacceptable
regression.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 92776 [details] [diff] [review]
patch

The regression of this patch with the PostScript printing module (on Linux)
seems unacceptable.  I presume you've tested this a good bit, so I'm guessing
you're doing your testing on Windows.  Based on my memory of looking at some of
the printing code a few months ago and based on the symptoms I'm seeing in the
screenshot, I suspect that the problem here is that the Windows printing code
and the PostScript printing code are doing different things -- i.e., they're
reporting different numbers for these various values.

I suspect this comes back to the question of defining what the model is (see
comment 9).  Have we documented the current model of what these three numbers
mean well enough to know which one is right?  Which one is right?  Why?  Can we
fix the other one easily?

Comment 21

[rods (gone)]

Looking at the screenshot, I don't see a regression. I see that "medium" and
"font-size: 12pt" still aren't right on Linux, but overall it is an improvement.
Please point out the regression.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

12pt is right in the upper screenshot, but way too small in the lower.  Still,
why isn't this working cross-platform?  If the model were well-defined, it would
just work.

Comment 23

[Roland Mainz]

David Baron wrote:
> 12pt is right in the upper screenshot, but way too small in the lower.  Still,
> why isn't this working cross-platform?  If the model were well-defined, it 
> would just work.

1. The results using attachment 92776 [details] [diff] [review] do not differ much from the output when
the original shrink-to-fit code was working properly.
I don't see a reason why this patch should be blocked.
2. The PostScript module has more than one bug related to font size
calculations. I would not wonder when we hit one or the other issue there...

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*Why* does the postscript module have these issues?

Comment 25

[Roland Mainz]

David Baron wrote:
> *Why* does the postscript module have these issues?

What do you want to hear ? I am not sure whether the authors did any tests with
small font sizes and/or a scaling factor != 1.0 when the code was written in
~1999.

Comment 26

[rods (gone)]

Yes, I think the patch should go in also. Think of this patch as fixing the XP
portion of the font sizing/scaling/zooming bug. Then we can open a new bug for
the Linux specific issues (or maybe there are bugs already filed).

If the font sizing is already wrong it can't really be a regression....Also,
once this is in it can help us better understand what is wrong with Linux. 

Plus, this patch fixes some problems with text zooming as it is.

Comment 27

[Roland Mainz]

Is there a slight chanche that this patch gets it's way into the 1.0.1-branch or
should we add a comment to the "1.0.1 release notes" that the "shrink-to-fit
functionality has been regressed since 1.0 and that people have to use either
1.0FCS or wait for fixed trunk builds to get usefull printouts on paper" ?

Comment 28

[Kevin McCluskey (gone)]

nsbeta1+. This is a very visible bug, since fit to page is the default setting.

Comment 29

[rods (gone)]

Attachment: Updated patch

The patch need to be re-merged with the tree.

Comment 30

[rods (gone)]

Attachment: re-merged patch

Comment 31

[rods (gone)]

Attachment: screen shot of print output with re-merged patch

This is the output from the "re-merged" patch. It now looks correct. I can't
explain why the previous screen shots were not correct.

This patch is ready to be checked in.

Comment 32

[rods (gone)]

Attachment: screen shot of print output with re-merged patch

Try this again

Comment 33

[dcone (gone)]

Comment on attachment 97997 [details] [diff] [review]
re-merged patch 

r=dcone

Comment 34

[nnooiissee]

I don't know what kind of bugs drivers is targeting for the 1.0 branch, but is
there any chance that this type of fix would be wanted for 1.0.2?

This is a bug that has not only made Mozilla evangelism harder, but has forced
me to use IE on a couple of occasions.

And I might as well ask, is there any document that defines what kind of patches
will be considered on the 1.0 branch?

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 97997 [details] [diff] [review]
re-merged patch 

>Index: content/base/src/nsRuleNode.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsRuleNode.cpp,v
>retrieving revision 1.43
>diff -u -r1.43 nsRuleNode.cpp
>--- content/base/src/nsRuleNode.cpp	4 Sep 2002 02:31:46 -0000	1.43
>+++ content/base/src/nsRuleNode.cpp	5 Sep 2002 19:50:34 -0000
>@@ -204,7 +204,19 @@
> {
>   NS_ASSERTION(aValue.IsLengthUnit(), "not a length unit");
>   if (aValue.IsFixedLengthUnit()) {
>-    return aValue.GetLengthTwips();
>+    // When Printing/PP scale the Point size here when it is initially calculated
>+    // NOTE: Point size fonts have nothing to do with CanonicalPixelScale
>+    //       and all fonts that have pixel derrived sizes have already been scaled
>+    //       by the CannonicalPIxelScale
>+    PRBool isPaginated = PR_FALSE;
>+    aPresContext->IsPaginated(&isPaginated);
>+    if (isPaginated) {
>+      float scale;
>+      aPresContext->GetPrintScale(&scale);
>+      return NSToIntRound(float(aValue.GetLengthTwips()) * scale);
>+    } else {
>+      return aValue.GetLengthTwips();
>+    }

This comment runs past the 80-character limit.

Also, I don't see why, to implement a full zoom, you have to be mucking around
at this level of unit calculation.  It seems like this change is at the wrong
point.

>   }
>   nsCSSUnit unit = aValue.GetUnit();
>   switch (unit) {
>@@ -1621,14 +1633,44 @@
>   return res;
> }
> 
>+/**
>+ * This helper function will zoomScale OR scale the font depending on whether we are in
>+ * Galley mode or Printing (Print Preview).
>+ * 
>+ * aDoZoomScale - indicates whether we should zoom or unzoom, scale or unscale 
>+ *
>+ * NOTE: We only want to scale fonts using the CananonicalPixelScale where the twips values 
>+ *       came from a Pixel type value. For example, Pixel, EM, or XHeight
>+ *       For font sized difine by Points, inches, millimeters, centimeters etc.
>+ *       we need to multiple them directly by the scale factor
>+ */
> static nscoord
>-ZoomFont(nsIPresContext* aPresContext, nscoord aInSize)
>+ZoomScaleFont(nsIPresContext* aPresContext, 
>+              nscoord         aInSize,
>+              PRBool          aDoZoomScale = PR_TRUE)
> {
>-  nsCOMPtr<nsIDeviceContext> dc;
>-  aPresContext->GetDeviceContext(getter_AddRefs(dc));
>-  float textZoom;
>-  dc->GetTextZoom(textZoom);
>-  return nscoord(aInSize * textZoom);
>+
>+  PRBool isPaginated = PR_FALSE;
>+  aPresContext->IsPaginated(&isPaginated);
>+  if (isPaginated) {
>+    float scale;
>+    aPresContext->GetPrintScale(&scale);
>+    if (aDoZoomScale) {
>+      return nscoord(float(aInSize) * scale);
>+    } else {
>+      return nscoord(float(aInSize) / scale);
>+    }
>+  } else {
>+    nsCOMPtr<nsIDeviceContext> dc;
>+    aPresContext->GetDeviceContext(getter_AddRefs(dc));
>+    float textZoom;
>+    dc->GetTextZoom(textZoom);
>+    if (aDoZoomScale) {
>+      return nscoord(aInSize * textZoom);
>+    } else {
>+      return nscoord(aInSize / textZoom);
>+    }
>+  }
> }

I don't see any reason to disable text zoom for printing, other than the fact
that you're adding this code at the same level as text zoom (which seems like
the wrong level) so that it makes text zoom harder to understand.  More on this
below.

I don't like boolean parameters with confusing meanings like this.  I much
prefer the syntax I used in my patch on bug 154751 (not checked in yet). 

>     aPresContext->GetFontScaler(&scaler);
>     float scaleFactor = nsStyleUtil::GetScalingFactor(scaler);
> 
>-    zoom = PR_TRUE;
>+    zoomScale = !isPaginated;
>     if ((NS_STYLE_FONT_SIZE_XXSMALL <= value) && 
>         (value <= NS_STYLE_FONT_SIZE_XXLARGE)) {

Changes **like these** make text zoom (which is a rather complicated operation
since it has to scale all text exactly once, which requires not doing
multiple-scaling on text sizes that are inherited from or relative to an
already scaled number, but which also requires knowing the scaled size at this
level of the system in order to scale line heights) even harder to understand. 
I don't see why you need changes in this area of code at all if you're
implementing a "full zoom" -- that should be able to live entirely towards the
gfx end of the system (perhaps with a few changes to layout/style consumers,
but not of this scale or complexity).

>-  // We want to zoom the cascaded size so that em-based measurements,
>+  // We want to zoomScale the cascaded size so that em-based measurements,
>   // line-heights, etc., work.

No, that comment makes sense for text zoom.  It makes no sense at all for full
zoom.  Furthermore, "zoomScale" isn't an English word that I'm familiar with.

>-  if (zoom)
>-    aFont->mSize = ZoomFont(aPresContext, aFont->mSize);
>+#ifdef DEBUG_rods
>+  printf("Before: %d", aFont->mSize);
>+  PrintCSSValueType("***** %-25s  ", sizeUnit);
>+
>+  if (zoomScale)
>+    aFont->mSize = ZoomScaleFont(aPresContext, aFont->mSize);
>+
>+  printf("  After: %d\n", aFont->mSize);
>+#else
>+  if (zoomScale)
>+    aFont->mSize = ZoomScaleFont(aPresContext, aFont->mSize);
>+#endif

#else is very dangerous -- you could just use two |#ifdef DEBUG_rods| regions
with the non-debug code between them, if you even feel this needs to be checked
in at all.

>@@ -2331,10 +2464,10 @@
>            SETCOORD_LH | SETCOORD_NORMAL, aContext, mPresContext, inherited);
> 
>   // line-height: normal, number, length, percent, inherit
>+  const nsStyleFont* font = NS_STATIC_CAST(const nsStyleFont*,
>+                                  aContext->GetStyleData(eStyleStruct_Font));
>   if (eCSSUnit_Percent == textData.mLineHeight.GetUnit()) {
>     aInherited = PR_TRUE;
>-    const nsStyleFont* font = NS_STATIC_CAST(const nsStyleFont*,
>-                                    aContext->GetStyleData(eStyleStruct_Font));
>     text->mLineHeight.SetCoordValue((nscoord)((float)(font->mSize) *
>                                      textData.mLineHeight.GetPercentValue()));
>   } else {

Any reason you chose to potentially slow this code down a bit?

Comment 36

[rods (gone)]

There are really three things that keep getting talked about with this code:
1) Text Zoom (requires reflow)
2) Scaling (require reflow)
3) Zooming/Magnification (gfx only no reflow needed)

#3 is completely unrelated to any of these changes or this problem.

Comments running past 80 chars, you have to be kidding me? At least there are
comments, so little code is checked in with detailed comments.

Currently Tex Zoom is turned off for printing, I have never once seen a
requirement for TextZoom to work printing. It makes more sense to have a full on
scale for printing were everything gets scaled. At the moment, print scaling and
Text Zooming do not behave well with each other when printing (or PP) and since
there is no requirement for those to work together at this time, it is easier
and saner to turn off TextZooming and just obey scaling.

I think it would be very confusing for a user to have TextZooming effect how a
"scaled" document is Print Previewed or printed out. I don't think they want to
be adjusting two things at once.

You wrote: "I don't see why you need changes in this area of code at all if
you're implementing a "full zoom" I AM NOT IMPLEMENTING FULL ZOOM! I am not sure
how many times I need to explain this, I am implementing scaling.

You must still not understand the problem that is being solved here. This
checkin has absolutely NOTHING to do with Full Zoom (magnification), this
checkin deals with how font sizes get scaled for printing and in PP. It is
complicated by the fact that Pixel-based fonts are already scaled because the
CanonicalPixelScale and Point-based sized fonts are not scaled. So I have to
figure out what to scale and what not to scale.

What is up with this comment:
"Furthermore, "zoomScale" isn't an English word that I'm familiar with."

Are you implying that every variable and function name must be found in the
English dictionary? I can change it to "isZoomOrScaleOn"

This comment:
"Any reason you chose to potentially slow this code down a bit?"

This is not a helpful comment. It is a comment that is pure FUD as to block this
patch from being checked in. Where am I slowing everything down? Have you done
timing tests, profiled it in order to make such a comment


If you don't understand the problem being solved just say so, I can explain it
in more detail. But nit picking the patch that works and fixes issues with Text
Zooming in Galley mode and fixes several problems with Scaling for Printing and
Print Preview is just not right.

The patch is well documented even if it does exceed 80 chars, it is clean, well
thought out, it solves ONLY the problem with scaling fonts.

I can't see where it adds an undue amount of complication or where it would even
make the code noticably slower. It does check to see if it paginated but that
check isn't a high cost operation.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> But nit picking the patch that works and fixes issues with Text
> Zooming in Galley mode and fixes several problems with Scaling for Printing and
> Print Preview is just not right.

I already tried general comments earlier, but you weren't interested in them,
and kept repeatedly asking for review of a detailed patch that I said was wrong,
so I gave it.

What issues does it fix with text zooming in galley mode?

How is scaling different from full zoom?  What does it not zoom?

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Note that what I refer to as "full zoom" does require reflow -- you do magnify
everything at a gfx level, but the size of the page / browser is the same, so
you have to reflow.)

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Let me summarize the issue as I understand it:

What is being implemented here is what you call scaling, but what I call full
zoom.  It is a type of zoom that scales everything, but that requires reflow
since the size of the page and/or window is not scaled.  This patch is an
attempt to compensate for bugs that I described in comment 9 by doing reverse
and/or missed corrections at a level of the code that should not know anything
about a type of zoom that scales all objects, uncessarily complicating (i.e.,
making unmaintainable) the code that does a different type of zooming (text zoom).

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> What is up with this comment:
> "Furthermore, "zoomScale" isn't an English word that I'm familiar with."

No, I'm implying that the comment that you were modifying was describing the
issue in complete English sentences.  The variable name |zoomScale| represents
something that would be described in English as, perhaps, "zoom or scale", so
the comment should reflect that.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> This comment:
> "Any reason you chose to potentially slow this code down a bit?"
> 
> This is not a helpful comment. It is a comment that is pure FUD as to block this
> patch from being checked in. Where am I slowing everything down? Have you done
> timing tests, profiled it in order to make such a comment

You're clearly moving a function call that's only needed in some cases so that
it's made in more cases than it's needed.  That does, unquestionably, slow
things down, although probably not measurably.  There's also no clear reason why
you made the change.

Comment 42

[rods (gone)]

I apologize for my last comment in the bug. Please ignore it, I will have an
updated patch coming soon.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is seriously messed up. We should not be special-casing print contexts in
the style system.

We need to fix our units system along the lines dbaron suggested. Apart from
anything else I've been thinking about what we have for an hour and I still
don't understand it, and I'm not feeling particularly stupid today.

I'd vote for a system where we keep everything in coordinates which are 1/256 of
the "CSS pixels" for a device, and where the device context tells us the number
of CSS pixels it gets per one physical inch. Isn't that all we need? GFX/widget
can take care of mapping those coordinates to whatever the device or toolkit
coordinate system is.

If we had that then we wouldn't need all this special case code, right? The
style system would just look up the CSSpixels/inch value to convert every fixed
length and font size. GFX would be responsible for doing print scaling.

Comment 44

[rods (gone)]

Robert,

I agree with you that this patch is not optimal and doesn't fix the real
problem. The fix for the real issue is Bug 167162. To fix the real problem we
will end up touching gfx (especially nsFont) and all of layout and the
ultimately it will be rather time consuming and risky and that is after we can
agree on an approach.

The reality is that "Shrink To Fit" is turned on as the default, and every page
with a CSS defined Point sized font will not scale correctly, look bad and get
cropped (see original testcase).

The code that is there is already wrong. This patch doesn't violate anything
that hasn't already been violated, in fact, it actually documents some of the
issues that are causing the problems.

I consider this to be a temporary patch until the real issue can be fix (Bug
167162). Again I request that this patch be allowed in so pages don't get cropped. 

I really don't want to see the next review of Mozilla say something like "Shrink
To Fit works great except that it doesn't shrink all the text, some still gets
cropped." (A final patch with dbaron's suggestions will be attached soon)

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can accept this temporary fix if there's a real commitment from you and other
interested parties to work toward a real fix. A real fix should carefully
define, and simplify, the systems of units we use, and the way we do scaling,
across the entire system, as dbaron has talked about above.

Bug 167162 may be a step in the right direction but my gut feeling is that the
presence of GetCanonicalPixelScale/GetDeviceRatio means we're doing something
fundamentally wrong.

Comment 46

[rods (gone)]

Attachment: patch with dbaron's suggestion

Comment 47

[rods (gone)]

Comment on attachment 100403 [details] [diff] [review]
patch with dbaron's suggestion

bringing don's r= forward

Comment 48

[Kevin McCluskey (gone)]

Can we can get a sr on the patch for this bug soon?. We need to get some testing
on this before Mozilla1.2 goes out. I can set up a teleconference so we can
start discussing a long term solution. 

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 100403 [details] [diff] [review]
patch with dbaron's suggestion

If I have the power to say no to this approach, then I've said no, and asking
for another review of the same or a similar patch won't change that answer. 
See comment 39 for a summary of my objections.	See also comment 20, comment 9,
and comment 5.

Comment 50

[rcummins]

The target milestone for this bug was recently changed to Mozilla 1.2 final,
which doesn't seem realistic.  As far as I can tell from the comments, this bug
should be marked "WONTFIX", or should be marked as a duplicate of bug 167162.

Comment 51

[rods (gone)]

Because the fix for 167162 will probably be quite involved I am still hoping to
get this in as interim fix.

Comment 52

[rcummins]

I'm afraid I don't understand comment #49: 
"(From update of attachment 100403 [details] [diff] [review]) ...I've said no [to this approach]"

Which lines in the patch are the approach that dbaron has said no to?  He can't
be referring to this entire bug, as he has already added 14 comments with
constructive critism about this bug in general and [e-mail]'s patches
in particular, most of which [e-mail] has addressed.  That this is a
temporary fix is clear; work is being done on bug 167162, with a rough patch
submitted and comments being solicited.  Shouldn't a working albeit temporary
patch get sr now, since it is not clear how long it will take to bang a patch
for the larger bug 167162 into shape?

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> Shouldn't a working albeit temporary
> patch get sr now, since it is not clear how long it will take to bang a patch
> for the larger bug 167162 into shape?

That's precisely the problem.  The code will become permanent and make fixing
other bugs a lot harder, and nobody will have an incentive to fix this one
correctly.

Comment 54

[Brendan Eich [:brendan]]

It's 1.3alpha now.  Please convene the right people, come up with a plan in the
1.3 cycle to fix the underlying bug(s) correctly.  Do what you need to do, take
"risks" (all changes have risks; risk is p * cost; the risk of patching around
underlying bugs is already exacting a high cost in Gecko, because the
probability that the temporary patching will be permanent gunk complicating the
wrong layeres of code is nearly 1), don't perpetuate the underlying problems.

/be

Comment 55

[rods (gone)]

Here are our options:

1) Check in the patch, it doesn't make the code significantly less maintainable
or more complicated. In fact, as I have said before, it actually documents (in
several) areas the hidden issues that already exist today.

2) Not check this patch until either david fixes it or I can get to it. The
problem with "doing it right", is that the correct fix will touch a lot of
areas: gfx (fonts, cannoical pixel scale, etc) layout (several frame classes),
content (the printing code). It will be a significant change that will require a
lot of thought, design, investigation, etc. The correct fix isn't just risky, it
has a high level of risk associated with it, and because of the breadth of the
changes the impact to the various areas and the amount of risk (which implies a
lot and lot of testing). This bug will require a signicant amount of time. Time
which I certainly do not have in the 1.3 milestone and possibly even 1.4.

So, I am all for fixing things right given the time and the resources, neither
of which we have right now. Since I don't feel this fix overly complicates the
code or makes it less maintainable I think the fix should go. That or it stays
broken for at least one or two more milestones (unless David signs up to fix it)

This is a very visible bug, fonts defined in point sizes do not scale. I think
it is a shame it wasn't fixed 1.2.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should spend whatever time and resources we have on fixing "deep"
bugs like bug 177805 rather than symptoms, like here. Otherwise we won't make
real progress.

So far, my proposal in bug 177805 seems to be withstanding scrutiny. The next
step is to figure out how to get there from here and how to break up the work
into manageable pieces.

Comment 57

[chris hofmann]

here is a proposal.  since we expect distribution of 1.2 to be low let's hold
off on this implementation in favor of doing work on bug 177805 and addressing
the problem there.   We do expect higher distribuition off of 1.3 or 1.4 so if
the development work for 177805 is looking to land in that time frame many could
benefit by the the fix in that milestone.   If 177805 is not done by then we
could reconsider adding the patch here to the 1.3 or 1.4 branch as a temporary
fix that gets users the benefits of this fix.   No need for more discussion here
or any more decision making on this bug.   We can revisit the status of work on
177805 in a month or two and make decisions then...

Comment 58

[Kenneth Herron]

*** Bug 161080 has been marked as a duplicate of this bug. ***

Comment 59

[Kevin McCluskey (gone)]

-> jkeiser

Comment 60

[Frankie]

*** Bug 166388 has been marked as a duplicate of this bug. ***

Comment 61

[Roland Mainz]

Four months later... no progress. Printing major pages is still defunct due this
bug.

I'll vote to get the current patch "in" - just preventing people from getting
usefull printouts because we wait for stuff which is not going to be implemented
in the near future is just silly.

Comment 62

[Paul Wyskoczka]

ADT: Nominating topembed

Comment 63

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030507]

Got this bug today:
[ <link href="impression.txt" rel="stylesheet"> ]
with
[ font-size : 16pt; ] (different sizes are used)
inside.

The page scaled horizontally, but not vertically: allways got 2 pages instead of 1.

I had to use I.E. (v6.0sp1) :-(

I read all the comments;
I support the idea of an temporary fix: this bug has been opened for almost a
year, with a patch standing by for 9 months.

Comment 64

[Serge Gautherie (:sgautherie)]

Addition to comment 63:

Afterward, I tried to print the page on paper:
got 2 pages, exactly as expected from Print Preview display :-|

In this case,
Mozilla is consistent :-)
but verified unusable to print that page :-(

NB: By the way, the page I am talking of is a 1 page (otherwise quite simple)
electronic (HTML) document generated by an Intranet application developped by my
company for its employees :-((
{I could attach it to this bug if needed.}

Comment 65

[Michael Buckland]

jkeiser, has further dev work been doen on this? If not can we get it into 1.4.  

Comment 66

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4b) Gecko/20030507]

Addition to comment 63:

Same bug with v1.4b/W95.

*****

Quoting <http://www.mozilla.org/roadmap.html>,
[
Fix crucial Gecko layout architecture bugs, paving the way for a more
maintainable, performant, and extensible future.
]

I believe that:
the patch should go in for v1.4, _which will replace v1.0_;
and the underlying issue planned for v1.5/v1.6/etc cycle.

Comment 67

[Samir Gehani]

Renominating for adt consideration.  Is this an adt1?

Comment 68

[Asa Dotzler [:asa]]

never worked, probably requires some re-architecting in layout. not gonna block
1.4 for this.

Comment 69

[Samir Gehani]

adt: nsbeta1-

Comment 70

[John Keiser (jkeiser)]

This is just highly unlikely to happen for 1.4.

Comment 71

[Matthias Versen [:Matti]]

*** Bug 210411 has been marked as a duplicate of this bug. ***

Comment 72

[R.K.Aa.]

are these duplicates? bug 207083 bug 208000 bug 212303 bug 186918 bug 193680

Comment 73

[David Huynh]

For what it's worth... As a user, I'd think of these various zooming
functionalities as follows:

1. Text size adjustment: The font size specified by the webpage's designer is
adjusted to suit the user's needs. Note that nothing is zoomed in the same sense
as a camera's zoom lens zooming into a subject.

---layout happens after text size adjustment---

2. Content scaling: The visual content, whose various elements are already laid
out with respect to one another, is scaled altogether as a whole before it gets
rendered onto a stroke-level canvas. This canvas represents the medium where the
final rendering will take place, e.g., paper. But this canvas can be previewed
onscreen first. In the Print Preview feature, the user sees this canvas as it
will be rendered.

3. Magnification/zooming: This is the real zooming feature where the user can
zoom in and out of the canvas as it is projected onto the screen.

The attachment "print preview.pdf" might illustrate what I meant.

Note that the ability to adjust the text wrapping margin (not only in the print
preview mode) is especially useful in a tabbed browser in which several tabs
might have different appropriate wrapping margins. I hate those pages that let
their text run across the width of my 18" monitor... so hard to keep track of
the lines.

Thanks.

David

Comment 74

[David Huynh]

Attachment: Ideas for consolidating various "zooming" functionalities

Comment 75

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 205001 has a nice simple patch that fixes this bug without adding huge amounts of additional complexity.  Apparently somebody did work out which numbers in our unit system represent the third dimension -- although it could certainly be clearer.

*** This bug has been marked as a duplicate of 205001 ***

Comment 76

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, it didn't actually work.  So I think the fix for this should look a lot like a combination of comment 5 and the patch in bug 205001.

Comment 77

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

David Baron or Somebody:

What's status in this bug?
If nobody works on this, I want to work on this.
# Or should we wait bug 177805? But that is not active in this 9 months...

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 177805 is waiting until we can remove support for non-cairo graphics. I recommend waiting until then before working on this. If you run out of other things to do, I can make some suggestions :-)

Comment 79

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for your reply. I wait bug 177805, because I see by your reply why it is stopped. thanks.

Comment 80

[Ulrich Windl]

I'm unsure what people need: The bug was opened 2002. It's 2006 now, and "fit to page" doesn't work in Firefox 1.5.0.1, Mozilla 1.7.12, nor Microsoft IE 6. All in Windows/XP. When printing the rightmost part of a table is simply cut off. Would an example file help?

Comment 81

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Please read the bug before commenting.

Comment 82

[Ulrich Windl]

(In reply to comment #81)
> Please read the bug before commenting.

Actually I did (read the comments), and I was wondering why it couldn't be fixed within 4 years. (Konqueror of KDE can do the scaling correctly). The gecko bug is just as old. Do you need another test case? I guess not.

Comment 83

[Serge Gautherie (:sgautherie)]

Bug 177805 (Fix the use of units in Gecko) was fixed by "2007-02-06 23:46	Eli Friedman".

What would be the next step for this print/scale bug ?

Comment 84

[Gábor Stefanik]

Maybe we should set the "helpwanted" keyword. This bug seems to be abandoned, even though we need it for full page zoom.

Comment 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is this not already fixed on trunk by the changes in bug 177805 (units patch)? If not, it should be easy to fix now.

Comment 86

[Eli Friedman]

Already fixed, as far as I can tell.