581470 - Ctrl+P and Ctrl+W not working from Print Preview window

Status: RESOLVED FIXED

(MailNews Core :: Printing, defect)

Description

[Kuno Meyer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.7) Gecko/20100713 Firefox/3.6.7 ( .NET CLR 3.5.30729; .NET4.0C)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.7) Gecko/20100713 Thunderbird/3.1.1

Pressing Ctrl+P in the main window launches the print dialog. Pressing Ctrl+P in the "Print Preview" window seems to have no effect...

Reproducible: Always

Comment 1

[Thomas D. (:thomas8)]

Attachment: Request UI-review for Ctrl+P to invoke Print dialogue from Print Preview, as in FF4

Confirming: Implement Ctrl+P keyboard shortcut to invoke Print dialogue from Print Preview (currently only possible with Alt+P)

Bryan (ui-review), this should be straightforward as FF4 already has it.

Kuno, thanks for reporting this issue, which is much appreciated as / even though such little things can easily get overlooked among the large number of more urgent issues.

STR

1) From TB main window, File > Print Preview
2) Press Ctrl+P, expecting to invoke the print dialogue

Actual Result

- nothing

Expected Result

- invoke the Print dialogue

Reasons:
1) Ctrl+P to invoke the Print dialogue is expected default behaviour, anywhere
2) Ctrl+P works as expected in Firefox 4 Print Preview
3) No reason to force the user into clumsy Alt+P only

This should be straightforward and "easy" to implement. Any volunteers?

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 523948 [details]
Request UI-review for Ctrl+P to invoke Print dialogue from Print Preview, as in FF4

Sounds totally reasonable.  Blake's call.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 523948 [details]
Request UI-review for Ctrl+P to invoke Print dialogue from Print Preview, as in FF4

Yep, makes sense to me.

As a side note, when I hit Ctrl-P from within the Print Preview window, the console shows me the following message:
JavaScript error: chrome://global/content/printUtils.js, line 287: document.getElementById("printKb") is null

Hopefully that'll help you zero in on a fix a little quicker.  :)

Thanks,
Blake.

Comment 4

[Ludovic Hirlimann [:Usul]]

Mark beginner would like to take a stab at fixing this bug can you point him were to look in the source tree ? 

Beginner, you'll need to have a a build environment see https://developer.mozilla.org/en-US/docs/Simple_Thunderbird_build - once you have that and a patch (see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F) you'll need to request reviews.

Some people might be able to help you in #maildev on irc.mozilla.org.

Comment 5

[Ludovic Hirlimann [:Usul]]

You can ask directly in the bug it's better than sending emails :-)

Comment 6

[sakshi]

I have successfully built Thunderbird. I have also gone through some of the files in
the source code(related to print preview). Could you help me by giving some ideas
on how I could proceed.

Comment 7

[Thomas D (user)]

Hi Sakshi,

nice to meet you here! It's great news that you want to contribute by bug-fixing, most welcome... :)

So as an experienced volunteer myself, I'll happily give you some ideas how to proceed.

To begin with, may I suggest that you change your bmo alias name from "beginner" to "Sakshi" or "Sakshi (beginner)" if you so wish. We appreciate the humbleness of presenting yourself as a beginner, but it feels very odd to address you anonymously (and somewhat patronizingly) as "beginner" instead of just using your first name as is the custom in bmo. Using "beginner" also makes it much harder to correlate your alias to your email address, which will exclusively be used in several places in bmo (for an example, see comment 5), and it can easily lead to ambiguities like in comment 4 ("Mark beginner..."? Who's that?). Lastly, somebody who can build Thunderbird for the first time in a single day imho is no longer a "beginner" ;)

Comment 8

[Thomas D. (:thomas8)]

OT: (In reply to Thomas D (user) from comment #7)
> So as an experienced volunteer myself, I'll happily give you some ideas how
> to proceed.

In an ironic twist (just to check if everyone's paying attention), I've used the wrong account for writing comment #7... So no, as can be seen from my administrative account (bugzilla2007), I'm no longer "New to Bugzilla", that's just because I never use the other account unless for testing purposes ;)

Comment 9

[Thomas D. (:thomas8)]

Sakshi, here's how to find the broken code:

Luckily in this case, there's a hint already:

(In reply to Blake Winton (:bwinton) from comment #3)
> As a side note, when I hit Ctrl-P from within the Print Preview window, the
> *console* shows me the following message:
> JavaScript *error*: chrome://global/content/printUtils.js, line 287:
> document.getElementById("printKb") is null
> Hopefully that'll _help you zero in on a fix_ a little quicker.  :)

1) Turn on traditional menu bar (right-click tab bar, check menu bar); will save you some clicks as we go along
2) Tools > Error Console (or Ctrl+shift+j), -> click [Clear], keep it open
3) view a msg in print preview, hit Ctrl+P
4) view Error Console again, find this error:

> Error: TypeError: document.getElementById(...) is null
> Source File: chrome://global/content/printUtils.js
> Line: 291

