271470 - print dialog doesn't have print frames options in 11 localizations

Status: RESOLVED DUPLICATE of bug 271802

(Firefox Build System :: General, defect, P1)

Description

[Ville Pohjanheimo (inactive)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fi-FI; rv:1.7.5) Gecko/20041119 Firefox/1.0 (Debian package 1.0-3)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fi-FI; rv:1.7.5) Gecko/20041119 Firefox/1.0 (Debian package 1.0-3)

Finnish localized Firefox 1.0 on Windows uses Windows' native print dialog,
which has the effect of not allowing the user to print with the same layout as
on screen.

Instead the option is not present and the user thinks everything is ok, hits
Print and FF chews out one page for each frame. And then user gets annoyed.

Conversation of the bug in MozillaZine:
http://forums.mozillazine.org/viewtopic.php?t=116735&start=0&postdays=0&postorder=asc&highlight=print+frames

which includes a link to this picture comparing Finnish FF 1.0 print dialog
(down) and en-US FF 1.0 print dialog (up):
http://koti.mbnet.fi/digitale/ff_print_dialog.gif

The OS in the picture is Finnish XP Pro. The same behaviour has been tested with
English XP Pro. This bug is locale-specific as this behaviour does not occur
with the Swedish sv-SE build). Only the installer version has been tested.


Reproducible: Always
Steps to Reproduce:
1. Open a page with frames with Finnish Firefox 1.0 (on Windows)
2. Select File - Print
3. Observe windows native print dialog
Actual Results:  
Inability to select "as laid out on the screen" and thus FF prints every frame
on a separate sheet of paper.

Expected Results:  
FF should use its own print dialog which has the correct options.

Comment 1

[Steffen Wilberg]

Confirming.

http://lxr.mozilla.org/aviarybranch/source/toolkit/locales/en-US/chrome/global/printdialog.properties

says:
 20 #-----------------------------------------------------------
 21 # Note: IMPORTANT!
 22 #
 23 # Set "extend" to false to get the native platform dialog
 24 #
 25 # Set "extend" to true to have the native dialog extended with
 26 #   the "Print Frames" Frame group box
 27 #-----------------------------------------------------------
 28 #
 29 extend=true

That should be "true" everywhere.

These have "extend=true":
ast-ES, de-DE, el-GR, en-GB, fr-FR, he-IL, hu-UL, it-IT, ja-JP, ja-JPM, nb-NO,
nl-NL, pl-PL, pt-BR, ro-RO, ru-RU, sl-SI, sv-SE, tr-TR, zh.TW

However, these are wrong:
ca-AD: extend=cert
cs-CZ: extend=ano
da-DK: extend=sand
es-AR: extend=verdadero
es-ES: extend=verdadero
fi-FI: extend=tosi
ga-IE: extend=f\u00EDor
ko-KR: extend=\uC18D\uC131
sk-SK: extend=\u00E1no
sq-AL: extend=e v\u00EBrtet\u00EB
zh-CN: extend=\u771F

I tested the fi-FI build. Setting extend to true in
fi-FI.jar/locale/global/printdialog.properties fixes this problem.

Comment 2

[Pavel Franc - Mozilla.cz]

CCing other localizers of affected localization.

Comment 3

[Mike Connor [:mconnor]]

whee.  Steffen, care to attach a patch for the en-US original that adds a "don't
translate this" string so localizers don't make this mistake again?

Also if there's anything else that needs to stay an English string, we NEED to
ensure that its clear to localizers to not break it.

Comment 4

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

Why should this be in a language pack at all?  It seems like it either belongs
in a pref or it should be hardcoded.

Comment 5

[Steffen Wilberg]

This is the code that references "extend" in printdialog.properties:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp&mark=914-925#914
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp&mark=1261-1272#1261
That's from bug 135441. Attachment 82120 [details] contains some documentation.

The comment seems to be wrong. True extends the dialog by the Print Frames
options. False does not.

