363265 - Scroll arrows, page up/page down, home, end, and space don't work in print preview

Status: VERIFIED FIXED

(Core :: Print Preview, defect, P2)

Description

[Eli Friedman]

Steps to reproduce:

1. Print preview.
2. Use the scroll arrows on your keyboard.

Expected:
Page scrolls up and down

Actual:
Nothing happens.

I'm pretty sure this is Seamonkey only.

Side note: Discovered while working on print code; I thought I broke something because it works on FF trunk.  How different is the print preview code between Seamonkey and Firefox?

Comment 1

[Frank Wein [:mcsmurf]]

(In reply to comment #0)
> Side note: Discovered while working on print code; I thought I broke something
> because it works on FF trunk.  How different is the print preview code between
> Seamonkey and Firefox?

I think they use different XML overlays, JS code, etc. see for example /toolkit/components/printing/content/printPreviewBindings.xml and 
/xpfe/communicator/resources/content/printPreviewBindings.xml (same goes for other files). /suite/common/printPreviewBindings.xml also seems to be used these days?

Comment 2

[Wayne Mery (:wsmwk)]

do pgup/pgdn/home/end also fail (does for me)

arrows, etc fail for me both FF and SM trunk.  you might check current FF trunk and if so it (obviously) regressed from what you reported.

my FF is Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070227 Minefield/3.0a3pre

I don't see likely bug suspects, but here are some references
bug 281932
bug 250442
bug 311028

Comment 3

[Wayne Mery (:wsmwk)]

ah ha, fallout as Eli notes in bug 361844 comment 4 

Comment 4

[Martijn Wargers (dead)]

This is also a problem in Firefox.

Comment 5

[Stuart Parmenter]

not sure if this is helpful -- I don't understand our DOM keypress event stuff very well, but whenever i page up or down in print preview I end up with:

WARNING: GetCharCode used for wrong key event; should use onkeypress.: file c:/builds/mozilla/objdir/content/events/src/../../../../content/events/src/nsDOMKeyboardEvent.cpp, line 108

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

So pageup/down etc scrolls the primary shell, not print preview shell.
Eli, I wouldn't want to back out too much of bug 361844. Do you see some
simple way to ensure that right shell is used as nsISelectionController?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

This fixes keyboard scrolling, and makes full zooming to affect print preview.
Full zoom zooms pages, so doesn't affect the layout. I actually like that, but I'm not at all sure what others think about that.
In 1.8 text zoom does affect print preview layout.

The changes in zoom handling make sure that when someone zooms print preview, the changes don't affect normal layout and vice versa.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP2

Don't save domain specific zoom levels if using zoom in print preview

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP3

This fixes problems when one window is in printpreview and one in normal mode 
and the window in normal mode is zoomed.

Roc, what do you think about documentviewer changes? mPresShell of DocumentViewer points always to the first shell, but when in printpreview, the public GetPresShell(nsIPresShell** aShell) method (and other similar methods) returns the last shell of document. Not perhaps the nicest fix, but something like that is needed unless bug 361844 is backed out (and that means backing out some other patches too etc.).

At least this patch makes keyboard and zooming working in print preview.

Comment 10

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

Comment on attachment 287213 [details] [diff] [review]
WIP3

This looks good to me but you need to check who's calling GetPresShell/GetPresContext/GetViewManager on the documentviewer and make sure they can handle getting a print-preview shell.

Also, document mTextZoom/mPageZoom to make it clear that these record the textzoom/pagezoom of the first (galley) presshell only. "Print preview doesn't use these" is not quite clear enough.

You'll need other review on the chrome changes of course.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #10)
> (From update of attachment 287213 [details] [diff] [review])
> This looks good to me but you need to check who's calling
> GetPresShell/GetPresContext/GetViewManager on the documentviewer and make sure
> they can handle getting a print-preview shell.
Should be ok. And those methods used to return printpreview shell/ctx/vm.
But will re-check still.

> Also, document mTextZoom/mPageZoom to make it clear that these record the
> textzoom/pagezoom of the first (galley) presshell only. "Print preview doesn't
> use these" is not quite clear enough.
Will fix.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 287213 [details] [diff] [review]
WIP3

Myk, perhaps you could review browser/ part of the patch?

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 287213 [details] [diff] [review]
WIP3

>Index: browser/base/content/browser-textZoom.js

>   _handleMouseScrolled: function (event) {
>+    // If we're in print preview, no need to set the pref.
>+    if (gInPrintPreviewMode)
>+      return;

This seems unnecessary, given that _handleMouseScrolled sets the pref by calling _applySettingToPref, which contains the same logic.


>+  setPrefValue: function () {
>+    if (this._cps.hasPref(gBrowser.currentURI, this.name)) {
>+      ZoomManager.fullZoom = this._cps.getPref(gBrowser.currentURI, this.name);
>+    } else {
>+      this.reset();
>+    }
>+  },

This needs to take a global pref into account as well.  A simple way to do that is to just call _applyPrefToSetting with the value of the site-specific pref (if any), i.e.:

  setPrefValue: function FullZoom_setPrefValue() {
    var value = this._cps.getPref(gBrowser.currentURI, this.name);
    this._applyPrefToSetting(value);
  },

That method will then take care of applying the global pref (or resetting to the default value) if there isn't a site-specific pref.

Also, this would more accurately be called setSettingValue (or setSettingForCurrentSite, to make it clear that we're setting the setting to whatever value makes sense for the current site), since, in the context of site-specific preferences, a "pref" is a value stored in the preferences datastore, while a "setting" is a browser feature (like zoom) that uses site-specific prefs when available, and it's the setting that is being updated by this function.


>   _applyPrefToSetting: function (aValue) {
>+    if (gInPrintPreviewMode)
>+      return;
>     // Bug 375918 means this will sometimes throw, so we catch it
>     // and don't do anything in those cases.

Nit: it'd be good to have a line of space between the return and the following comment.


>   _applySettingToPref: function () {
>-    var fullZoom = ZoomManager.fullZoom;
>-    this._cps.setPref(gBrowser.currentURI, this.name, fullZoom);
>+    if (!gInPrintPreviewMode) {
>+      var fullZoom = ZoomManager.fullZoom;
>+      this._cps.setPref(gBrowser.currentURI, this.name, fullZoom);
>+    }
>   },

Nit: I think this'd be simpler and require changing less code if written as an early return, like the others have been written, i.e.:

  _applySettingToPref: function () {
    if (gInPrintPreviewMode)
      return;

    var fullZoom = ZoomManager.fullZoom;
    this._cps.setPref(gBrowser.currentURI, this.name, fullZoom);
  },


>Index: browser/base/content/browser.js

> var gMustLoadSidebar = false;
> var gProgressMeterPanel = null;
> var gProgressCollapseTimer = null;
> var gPrefService = null;
> var appCore = null;
> var gBrowser = null;
> var gNavToolbox = null;
> var gSidebarCommand = "";
>-
>+var gInPrintPreviewMode = false;
> // Global variable that holds the nsContextMenu instance.
> var gContextMenu = null;

Nit: I'd leave in the line of whitespace before the "global variable" comment.


> function onEnterPrintPreview()
> {
>+  gInPrintPreviewMode = true;
>   toggleAffectedChrome(true);
> }

Would it make sense to reset the zoom at this point?  Given that changes to the zoom while in print preview mode don't affect normal mode, perhaps changes to the zoom in normal mode shouldn't affect print preview mode either.


> This fixes keyboard scrolling, and makes full zooming to affect print preview.

Keyboard scrolling works for me now, but zooming didn't work for me in print preview mode.  I tried the keyboard shortcuts (ctrl-Plus, ctrl-Minus), but I haven't tried using a scrollwheel yet.  Perhaps the keyboard shortcuts aren't hooked up in print preview mode?


> Full zoom zooms pages, so doesn't affect the layout. I actually like that, but
> I'm not at all sure what others think about that.
> In 1.8 text zoom does affect print preview layout.

I think this is the right behavior.  One analogy is PDF viewers, where you can zoom in on a document, and that doesn't affect its layout (or its size when printed).

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #13)
> This seems unnecessary, given that _handleMouseScrolled sets the pref by
> calling _applySettingToPref, which contains the same logic.
But _applySettingToPref is called after a timeout, so that shouldn't get called 
if mode changes before timeout fires.

> Would it make sense to reset the zoom at this point?  Given that changes to the
> zoom while in print preview mode don't affect normal mode, perhaps changes to
> the zoom in normal mode shouldn't affect print preview mode either.
That is done in C++ code.

> Keyboard scrolling works for me now, but zooming didn't work for me in print
> preview mode.  I tried the keyboard shortcuts (ctrl-Plus, ctrl-Minus), but I
> haven't tried using a scrollwheel yet. 
ctrl++/ctrl+- don't work. They don't work in 1.8 either. 
That is different bug. I'm "just" fixing regressions here.
The patch fixes ctrl+mousewheel, and pageup/down/etc.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #14)
> (In reply to comment #13)
> > This seems unnecessary, given that _handleMouseScrolled sets the pref by
> > calling _applySettingToPref, which contains the same logic.
> But _applySettingToPref is called after a timeout, so that shouldn't get called 
Hmmm, right, this shouldn't be needed after all.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fixes to browser/

And yes, I prefer the zooming behavior we get with this patch. It is similar
to what Opera has. Additional bug might be to hook up ctrl++/ctrl+-.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 287704 [details] [diff] [review]
fixes to browser/

Looks good, works well. r=myk

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 287704 [details] [diff] [review]
fixes to browser/

Oops, this is blocking 1.9, no need for approval

Comment 19

[Steffen Wilberg]

(In reply to comment #11)
> (In reply to comment #10)
> > Also, document mTextZoom/mPageZoom to make it clear that these record the
> > textzoom/pagezoom of the first (galley) presshell only. "Print preview doesn't
> > use these" is not quite clear enough.
> Will fix.

Looks like you didn't address this.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

yikes, forgot. Will fix.

Comment 21

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112805 Minefield/3.0b2pre. Scroll bars, pgup/pgdn/home/end all work for me.