In the error message, if you click on _chrome://global/content/printUtils.js_, it will helpfully open another window showing the source of printUtils.js with erratic line 291 highlighted:

> var printKey =
> document.getElementById("printKb").getAttribute("key").toUpperCase();

So according to the error msg,
> document.getElementById("printKb") is null

So that's where the problem starts. Looks like we no longer have an element with ID="printKb", or it can't be found from print preview, so it's a regression.

Comment 10

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #9)
> Sakshi, here's how to find the broken code:
[snip]
> So according to the error msg,
> > document.getElementById("printKb") is null

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/printing/content/printUtils.js#291

> So that's where the problem starts. Looks like we no longer have an element
> with ID="printKb", or it can't be found from print preview, so it's a
> regression.

One way of finding related code (i.e. the needle in the haystack) is searching Mozilla Cross-Reference (mxr) for the string "printKb" using "Search for" field:

http://mxr.mozilla.org/comm-central/
http://mxr.mozilla.org/comm-central/search?string=printKb

There are only two places which have an element with ID="printKb":

> /mozilla/browser/base/content/browser-sets.inc
>     line 275 -- <key id="printKb" key="&printCmd.commandkey;"
>                                   command="cmd_print" modifiers="accel"/>
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser-sets.inc#275

> /mozilla/toolkit/components/help/content/help.xul
>    line 83 -- <key id="printKb" key="&printCmd.commandkey;"
>                                 oncommand="print();"
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/help/content/help.xul#83

So with a view on these two we have to find out why
> document.getElementById("printKb") is null

Comment 11

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #10)
> So with a view on these two we have to find out why
> > document.getElementById("printKb") is null

After finding & fixing the bug in your code, what we ultimately need from you is a text file containing the patch ("Add an attachment" at the top of this bug), which should look like attachment 704303 [details] [diff] [review].

Comment 12

[sakshi]

Hello,

Thanks a lot for helping me to get started. I did as you told. I compared the files you mentioned above(printKB) with the files of firefox(since Ctrl+P in print preview) works in firefox. But I found it pretty much the same so I did not really understand why printKb is null.

Comment 13

[sakshi]

ya

Comment 14

[sakshi]

Hello, 
Sorry for the trouble and thanks a lot

Comment 15

[Thomas D. (:thomas8)]

(In reply to sakshi from comment #12)
> Hello,
> 
> Thanks a lot for helping me to get started. I did as you told. I compared
> the files you mentioned above(printKB) with the files of firefox(since
> Ctrl+P in print preview) works in firefox. But I found it pretty much the
> same so I did not really understand why printKb is null.

Indeed. FF has exactly the same constellation of files and their use of printKb:
http://mxr.mozilla.org/mozilla-central/search?string=printKb

(In reply to Thomas D. from comment #10)
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/
> printing/content/printUtils.js#291
> 
> > So that's where the problem starts. Looks like we no longer have an element
> > with ID="printKb", or it can't be found from print preview [...]
> There are only two places which have an element with ID="printKb":
> 
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser-sets.inc#275
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/help/content/help.xul#83

Judging from the references in comm-central, I think help.xul is unused in main TB (if not obsolete):
http://mxr.mozilla.org/comm-central/search?string=help.xul
So the keybinding with ID="printKb" for print will *not* be available from there. Good.

I'm not sure how to find out if browser-sets.inc is used and/or "available" in the context of print preview:
http://mxr.mozilla.org/comm-central/search?string=browser-sets.inc

I suspect that even if it's used in TB (via inclusion from browser.xul), it would not be reachable (out of scope) from print preview (e.g. for security reasons). So according to this hypothesis, the print keybinding with ID="printKb" is not available from here, either.

So we probably need a new way of re-enabling the print keybinding.
I'm not sure how to proceed from here. Suppose we could just snatch the required key from somewhere else where it's available, perhaps from here:

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#426

> <key id="key_print" key="&printCmd.key;"
>                     oncommand="goDoCommand('cmd_print')" modifiers="accel"/>

So as a random guess, perhaps we could just swap the ID and change our broken line printUtils.js#291 to:

var printKey = document.getElementById("key_print").getAttribute("key").toUpperCase();

Sakshi, can you try that in your build?

(However, even if that works, I have no idea what else we might be breaking.)

> So with a view on these two we have to find out why
> > document.getElementById("printKb") is null

Ludovic, any idea how to find out more, or whom to invite to this bug?

N.B. The onKeyPressPP: function in printUtils.js looks hackish... But perhaps it's alright, or we can get away with that.

Comment 16

[Thomas D. (:thomas8)]

As a sidenote, interestingly, Print Preview command is *disabled* for browser windows within TB (highlight a random word in a message body, right-click, "Search bing for: yourword"), so from those, you can only use Ctrl+P directly to just print.

Comment 17

[sakshi]

Hello, 

I changed the line in printUtils.js#291 to
var printKey = document.getElementById("key_print").getAttribute("key").toUpperCase();

Then i built it again. 

However in TB in print-preview on hitting Ctrl+P, int the error console it shows me the same error:

 document.getElementById(...) is null
> Source File: chrome://global/content/printUtils.js
> Line: 291

On clicking the error message i go to the file with line #291 high lightened:
 var printKey = document.getElementById("printKb").getAttribute("key").toUpperCase();

printKb remains unchanged.

Comment 18

[Thomas D. (:thomas8)]

(In reply to sakshi from comment #17)
> I changed the line in printUtils.js#291 to
> var printKey =
> document.getElementById("key_print").getAttribute("key").toUpperCase();
> 
> Then i built it again. [...]
> On clicking the error message i go to the file with line #291 high lightened:
>  var printKey =
> document.getElementById("printKb").getAttribute("key").toUpperCase();
> 
> printKb remains unchanged.

I think you made a mistake somewhere. Following the source link from the console error message should not show the old code after you have changed that code.
But I am having a similar problem, and it's very confusing.

I don't have a build environment, and for some of the more simple changes it's not needed. So for those small javascript or XUL changes, I'll just edit the source files manually. This is what I did:

(on WinXP, TB17)
0) Exit Thunderbird (make sure no windows or hidden instances are left)
1) explore c:\programs\Mozilla Thunderbird\
2) backup omni.ja as omni.orig.ja in same folder
3) rename omni.ja to omni.zip, extract all into subdir named omni
4) explore omni\chrome\toolkit\content\global (as extracted from zip)
5) edit printUtils.js with NotePad++ text editor
6) replace line 291 of the code for testing purposes with this line:
alert('hello world! Modifier key pressed');
7) explore omni folder; zip folder content into omni.zip; rename to omni.ja
8) revisit step 0)
9) replace c:\programs\Mozilla Thunderbird\omni.ja with your own modified version of omni.ja
10) verify last-changed datestamp of omni.ja to ensure it's your version

