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

VERIFIED FIXED in mozilla1.9beta2

Status

()

Core
Print Preview
P2
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Eli Friedman, Assigned: smaug)

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla1.9beta2
x86
Windows XP
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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

11 years ago
(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

11 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

11 years ago
ah ha, fallout as Eli notes in bug 361844 comment 4 
Blocks: 361844
This is also a problem in Firefox.
Assignee: general → nobody
Component: General → Print Preview
Product: Mozilla Application Suite → Core
QA Contact: general → printing

Updated

10 years ago
Flags: blocking1.9?
Keywords: regression
Flags: blocking1.9? → blocking1.9+

Updated

10 years ago
Summary: Scroll arrows don't work in print preview → Scroll arrows, page up/page down, home, end, and space don't work in print preview

Updated

10 years ago
Priority: -- → P4

Comment 5

10 years ago
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
Priority: P4 → P2
(Assignee)

Updated

10 years ago
Assignee: nobody → Olli.Pettay
(Assignee)

Comment 6

10 years ago
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?

Updated

10 years ago
Blocks: 361941
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9 M10
(Assignee)

Comment 7

10 years ago
Created attachment 287105 [details] [diff] [review]
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.
(Assignee)

Comment 8

10 years ago
Created attachment 287155 [details] [diff] [review]
WIP2

Don't save domain specific zoom levels if using zoom in print preview
Attachment #287105 - Attachment is obsolete: true
(Assignee)

Comment 9

10 years ago
Created attachment 287213 [details] [diff] [review]
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.
Attachment #287155 - Attachment is obsolete: true
Attachment #287213 - Flags: review?(roc)
(Assignee)

Updated

10 years ago
Blocks: 372248
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.
Attachment #287213 - Flags: superreview+
Attachment #287213 - Flags: review?(roc)
Attachment #287213 - Flags: review+
(Assignee)

Comment 11

10 years ago
(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.
(Assignee)

Comment 12

10 years ago
Comment on attachment 287213 [details] [diff] [review]
WIP3

Myk, perhaps you could review browser/ part of the patch?
Attachment #287213 - Flags: review?(myk)
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).
Attachment #287213 - Flags: review?(myk) → review-
(Assignee)

Comment 14

10 years ago
(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.
(Assignee)

Comment 15

10 years ago
(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.
(Assignee)

Comment 16

10 years ago
Created attachment 287704 [details] [diff] [review]
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+-.
Attachment #287213 - Attachment is obsolete: true
Attachment #287704 - Flags: review?(myk)
Comment on attachment 287704 [details] [diff] [review]
fixes to browser/

Looks good, works well. r=myk
Attachment #287704 - Flags: review?(myk) → review+
(Assignee)

Updated

10 years ago
Attachment #287704 - Flags: approval1.9?
(Assignee)

Comment 18

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

Oops, this is blocking 1.9, no need for approval
Attachment #287704 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 19

10 years ago
(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.
(Assignee)

Comment 20

10 years ago
yikes, forgot. Will fix.
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.
Status: RESOLVED → VERIFIED
Blocks: 444448
Depends on: 444458
You need to log in before you can comment on or make changes to this bug.