We might hardcode PRPackedBool doExtend = PR_TRUE;
I'm not building on Windows though.

I guess it's safer to fix the printdialog.properties files.

Comment 6

[Mike Connor [:mconnor]]

Safer for branch yes, the right fix is replacing with a pref, as dbaron said.

Comment 7

[Ville Pohjanheimo (inactive)]

Ok. The bug's no properly identified etc. Should (we /) the localizers now fix
the problem by CVS committing the correct printdialog.properties file? (AFAIK
the tree is still closed)

If any localizer has a smart fix ready for the people that've already downloaded
one of the problem FF 1.0s, I'm sure we would all be grateful if that person
would share the fix.

Comment 8

[Mike Connor [:mconnor]]

For all of the localizers: 

The fix is to not translation the extend=true line.  It appears that all of the
localizers where this is affected didn't understand the comment, and translated
"true" to the localized term.  Unfortunately, the guy who wrote the printing
code sucks for having used the .properties file for this.

Comment 9

[Kadir Topal [:atopal]]

Please blame MozillaTranslator, not the localizers for this problem. MT doesn't
show any comments. If it would, I'm sure not a single localizer would've
translated that string.

Comment 10

[Søren Munk Skrøder (extended hiatus)]

In reply to Mike Connor's comment #8 :

One should notice the fact, that people that have been using Mozilla Translator
to translate this file, never have been presented with this comment about
leaving "extend" to "true" 

In da-DK, i know that that was the problem -- We were just presented with a key
called "extend" - that had the english valdue "true" - and with MT, we
translated it to danish ("sand")

So it (in some cases) it's not an issue of "understanding" - but more an issue
of even being "presented" - the comment... 

As a side-remark ... yes, it sucks that a value, not meant to be localized
appear in a file meant to be localized --- I, for one won't blame the 11
loalizations for being confused or at least for getting this one wrong....

IMHO the "extend" key should be removed from printdialogs.properties and
hardcoded into the XUL -- unless, of couse, some can come up with a very good
reason why is should be changeable for localizers....

Comment 11

[Mike Connor [:mconnor]]

From CVS/Bugzilla archaeology, I've found out that the extend=true bits were
added as a fallback seemingly so if localized bits weren't in place, the dialog
wouldn't be extended and we'd just use the straight native dialog.

As a note, I'm aware of a number of other "do not translate" strings that could
cause problems...

Comment 12

[Mike Connor [:mconnor]]

I spun off bug 271802 to address fixing this "better" in future development
trunk. On branch fixing the .properties file would be correct.

Comment 13

[Benjamin Smedberg]

Branch changes should not be made without approval, of course, and I am not
inclined to fix this for those locales which have already been released. Is
there a good reason why this can't wait until Firefox 1.1?

Comment 14

[Ville Pohjanheimo (inactive)]