11) Start TB
12) Clear Error Console
13) Print Preview a msg
14) press Ctrl+P (or Ctrl+a where a is any alphanumeric character)
15) check Error Console

Actual result:

Error: TypeError: document.getElementById(...) is null
Source File: _chrome://global/content/printUtils.js_
Line: 291

When you click on the source link of that error, you'll see the following file...

jar:file:///C:/programs/Mozilla%20Thunderbird/omni.ja!/chrome/toolkit/content/global/printUtils.js

...with the following line highlighted:

291 alert('hello world! Modifier key pressed')

I'm failing to understand how the error message can still claim that for line 291, "document.getElementById(...) is null" while that code does not even exist in the currently used source for that line.

I conclude that in spite of modified source file (printUtils.js in omni.ja), which is even correctly linkified from console error, TB is using another (stale, cached?) copy of that file internally.

Expected result:

After pressing Ctrl+P, TB should show alert('Hello world...'), as defined in my modified version of printutils.js (inside omni.ja), line 291.
TB should not show error messages involving stale code which does not currently exist in omni.ja.

If I understand this correctly, omni.ja is a "live" source file which TB will load/parse each time TB starts. If I make changes to that file while TB is closed, those changes should be effective at runtime when I restart TB.

Can someone please enlighten me if I am wrong somewhere or otherwise what's happening here?

Comment 19

[Thomas D. (:thomas8)]

Btw, Ctrl+W to close the print preview window does not work for me either (using unmodified TB), so neighbouring code is also broken. Sakshi, can you confirm this?
Which OS are you using?

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/printing/content/printUtils.js#284

Comment 20

[Thomas D. (:thomas8)]

restoring needinfo flag for comment 18

OT: omg, automatic clearance of needinfo-anyone with the next comment really sucks!
Users providing that info should have to check the "I am providing the requested info..." checkbox, ux-consistent with what we do for the needinfo-user flag.

Comment 21

[sakshi]

Hello, 
I am using ubuntu 11.10 kernel version 3.0.0-19-generic. Is it mandatory to use Windows?

Comment 22

[Ludovic Hirlimann [:Usul]]

No

Comment 23

[sakshi]

Okay, but I cannot understand why the change is not being reflected in the error console, inspite of changing in the TB source code

Comment 24

[sakshi]

Hi,

ctrl+W to close the print preview does not work for me also.

Comment 25

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #18)
> I conclude that in spite of modified source file (printUtils.js in omni.ja),
> which is even correctly linkified from console error, TB is using another
> (stale, cached?) copy of that file internally.

True. I found that starting TB in *Safe Mode* will clear the script cache and allow hot fixes to be immediately effective after having applyied them directly to omni.ja in TB's progam folder.

Comment 26

[Thomas D. (:thomas8)]

Sakshi and I have been communicating to understand the code better and approach a fix, including some mentoring e.g. along my comment 18.

I'm happy to say that the following code changes are working flawlessly to re-enable Ctrl+P and Ctrl+W on my installation:

