158362 - spacebar does not scroll in print preview

Status: VERIFIED FIXED

(Core :: Print Preview, defect, P3)

Description

[Joshua Goldberg]

buildID: 2002061401

Hitting space in print preview mode doesn't scroll.  Only PageDown works.

(always reproducible)
1. Print preview
2. Press spacebar

Comment 1

[R.K.Aa.]

this was also mentioned in bug 111432 comment 1

Comment 2

[R.K.Aa.]

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

Comment 3

[Martijn Wargers (dead)]

Attachment: patch

This makes spacebar scrolling in print preview work for me.

Comment 4

[[e-mail]]

Comment on attachment 188066 [details] [diff] [review]
patch

>-                                  nsIDOMKeyEvent::DOM_VK_TAB, 0};
>+                                  nsIDOMKeyEvent::DOM_VK_TAB, nsIDOMKeyEvent::DOM_VK_SPACE, 0};
Don't need to list space here, it's always a char code.

>     keyEvent->GetShiftKey(&b);
>     if (b) return PR_FALSE;
> 
>     PRUint32 keyCode;
>     keyEvent->GetKeyCode(&keyCode);
>+    PRUint32 charCode;
>+    keyEvent->GetCharCode(&charCode);
>     PRInt32 i = 0;
>     while (kOKKeyCodes[i] != 0) {
>-      if (keyCode == kOKKeyCodes[i]) {
>+      if ((keyCode == kOKKeyCodes[i]) || (charCode == 0x20)) {
No point checking the charCode inside the loop. In fact, as Shift+Space is
legal for page up you might want to move it to before the shift test. Also you
should compare it to ' ' for readability.

Comment 5

[Martijn Wargers (dead)]

Attachment: patch2

Thanks, Neil. All your comments seems logical to me, but for some reason I need
to keep nsIDOMKeyEvent::DOM_VK_SPACE in, otherwise it doesn't work.
Also, I can't get Shift->spacebar scrolling to work.

Comment 6

[Martijn Wargers (dead)]

Neil, maybe you know why I get these results in comment 5? Thanks.

Comment 7

[[e-mail]]

(In reply to comment #6)
>Neil, maybe you know why I get these results in comment 5? Thanks.
Ah, this is due to aaron's keydown prevents keypress code, so we need to allow
space twice, once as a keydown of DOM_VK_SPACE and once as a keypress of " ".
We also need to allow the shift key in both cases. Maybe it would be easier to
write some special-case code rather than trying to hook into the existing loop?

Comment 8

[Martijn Wargers (dead)]

Or remove the keypress eventlistener, which should be safe, because the keydown
eventlisteners prevents keypress events of firing, not? But that doesn't seem to
change anything.

Comment 9

[Martijn Wargers (dead)]

Attachment: patch3

Off course! The shift key also gets blocked. Why not remove that block? That
can't hurt anything, can it? And those keypress event listeners could also be
removed safely, I guess, since keydown also blocks keypress automatically.

Comment 10

[[e-mail]]

What I don't want to happen is for a keypress to either a) activate a shortcut
or b) run a script, when it shouldn't (see bug 245024 for a click example).

Comment 11

[[e-mail]]

Comment on attachment 188327 [details] [diff] [review]
patch3

>-    mEventTarget->AddEventListener(NS_LITERAL_STRING("keypress"), this, true);
Although it's true that calling PreventDefault on the keydown now calls
PreventDefault on the keypress that doesn't handle the StopPropagation case.
r=me on the rest of the patch.

Comment 12

[Martijn Wargers (dead)]

Attachment: patch4

Ok, I've put it back in this patch.

Comment 13

[Mats Palmgren (inactive)]

Attachment: Patch rev. 5

Propagate DOM_VK_SPACE and ' ' with and without SHIFT (but nothing else).
I have tested this on Windows and Linux.

Comment 14

[Mats Palmgren (inactive)]

Checked in to trunk 2005-07-25 17:21 PDT

-> FIXED

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2005-07-26-05 on Windows XP SeaMonkey trunk.