(In reply to comment #13)
> Is there a good reason why this can't wait until Firefox 1.1?

This all depends on how much Mozilla Foundation cares about the user experience
of present and future Mozilla Firefox users in China, Spain, Denmark and Czech
Republic among others. Most people print documents from the net and a large
portion of pages use frames.

Is making a patch for this difficult? Patches have been done earlier eg. the
german eBay plugin.

Comment 15

[Steffen Wilberg]

Correcting the source is a one-line fix: extend=true, see comment 1.

Providing a patch for end-users is easy as well, but it's not as small as the
eBay.de-patch: It would be a new language pack, containing the fixed
printdialog.properties, from
http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/1.0/win32/xpi/. These
language packs are being created automatically by the build process.

Comment 16

[Henri Sivonen (:hsivonen)]

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

Comment 17

[Axel Hecht]

Following up on #5,
My code investigation goes down to two places, triggered by "true". Or anything
that is true when compared to "true" case insensitivly.

InitializeExtendedDialog is the one function that reads on "true", the other is
ShowNativePrintDialogEx. Both together expose all the strings of 
http://lxr.mozilla.org/aviarybranch/source/toolkit/locales/en-US/chrome/global/printdialog.properties
which are otherwise hidden.

Any patch that would switch extend from something non-true to "true" requires
some serious QA on the print dialog and all the strings in 
printdialog.properties on a windows machine for each of the affected builds.

Comment 18

[Ville Pohjanheimo (inactive)]

(In reply to comment #17)
> 
> Any patch that would switch extend from something non-true to "true" requires
> some serious QA on the print dialog and all the strings in 
> printdialog.properties on a windows machine for each of the affected builds.

Please define serious QA, can L10n teams help with this and how?

Comment 19

[Axel Hecht]

(In reply to comment #18)
> Please define serious QA, can L10n teams help with this and how?

It seems that this extended dialog offers different options on different 
settings, like, if you have a selection or not etc.

http://testrunner.mozilla.org/test.cgi?run_id=778#858 is not really verbose on
the options for printing.

Asa, is there a set of tests that you would like to see done?

To all, we're not yet approving or disapproving a patch here, just evaluating
what we would like to see done to decide that.

Comment 20

[Yrjö Kari-Koskinen]

Is there going to be a fix for this bug available soon?

As I understand it from comment #15, you would only need to change one line in
few language packs, and installing the new language pack would fix the problem,
right?

And why, oh why, was the fix not added to the FF 1.0.1 release?

Comment 21

[Asa Dotzler [:asa]]

Yrjö Kari-Koskinen, Firefox 1.0.1 was a security update, not a general release.
We didn't make any core changes that would break localizations and we didn't
take any changes to 1.0 localizations. 

Comment 22

[Oscar Manuel Gómez Senovilla]

es-ES toolkit was fixed long ago, in both firefox and in thunderbird, but MoFo
didn't accept patches. I think the fix should be done by MoFo internally.

Comment 23

[Benjamin Smedberg]

This should not be localizable, and I intend to make it so.

Comment 24

[Benjamin Smedberg]

Attachment: Use a pref, not a localized setting, rev. 1

Comment 25

[Steffen Wilberg]

The comments are wrong. "true" means that the dialog is extended.
http://lxr.mozilla.org/mozilla/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#925
http://lxr.mozilla.org/mozilla/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#1272

Comment 26

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

I filed a separate bug on making it not localizable and posted an untested patch
there...

Comment 27

[Benjamin Smedberg]

Steffen: I don't understand... do you mean the comments that I'm removing, or
the new comment in all.js, or something else?

Comment 28

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

Or, rather, mconnor spun off a separate bug, bug 271802, as noted in comment 12,
and I posted a patch there.  My patch consolidates things into one function,
also fixes the embedding code, and doesn't rely on the undocumented and unfrozen
(as far as I can tell) implementation detail that the pref service is also the
root branch (or should we be relying on that?).

Comment 29

[Steffen Wilberg]

Sorry for the bad explanation. The new comment in all.js is correct, as well as
the to-be-removed comment in printdialog.properties. But there are two wrong
comments here:
http://lxr.mozilla.org/mozilla/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#925
http://lxr.mozilla.org/mozilla/source/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp#1272

These are not part of the patch, but I'd suggest to fix them while you're at it.

Comment 30

[Benjamin Smedberg]

That the pref service implements the root prefbranch is now a frozen feature.
Your patch looks better than mine, can we get it landed?

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

do we need the pref at all?

Comment 32

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

(In reply to comment #30)
> That the pref service implements the root prefbranch is now a frozen feature.
> Your patch looks better than mine, can we get it landed?

I don't think anybody's tested it.

Comment 33

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

Comment on attachment 180071 [details] [diff] [review]
Use a pref, not a localized setting, rev. 1

minusing per comment 30 (and this also belongs in bug 271802)

Comment 34

[Benjamin Smedberg]

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

Comment 35

[Jo Hermans]

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

Comment 36

[Jo Hermans]

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