1) printUtils.js (1): no changes because imo we shouldn't touch toolkit files unless required, so that we can keep them in sync with FF more easily
So the main issue is to make the following keys available as expected by printUtils.js:
<key id="printKb" ...>
<key id="key_close" ...>

2) msgPrintEngine.xul (2): that's the print preview window (also used as a hidden window for printing) which loads printUtils.js.
So I think here's the best place to put the missing <keys>:

<key id="printKb" key="&printCmd.key;" command="cmd_print" modifiers="accel"/>
<key id="key_close" key="&closeCmd.key;" command="cmd_close" modifiers="accel"/>

All other places where <keys> currently live are out of scope, and I think it's good to keep those two right here where they belong (rather than trying to reference large chunks from elsewhere).

Btw: Due to the current design of toolkit, printUtils.js itself is listening for keypresses in onKeyPressPP function and will run its own few commands from there (while all unknown shortcut keys will be cancelled).
So the command="..." attributes of the 2 suggested <keys> in msgPrintEngine.xul are just for display and ease of tracking.

With that, things are better but still broken because none of the existing string entities for the key attribute (e.g. &printCmd.key; to get localized "P" shortcut key) are available in the current context of msgPrintEngine.xul. Solution: see 3) below.

3) msgPrintEngine.dtd: suggested new file to provide the missing string entities (localized "P" and "W") for msgPrintEngine.xul

<!ENTITY printCmd.key "p">
<!ENTITY closeCmd.key "w">

To make them available, add the following line to msgPrintEngine.xul, before the <window> element:

<!DOCTYPE window SYSTEM "chrome://messenger/locale/msgPrintEngine.dtd">

According to MDN Reference on Localization (1):
> Typically, you will have one DTD file for each XUL file.

Following that principle, TB has plenty of small DTDs to go with their respective .xul counterparts, they live here:

http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/

Again, rather than referencing some other huge and mostly unrelated existing DTDs, it's much better and cleaner to just include our own small and file-specific DTD here, especially because we're bridging to toolkit files, a special animal not fully under our control.

There's an existing printPreview.dtd (4), but that's also a toolkit file currently in sync with FF and it does not have any entities for shortcut keys by design, so it doesn't look like a good place to dump our strings there.

With those few little tweaks, Ctrl+P and Ctrl+W will work from print preview as expected :)

(1) http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/printing/content/printUtils.js#291
(2) http://mxr.mozilla.org/comm-central/source/mail/base/content/msgPrintEngine.xul
(3) https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Localization
(4) http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/printPreview.dtd

Comment 27

[sakshi]

Hello,  

I am unable to locate the file msgPrintEngine.dtd. Also when I add
<!DOCTYPE window SYSTEM "chrome://messenger/locale/msgPrintEngine.dtd"> 
msgPrintEngine.xul it gives me an error.

Could you tell me the location of msgPrintEngine.dtd

Comment 28

[sakshi]

Hello,

I made the new file msgPrintEngine.dtd and 
Ctrl+P
Ctrl+W 
are now finally working for me.

Comment 29

[sakshi]

Attachment: Patch: Enable Ctrl+P and Ctrl+W in print preview window (add two missing <keys> to msgPrintEngine.xul; add two missing shortcut key string entities to new file msgPrintEngine.dtd)

Hello Ludovic,

Could you set a reviewer for my patch.

Comment 30

[Ludovic Hirlimann [:Usul]]

Comment on attachment 710060 [details] [diff] [review]
Patch: Enable Ctrl+P and Ctrl+W in print preview window (add two missing <keys> to msgPrintEngine.xul; add two missing shortcut key string entities to new file msgPrintEngine.dtd)

I think mike would do the job for this one.

Comment 31

[Thomas D. (:thomas8)]

Comment on attachment 710060 [details] [diff] [review]
Patch: Enable Ctrl+P and Ctrl+W in print preview window (add two missing <keys> to msgPrintEngine.xul; add two missing shortcut key string entities to new file msgPrintEngine.dtd)

Mike, your review of 5-line patch Attachment 710060 [details] [diff] might be a matter of minutes ;) To ease your review, Comment 26 has a concise explanation of what we tweak here and why. Patch author, Sakshi, is a new contributor who has been very responsive to provide her first patch in close cooperation with me, so a quick review would be ideal to round off this encouraging experience :)

Comment 32

[Mike Conley (:mconley) (:⚙️)]

(In reply to Thomas D. from comment #31)
> Comment on attachment 710060 [details] [diff] [review]
> Patch: Enable Ctrl+P and Ctrl+W in print preview window (add two missing
> <keys> to msgPrintEngine.xul; add two missing shortcut key string entities
> to new file msgPrintEngine.dtd)
> 
> Mike, your review of 5-line patch Attachment 710060 [details] [diff] might
> be a matter of minutes ;) To ease your review, Comment 26 has a concise
> explanation of what we tweak here and why. Patch author, Sakshi, is a new
> contributor who has been very responsive to provide her first patch in close
> cooperation with me, so a quick review would be ideal to round off this
> encouraging experience :)

