Closed Bug 1009890 Opened 6 years ago Closed 6 years ago

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

Categories

(SeaMonkey :: MailNews: General, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(seamonkey2.32 fixed)

RESOLVED FIXED
seamonkey2.32
Tracking Status
seamonkey2.32 --- fixed

People

(Reporter: email, Assigned: philip.chee)

Details

Attachments

(1 file, 2 obsolete files)

I was asked to port Bug 581470 from Thunderbird to add Ctrl+P and Ctrl+W shortcuts to the Print Preview window as implemented in TB.
Assignee: nobody → email
Status: NEW → ASSIGNED
Attached patch Ported Patch (obsolete) — Splinter Review
Attachment #8422050 - Flags: review?(iann_bugzilla)
Attachment #8422050 - Flags: feedback?(philip.chee)
Attachment #8422050 - Flags: review?(iann_bugzilla)
Attachment #8422050 - Flags: feedback?(philip.chee)
Attached patch patch.diff (obsolete) — Splinter Review
Remind me to actually TEST patches before I propose them!
Attachment #8422050 - Attachment is obsolete: true
Attachment #8422075 - Flags: review?(iann_bugzilla)
Attachment #8422075 - Flags: feedback?(philip.chee)
Note that the traditional way of doing this in SeaMonkey is to overlay utilityOverlay.xul which provides you with the definitions of key_close and key_print. (key_close lives in platformCommunicatorOverlay.xul but that's just an implementation detail.)

Also note that keys don't work unless you put them in a keyset.
Hmm.. This implementation does work but if I am in essence replicating already defined methods I should do it that way. I will look into it. I just hope I can get a handle of implementing things like this the proper way.
Seems utilityOverlay is already present. Simply adding key_close works fine with or without an enclosing keyset tag. However, key_print doesn't function at all from the print preview window. Though alt+P does start printing so do we actually want ctrl+P?

At any rate I do think key_close needs to be on every window for consistency.
Comment on attachment 8422075 [details] [diff] [review]
patch.diff

So the key bit is this:
<!-- Provide shortcut keys for toolkit's print preview; commands will be overridden by printUtils.js -->

Looking at the source for onKeyPressPP:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/printing/content/printUtils.js?rev=5959f80e1831#292

We see that the keypress listener:
(1) Grabs the event before it can reach our key handlers in utilityOverlay and does it's own processing for these two keys.
(2) is hard coded to look for "printKb" Which is why <key id="key_print"> doesn't work.
(3) Note onKeyPressPP looks for both ALT-W and CTRL-W, ALT-P and CTRL-P

Suggestion 1: minimal changes needed:
-------------------------------------
utilityOverlay.xul:

   <key id="key_print"
        key="&printCmd.key;"
        command="cmd_print"
        modifiers="accel"/>
+  <key id="printKb"
+       key="&printCmd.key;"/>

msgPrintEngine.xul:

+  <!-- Provide shortcut keys for toolkit's print preview; commands will be overridden by printUtils.js -->
+  <key id="printKb"/>
+  <key id="key_close"/>

Plus remove printEngine.dtd since we don't need it any more.

Suggestion 2:
-------------------------------------
Fix the toolkit printUtils.js and cut down on the insanity.
Attachment #8422075 - Flags: feedback?(philip.chee) → feedback-
Comment on attachment 8422075 [details] [diff] [review]
patch.diff

Nothing to add on top of what Philip said. Short term fix is suggestion 1, long term fix is suggestion 2.
I would do suggestion 1 within this bug and spin off the long term fix to another bug.
Attachment #8422075 - Flags: review?(iann_bugzilla) → review-
So the consensus is Suggestion 1:

> utilityOverlay.xul:

>    <key id="key_print"
>         key="&printCmd.key;"
>         command="cmd_print"
>         modifiers="accel"/>
> +  <key id="printKb"
> +       key="&printCmd.key;"/>
Maybe add |modifiers="accel"|

> msgPrintEngine.xul:

> +  <!-- Provide shortcut keys for toolkit's print preview; commands will be overridden by printUtils.js -->
> +  <key id="printKb"/>
> +  <key id="key_close"/>

> Plus we don't need to add printEngine.dtd at all.

Tobin, care to whip up a  new patch?
Flags: needinfo?(email)
Understood.. Stay tuned for that while I pull latest c-c and get that patch created.
Flags: needinfo?(email)
Assignee: email → nobody
Status: ASSIGNED → NEW
Tobin has moved on to Palemoon. So I'm taking over this bug. This is the minimal patch discussed in previous comments.
Assignee: nobody → philip.chee
Attachment #8422075 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8489152 - Flags: review?(iann_bugzilla)
Attachment #8489152 - Flags: review?(iann_bugzilla) → review+
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/3a8e98f52f91
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.32
You need to log in before you can comment on or make changes to this bug.