275387 - Print Preview Scale and orientation not adjusted (stuck on page setup settings)

Status: RESOLVED FIXED

(Core :: Print Preview, defect, P1)

Description

[Aaron Pfeifer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

When using Print Preview, the initial Scale value is 100%.  If you try to
specify a custom scale below 10%, the value in the scale box will be changed to
30% (instead of the previous 100%),  but the actual scaled document is never
changed.

Reproducible: Always

Steps to Reproduce:
1. Visit http://www.google.com
2. Select Print Preview from the File menu
3. Drop-down the Scale combobox and select "Custom..."
4. Specify a value below 10% (not including 10%)

Actual Results:  
The value in the Scale combobox is changed to 30%, but the actual scaled
document remains unchanged.

Expected Results:  
Assuming that the correct behavior is using the minimum value, it should either
(1) Reflect the value in the Scale combobox or (2) Use the real minimum value,
which it seems is 10% in my test cases.

Comment 1

[Tracy Walker [:tracy]]

confirmed on Windows Firefox trunk build 2005-05-18-07-trunk

no change in Scale works.  any change reverts back to "Shrink to fit"

Comment 2

[Tracy Walker [:tracy]]

marking this as a dupe of 302495 because it has more info and is already flagged
to block 1.5

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

Comment 3

[Tracy Walker [:tracy]]

Going to the page setup dialog and changing settings works. That's covered and
WFM in bug 302495.  

However, the Scale and Orientation controls on the Print preview page still do
not function at all.  Landscape/Portrait buttons do not change orientation.
Scale menus has no affect. In fact anything selected just reverts to whatever is
set in the Page setup dialog.

reopening, nominating and morphing slightly to generalize the Scale and
Orientation tools failure.

 

Comment 4

[Asa Dotzler [:asa]]

we need to find a better owner for this. any suggestions?

Comment 5

[Adam Guthrie]

This should probably be duped as bug 267422.

Comment 6

[Tracy Walker [:tracy]]

No, that bug occurs anytime a change in print preview settings is attempted,
whether it be from the main tools (broken - this bug) or from the Page setup
dialog (works - bug 302495).  If it tunrs out both bugs get fixed by a single
checkin, great.  But let's not lump them together just yet.  
   

Comment 7

[Ali Chhotani]

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

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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

Comment 9

[Adam Guthrie]

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

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #5)
> This should probably be duped as bug 267422.

Maybe we shouldn't dup, but this bug is spectacularly miscategorized and
misassigned.  Roc, what can be done here?

/be

Comment 11

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

This is Windows only, other platforms don't have the UI. It sounds like a chrome
bug. Get mconnor to check?

Comment 12

[Brendan Eich [:brendan]]

Mike, see comment 11.

/be

Comment 13

[Mike Connor [:mconnor]]

This seems massively broken on Windows, yes...

Taking to investigate.

Comment 14

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

This may be my regression. Please wait.

Comment 15

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

My previous comment is wrong. This is not my regression.

I'm worried, see this code:
http://lxr.mozilla.org/mozilla/source/toolkit/components/printing/content/printPreviewBindings.xml#374
> 374           settings.shrinkToFit = false;
> 375           settings.scaling = aValue;
> 376           PrintUtils.savePrintSettings(settings);
> 377           PrintUtils.printPreview();

If user change the scaling, this code try to save these values. But in
printUtils, savePrintSettings save nothing data.

http://lxr.mozilla.org/mozilla/source/toolkit/components/printing/content/printUtils.js#132

PSSVC.savePrintSettingsToPrefs(aPrintSettings, false,
aPrintSettings.kInitSaveNativeData);

Currently we don't use kInitSaveNativeData. But I think we need to save
kInitSaveShrinkToFit and kInitSaveScaling to current printer prefs in this time
for next print preview.

Comment 16

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

I think that this is regression of bug 270079.
Bug 270079's patch is wrong. It breaks global printer settings.
Currently printer code does not assume that |GetGlobalPrintSettings| returns
latest printer settings.

Comment 17

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

Oops..

> Currently printer code does not assume that |GetGlobalPrintSettings| returns
latest printer settings.

right comment:
Currently printer code assumes that |GetGlobalPrintSettings| returns
latest printer settings.

Comment 18

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

Attachment: backing-out patch

We should back out the patch of bug 270079. I tested this patch, the print
preview's scale list box worked.

But we have another bug.
If changing scale, the content area is shrinked.

Comment 19

[Phil Ringnalda (:philor)]

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

Comment 20

[Jo Hermans]

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

Comment 21

[Kevin Brosnan]

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

Comment 22

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

(In reply to comment #15)
> If user change the scaling, this code try to save these values. But in
> printUtils, savePrintSettings save nothing data.
> 
>
http://lxr.mozilla.org/mozilla/source/toolkit/components/printing/content/printUtils.js#132
> 
> PSSVC.savePrintSettingsToPrefs(aPrintSettings, false,
> aPrintSettings.kInitSaveNativeData);
> 
> Currently we don't use kInitSaveNativeData. But I think we need to save
> kInitSaveShrinkToFit and kInitSaveScaling to current printer prefs in this time
> for next print preview.

This sounds right to me. Did you try it?

Comment 23

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

> This sounds right to me. Did you try it?

Yes, of course. But it is not better than current patch. Because we should not
save the prefs in print preview. The prefs should be written in printing.(I
think the users hope that the prefs should be saved with commiting time. I don't
think so for print preview. We should not save prefs in this time.)

Why you have granted to the patch of bug 270079? I think that we should not
destroy global printer object. Because we are using same instance of global
printer object in some places. Therefore, we have a problem in bug 270079. But
it is wrong way that global object's all field reset for some files of printer
object. Bug 270079 should change only the fields that has problem of global object.

I think bug 270079 made some regressions. Those are bug 308173 and a part of bug
118563(always reset the printing settings by every printing).

Comment 24

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

And see my comment 17.

Comment 25

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

(In reply to comment #23)
> Yes, of course. But it is not better than current patch. Because we should not
> save the prefs in print preview. The prefs should be written in printing.(I
> think the users hope that the prefs should be saved with commiting time. I
> don't think so for print preview. We should not save prefs in this time.)

I don't agree. I think it's fine to save prefs from the settings in print
preview. Can you give me the patch for that?

Comment 26

[Mike Connor [:mconnor]]

We should absolutely save prefs in the print preview window, can we get traction
on that today?  I'll certainly review that ASAP.

Comment 27

[Kevin Brosnan]

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

Comment 28

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

I think that we should not save prefs in PrintPreview.
Because we must not change prefs if "print.save_print_settings" is false.
Currently, if "print.save_print_settings" is false, the global printer object is
always reset by every printing and print preview. This is not good behavior.

I think you are wrong, roc. We should have global printer object that is not
related prefs. But the global printer object is broken by bug 270079.

Comment 29

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

And if bug 270079 is correct, we cannot solve following paradox.

1. We should not save prefs if "print.save_print_settings" is false.
2. We need to change prefs for changing scale or orientation in print preview.

If we don't have global object, we need to save prefs in 2. But if
"print.save_print_settings" is false, we need to ignore
"print.save_print_settings". I.e, "print.save_print_settings" cannot control
saving print setteings.

However, if global object is not broken, we can solve this paradox.

1. -> Don't save if "print.save_print_settings" is false.
2. -> Don't save prefs, but we can change in global object and the global object
is refered in every print preview.

Comment 30

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

(In reply to comment #26)
> We should absolutely save prefs in the print preview window,

We don't save the prefs in Firefox 1.0 in print preview window. Firefox 1.0 is
only changing global object, not saving prefs.

Comment 31

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

print.save_print_settings is true by default and there is no UI for it. If it
causes any problems, we should just remove/ignore the pref. Let's go with the
patch for saving prefs, at least for branch. We can have a longer discussion
about what to do on trunk.

Comment 32

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

O.K. I have a new idea. The new patch will come today or later.

Comment 33

[Scott MacGregor]

Comment on attachment 196706 [details] [diff] [review]
backing-out patch

obsoleting this patch to help improve our release bug queries so we don't think
we have a patch for this.

Time is running short if we are going to get this in. Having a new patch today
would improve the chances of this getting fixed in time.

Comment 34

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

This is pretty serious. I would think hard before releasing with this bug. Some
UI in the print preview window simply doesn't work, as I understand it.

Comment 35

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

Attachment: Patch

I propose this approach.
But I don't have much time for testing this patch.
Please wait 12 - 15 hours.

Comment 36

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

Attachment: Patch

Comment 37

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

Comment on attachment 198140 [details] [diff] [review]
Patch

This patch doesn't work fine on Thunderbird. Because Thunderbird is using only
a part of printUtils. I'll try next.

Comment 38

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

Attachment: quirk patch

This patch fixes regression.

But I trust that this patch is incorrect. Because we should be able to get
latest printer settings by nsPrintOptions::GetGlobalPrintSettings. I cannot
understand why we have nsPrintOptions::GetGlobalPrintSettings and
nsPrintOptions::GetNewPrintSettings. These methods are same.

If roc is correct, we need refactoring for printUtils.js in toolkit.

Comment 39

[Mike Connor [:mconnor]]

Comment on attachment 198179 [details] [diff] [review]
quirk patch

yeah, toolkit's printing impl needs love, let's go with this for now, and
figure out what we need to whack on trunk

Comment 40

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

checked in to trunk

Comment 41

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

checked in to 1.8branch too.

Comment 42

[Andrew Schultz]

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

Comment 43

[tech]

Still present in Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.0.1) Gecko/20060129 SeaMonkey/1.0

REOPEN

Comment 44

[Mike Connor [:mconnor]]

XPFE probably needs a port of this fix.  Please file a new bug against Seamonkey.