Sold. On it.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 710060 [details] [diff] [review]
Patch: Enable Ctrl+P and Ctrl+W in print preview window (add two missing <keys> to msgPrintEngine.xul; add two missing shortcut key string entities to new file msgPrintEngine.dtd)

Review of attachment 710060 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following nits fixed. Thanks!

::: mail/base/content/msgPrintEngine.xul
@@ +21,5 @@
>  
>    <stringbundleset id="stringbundleset">
>      <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
>      <stringbundle id="bundle_messenger" src="chrome://messenger/locale/messenger.properties"/>
>   </stringbundleset>

Nit - the lines from 25 to 30 need their indentations fixed.

@@ +23,5 @@
>      <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
>      <stringbundle id="bundle_messenger" src="chrome://messenger/locale/messenger.properties"/>
>   </stringbundleset>
>  
> + <key id="printKb" key="&printCmd.key;" command="cmd_print"  modifiers="accel"/>

Nit - just one space before modifiers attribute please.

::: mail/locales/en-US/chrome/messenger/msgPrintEngine.dtd
@@ +1,1 @@
> +<!ENTITY printCmd.key "P">

This file needs a license header.

Comment 34

[sakshi]

Attachment: The new patch with all the modifications.

Comment 35

[Ian Neal]

Comment on attachment 712080 [details] [diff] [review]
The new patch with all the modifications.

>+++ b/mail/base/content/msgPrintEngine.xul

>  <stringbundleset id="stringbundleset">
>    <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
>    <stringbundle id="bundle_messenger" src="chrome://messenger/locale/messenger.properties"/>
> </stringbundleset>
> 
>-  <script type="application/javascript" src="chrome://global/content/printUtils.js"/>
>+ <key id="printKb" key="&printCmd.key;" command="cmd_print" modifiers="accel"/>
>+ <key id="key_close" key="&closeCmd.key;" command="cmd_close" modifiers="accel"/>
>+
>+ <script type="application/javascript" src="chrome://global/content/printUtils.js"/>
>   <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
>   <script src="chrome://messenger/content/msgPrintEngine.js"/>
I think the intention was for the <stringbundleset> and the <key>s to be properly indented by 2 spaces rather than making one of the <script> lines only indented by 1 space.

Comment 36

[sakshi]

Attachment: Made the changes

Made the changes.

Comment 37

[Ian Neal]

Comment on attachment 712129 [details] [diff] [review]
Made the changes

>+++ b/mail/base/content/msgPrintEngine.xul

>  <stringbundleset id="stringbundleset">
>    <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
>    <stringbundle id="bundle_messenger" src="chrome://messenger/locale/messenger.properties"/>
> </stringbundleset>
Closer, but the </stringbundleset> line above needs indenting by one more space whilst you are here (so it lines up with <stringbundleset>).

>- <script type="application/javascript" src="chrome://global/content/printUtils.js"/>
>+  <key id="printKb" key="&printCmd.key;" command="cmd_print" modifiers="accel"/>
>+  <key id="key_close" key="&closeCmd.key;" command="cmd_close" modifiers="accel"/>
>+
>+  <script type="application/javascript" src="chrome://global/content/printUtils.js"/>
>   <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
>   <script src="chrome://messenger/content/msgPrintEngine.js"/>

Comment 38

[sakshi]

Changed made.

Comment 39

[sakshi]

Attachment: Made the changes

Comment 40

[Thomas D. (:thomas8)]

^^ I deliberately hadn't obsoleted the attachments yet to let Sakshi do that herself ;)

I'll also comment on the current patch.

Comment 41

[Thomas D. (:thomas8)]

Comment on attachment 712146 [details] [diff] [review]
Made the changes

Sakshi, thanks for providing the updated patches so quickly, that's great.
Pls remember to set "obsolete" flag on old patches and remove their review requests.
In fact, let's not request review from Mike for each little nitfix, we can sort that out on a lower level here given the existing review+ for the technical parts of the patch. You can set "needinfo?anyone" (or me) to check if there's anything else.

># HG changeset patch
>...
>Bug 581470- Ctrl+P and Ctrl+W does not work inside Print Preview in Thunderbird

Nit: Pls add a space before the hyphen

>diff --git a/mail/base/content/msgPrintEngine.xul b/mail/base/content/msgPrintEngine.xul
>--- a/mail/base/content/msgPrintEngine.xul
>+++ b/mail/base/content/msgPrintEngine.xul
>...
><window id="printEngineWin"

Nit: Pls add a space before <window... because all unchanged lines require one leading space (only in the patch file itself) to align them correctly with the changed lines marked + or -

>  <stringbundleset id="stringbundleset">
>    <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
>    <stringbundle id="bundle_messenger" src="chrome://messenger/locale/messenger.properties"/>
>  </stringbundleset>

<stringbundleset...>: That's correct now, as in the current original on comm-central, so we're correctly not changing anything here.

>- <script type="application/javascript" src="chrome://global/content/printUtils.js"/>
>+  <key id="printKb" key="&printCmd.key;" command="cmd_print" modifiers="accel"/>
>+  <key id="key_close" key="&closeCmd.key;" command="cmd_close" modifiers="accel"/>

Given the pretty complex distribution of things and the rather complex interaction with toolkit files, can we please add the following comment above the <keys>, with 2 spaces indentation in the original file:

+  <!-- Provide shortcut keys for toolkit's print preview; commands will be overridden by printUtils.js -->
+  <key id="printKb" key="&printCmd.key;" command="cmd_print" modifiers="accel"/>
+  <key id="key_close" key="&closeCmd.key;" command="cmd_close" modifiers="accel"/>

>+
>+  <script type="application/javascript" src="chrome://global/content/printUtils.js"/>

That's correctly indented now :)

>   <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
>   <script src="chrome://messenger/content/msgPrintEngine.js"/>
> 
>   <!-- The main display frame -->
>   <browser id="content" type="content-primary" name="content" src="about:blank" flex="1" disablehistory="true" disablesecurity="true"/>
> </window>
>diff --git a/mail/locales/en-US/chrome/messenger/msgPrintEngine.dtd b/mail/locales/en-US/chrome/messenger/msgPrintEngine.dtd
>new file mode 100644
>--- /dev/null
>+++ b/mail/locales/en-US/chrome/messenger/msgPrintEngine.dtd
>@@ -0,0 +1,2 @@

These numbers need correction, because the following 4 lines have been added (and more below):

>     - This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>

The above lines are lacking the + as they are also added to the new file.
The first line needs an introducing opening comment tag (<!--):

+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+   - License, v. 2.0. If a copy of the MPL was not distributed with this
+   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+

For the same reasons as above, I'd also strongly recommend adding a Localization Note here between the License and the entities, to explain why we have those lonely keys in their own file, i.e. where they belong and what they do:

+<!-- LOCALIZATION NOTE (printCmd.key, closeCmd.key):
+     As defined in msgPrintEngine.xul, Ctrl plus the command keys defined here
+     will be the keyboard shortcuts for print preview, e.g. Ctrl+P
+     -->
+
+<!ENTITY printCmd.key "P">
+<!ENTITY closeCmd.key "W">

Thanks!

Comment 42

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #41)
> For the same reasons as above, I'd also strongly recommend adding a
> Localization Note here between the License and the entities, to explain why
> we have those lonely keys in their own file, i.e. where they belong and what
> they do:
> 
> +<!-- LOCALIZATION NOTE (printCmd.key, closeCmd.key):
> +     As defined in msgPrintEngine.xul, Ctrl plus the command keys defined
> here
> +     will be the keyboard shortcuts for print preview, e.g. Ctrl+P

Correction, avoid ambiguity of localization note:

+<!-- LOCALIZATION NOTE (printCmd.key, closeCmd.key):
+     As defined in msgPrintEngine.xul, Ctrl plus the command keys defined here
+     will be the keyboard shortcuts effective in print preview, e.g. Ctrl+P
+     -->

Comment 43

[sakshi]

Attachment: Made the changes

Comment 44

[Magnus Melin [:mkmelin]]

Comment on attachment 712226 [details] [diff] [review]
Made the changes

I doubt this actually works, as you don't package the new dtd file (in jar.mn).

Also, it's begging for trouble doing changes to the actual patch files. Make the changes in the source and generate a new patch. That way you can also recompile and see that the changes really worked ;)

Comment 45

[Thomas D. (:thomas8)]

Comment on attachment 712226 [details] [diff] [review]
Made the changes

Review of attachment 712226 [details] [diff] [review]:
-----------------------------------------------------------------

Sakshi, per Magnus Comment 44, we need to iron out a few little things here to get the new file included, and get the line numbers right. The patch is doing the right thing in general (per mconley's review+ of initial patch), but some of the technical details aren't working yet. So I'll set review- for now until we'll have these addressed.

I'll provide some assistance later and/or via pm.

Comment 46

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #44)
> Comment on attachment 712226 [details] [diff] [review]
> Made the changes
> 
> I doubt this actually works, as you don't package the new dtd file (in
> jar.mn).

+1. Thanks Magnus, that's helpful (for my learning curve, too...).

Sakshi, pls have a look at these:

https://developer.mozilla.org/en-US/docs/JAR_Manifests

For a practical example very close to what we need here, check out this patch which also adds a new dtd file into a jar archive:

Bug 738194, attachment 666024 [details] [diff] [review]

The complete changeset belonging to that patch (after landing) is this:
http://hg.mozilla.org/comm-central/rev/328ac7ab1686

That patch also adds a new dtd file, like this:
http://hg.mozilla.org/comm-central/diff/328ac7ab1686/mail/locales/en-US/chrome/messenger/viewZoomOverlay.dtdhttp://hg.mozilla.org/comm-central/diff/328ac7ab1686/mail/locales/en-US/chrome/messenger/viewZoomOverlay.dtd

...and includes that new file into the build package via the respective jar.mn:
http://hg.mozilla.org/comm-central/diff/328ac7ab1686/mail/locales/jar.mn

In the same way, we need to add a pointer to the new msgPrintEngine.dtd file here:

http://mxr.mozilla.org/comm-central/source/mail/locales/jar.mn

Magnus, are the pointers inside the jar.mn ordered in any way (apart from per target folder)? Otherwise, can we just add our pointer after line 117?
http://mxr.mozilla.org/comm-central/source/mail/locales/jar.mn#117

Comment 47

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #46)
> Bug 738194, attachment 666024 [details] [diff] [review]
> http://hg.mozilla.org/comm-central/rev/328ac7ab1686
>
> That patch also adds a new dtd file, like this:
> http://hg.mozilla.org/comm-central/diff/328ac7ab1686/mail/locales/en-US/
> chrome/messenger/viewZoomOverlay.dtdhttp://hg.mozilla.org/comm-central/diff/
> 328ac7ab1686/mail/locales/en-US/chrome/messenger/viewZoomOverlay.dtd

Typo fix:
http://hg.mozilla.org/comm-central/diff/328ac7ab1686/mail/locales/en-US/chrome/messenger/viewZoomOverlay.dtd

> ...and includes that new file into the build package via the respective
> jar.mn:
> http://hg.mozilla.org/comm-central/diff/328ac7ab1686/mail/locales/jar.mn

Comment 48

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #46)
> Magnus, are the pointers inside the jar.mn ordered in any way (apart from
> per target folder)? Otherwise, can we just add our pointer after line 117?
> http://mxr.mozilla.org/comm-central/source/mail/locales/jar.mn#117

They aren't required to be in any order, but yes, for easier (human) findability, add the new file along side with others with same "folder".

Comment 49

[Ian Neal]

(In reply to Magnus Melin from comment #48)
> (In reply to Thomas D. from comment #46)
> > Magnus, are the pointers inside the jar.mn ordered in any way (apart from
> > per target folder)? Otherwise, can we just add our pointer after line 117?
> > http://mxr.mozilla.org/comm-central/source/mail/locales/jar.mn#117
> 
> They aren't required to be in any order, but yes, for easier (human)
> findability, add the new file along side with others with same "folder".

I would say in alphabetical order with others with the same "folder" but I think it is a lost cause in that particular file.

Comment 50

[sakshi]

Attachment: Made the changes in jar.mn

Made the changes in the jar.mn

Comment 51

[Thomas D. (:thomas8)]

Comment on attachment 713343 [details] [diff] [review]
Made the changes in jar.mn

Review of attachment 713343 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mike Conley (:mconley) from comment #33)
> Review of attachment 710060 [details] [diff] [review]:
> -----------------------------------------------------------------
> r=me with the following nits fixed. Thanks!

Complementing Mike's technical review+ for the actual code changes, I'm reviewing the nitfixes of what is still essentially the same patch, including the required one-line pointer in jar.mn per Magnus' comment 44.

In that cooperative sense, r=me with the following nit fixed. Pls don't change anything else :) Thanks!

::: mail/base/content/msgPrintEngine.xul
@@ -11,2 @@
>  <window id="printEngineWin"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

Nit: These 7 attribute lines are correctly unchanged from the original, but due to the manual edits of the patch file, the indentation comes out wrong. All attributes from xmlns to windowtype have 8 leading spaces originally, so being unchanged lines, they should have 9 leading spaces in the patch.

::: mail/locales/jar.mn
@@ +114,5 @@
>    locale/@AB_CD@/messenger/quickFilterBar.dtd                           (%chrome/messenger/quickFilterBar.dtd)
>    locale/@AB_CD@/messenger/safeMode.dtd                                 (%chrome/messenger/safeMode.dtd)
>    locale/@AB_CD@/messenger/taskbar.properties                           (%chrome/messenger/taskbar.properties)
>    locale/@AB_CD@/messenger/junkLog.dtd                                  (%chrome/messenger/junkLog.dtd)
> +  locale/@AB_CD@/messenger/msgPrintEngine.dtd                           (%chrome/messenger/msgPrintEngine.dtd)

That looks correct as it blends in seamlessly with existing references to the siblings of our new file, msgPrintEngine.dtd.

Comment 52

[sakshi]

> ::: mail/base/content/msgPrintEngine.xul
> @@ -11,2 @@
> >  <window id="printEngineWin"
> >         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> 
> Nit: These 7 attribute lines are correctly unchanged from the original, but
> due to the manual edits of the patch file, the indentation comes out wrong.
> All attributes from xmlns to windowtype have 8 leading spaces originally, so
> being unchanged lines, they should have 9 leading spaces in the patch.


Should I add an extra space for attributes from xmlns to windowtype

Comment 53

[Thomas D. (:thomas8)]

(In reply to sakshi from comment #52)
> > ::: mail/base/content/msgPrintEngine.xul
> > @@ -11,2 @@
> > >  <window id="printEngineWin"
> > >         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> > 
> > Nit: These 7 attribute lines are correctly unchanged from the original, but
> > due to the manual edits of the patch file, the indentation comes out wrong.
> > All attributes from xmlns to windowtype have 8 leading spaces originally, so
> > being unchanged lines, they should have 9 leading spaces in the patch.
> 
> 
> Should I add an extra space for attributes from xmlns to windowtype

Yes, I think that's what it boils down to now, just add an extra leading space before them.

Please note: If you create your patch automatically, you will not face such problems of changed lines where you didn't intend change: As Magnus indicated in comment 44, tweaking (creating, editing) the patch file manually has a high potential of making it fail (if not done with utmost precision). E.g. in this case, practically you have removed 1 leading space from those lines, so in the patch they are no longer identical with the original lines, thus the current patch might fail to apply. Debugging such invisible / unintended errors resulting from manual patch production can also cost a lot of time.

Comment 54

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #53)
> (In reply to sakshi from comment #52)
> > > ::: mail/base/content/msgPrintEngine.xul
> > > @@ -11,2 @@
> > > >  <window id="printEngineWin"
> > > >         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> > > 
> > > Nit: These 7 attribute lines are correctly unchanged from the original, but
> > > due to the manual edits of the patch file, the indentation comes out wrong.
> > > All attributes from xmlns to windowtype have 8 leading spaces originally, so
> > > being unchanged lines, they should have 9 leading spaces in the patch.
> > 
> > 
> > Should I add an extra space for attributes from xmlns to windowtype
> 
> Yes, I think that's what it boils down to now, just add an extra leading
> space before them.

Which means: Only if you are NOT generating your patch file automatically (which will also fix this problem), you will need to add an extra leading space in front of these attributes manually, *in your patch file*, not in the source. Then they will have the correct format for "unchanged lines of code in patch" (= original lines of code plus one extra leading space, where the other, changed lines have a + or -).

Comment 55

[sakshi]

Attachment: Made the change with one space

Comment 56

[Thomas D. (:thomas8)]

The "missing leading spaces" issue in "unchanged" lines is more pervasive than I first noticed, and I shouldn't have to review the context lines. Clearing review request for now, this needs more corrections.

Comment 57

[Thomas D. (:thomas8)]

Comment on attachment 713885 [details] [diff] [review]
Made the change with one space

^^

Comment 58

[sakshi]

Attachment: Made the changes

Comment 59

[Thomas D. (:thomas8)]

Comment on attachment 714810 [details] [diff] [review]
Made the changes

Review of attachment 714810 [details] [diff] [review]:
-----------------------------------------------------------------

Please create the patch automatically using the appropriate tools & procedures.
At least the msgPrintEngine.xul section of patch is still not applicable.

Comment 60

[sakshi]

Attachment: The new patch with all the modifications.

Comment 61

[sakshi]

Attachment: The new patch with all the modifications.

Comment 62

[Thomas D. (:thomas8)]

Comment on attachment 714886 [details] [diff] [review]
The new patch with all the modifications.

Review of attachment 714886 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome! :) This patch will apply cleanly. Thanks for speedy responses.

r=me after fixing the following nit already mentioned in comment 33:

::: mail/base/content/msgPrintEngine.xul
@@ +24,5 @@
>      <stringbundle id="bundle_messenger" src="chrome://messenger/locale/messenger.properties"/>
>    </stringbundleset>
>  
> +  <!-- Provide shortcut keys for toolkit's print preview; commands will be overridden by printUtils.js -->
> +  <key id="printKb" key="&printCmd.key;" command="cmd_print"  modifiers="accel"/>

(In reply to Mike Conley (:mconley) from comment #33)
> Comment on attachment 710060 [details] [diff] [review]
> > + <key id="printKb" key="&printCmd.key;" command="cmd_print"  modifiers="accel"/>
> 
> Nit - just one space before modifiers attribute please.

Pls remove the second space.

Comment 63

[sakshi]

Attachment: New patch

Comment 64

[Thomas D. (:thomas8)]

Comment on attachment 714891 [details] [diff] [review]
New patch

Review of attachment 714891 [details] [diff] [review]:
-----------------------------------------------------------------

Phantastic. Sakshi has a lightning-fast response time. As far as I can see - perfectly nit-free! :)
r=me (based on :mconley's technical review+ in comment 33, and bwinton's ui-review+ in comment 3)

Comment 65

[Thomas D. (:thomas8)]

Ready for checkin per comment 64.

Certainly safe to land on aurora, too, if that's compatible with localizing procedures. I'd guess Ctrl+P and Ctrl+W keyboard shortcuts are the same for most localizations...

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/b86825ef1b70