400568 - Keyboard Back/Forward keys sending VK_BROWSER_BACK/FORWARD go back/forward twice

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[bugs1m.wisefool]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8) Gecko/2007091216 GranParadiso/3.0a8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8) Gecko/2007091216 GranParadiso/3.0a8

This is a regression I'm noticing on my Thinkpad T60, Windows XP laptop. If I click on a fragment identifier for the page I'm on currently, pressing Thinkpad's Back key will jump back two URL's instead of one. Alt+left still goes back one as expected. The Forward key has the same problem.

Reproducible: Always

Steps to Reproduce:
1. Go to about:
2. Go to http://en.wikipedia.org/wiki/Mozilla_Firefox
3. Click 'History' in table of contents
4. Press 'Back' button on Thinkpad keyboard
Actual Results:  
about: is displayed

Expected Results:  
It should behave the same as if I pressed the Back button in the UI

Comment 1

[Jesse Maloney (jtmal0723 [at] gmail for email)]

I can't confirm this for you, I'm afraid. Perhaps try it with the latest build first.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102005 Minefield/3.0a9pre ID:2007102005

Comment 2

[bugs1m.wisefool]

I just checked; it still happens with a fresh profile in the 2007-10-21 nightly.

Comment 3

[Martijn Wargers (dead)]

Do you happen to know when this started regressing?

Comment 4

[bugs1m.wisefool]

Alright, I just tested it; this happens in the 2006-12-01-04 nightly, but doesn't happen in the 2006-11-30-04 nightly or Gran Paradiso Alpha 1.

Comment 5

[bugs1m.wisefool]

I'm still experiencing this. Is there any information in my system configuration I can ferret out that would be useful? I'm not sure if the back/forward keys are interpreted by Windows XP, or a Thinkpad driver that might be the problem.

Again, this first regressed in the 2006-12-01-04 nightly.

Comment 6

[Honza Bambas (:mayhemer)]

I could not reproduce this with FF 3.0.

Comment 7

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

(In reply to comment #6)
> I could not reproduce this with FF 3.0.
> 

The reporter wrote this bug still happens to him. Furthermore the correct resolution should be at least worksforme :-) I reopen it.

Wisefool, does this bug happens to you with the latest trunk (3.1) with a blank profile?

Comment 8

[qgl6049]

I have a Thinkpad T43p and used to experience this bug, then I updated a bunch of Thinkpad drivers and the problem disappeared.  Apparently Lenovo has squashed this one for us.

Comment 9

[Honza Bambas (:mayhemer)]

I was testing with quit new T61 and could not reproduce this problem.

In reply to comment 8: could you please just post a comment for record where/how did you update the drivers if it were not updated automatically by Lenovo update software? It could be useful for others still experiencing the problem and looking for solution.

Thanks.

Comment 10

[qgl6049]

Actually, the driver update does not seem to have entirely corrected the problem.  Now it seems to do it only some of the time.  Once it starts, it will consistently do it while Firefox remains open, but I can't always force it to reproduce the bug.

Comment 11

[qgl6049]

After additional testing...

I have managed to encounter the bug on a non-Thinkpad.  Apparently it is caused by the generic "Back" and "Forward" keyboard codes, as I have a pretty generic USB keyboard that didn't come with any driver, and using its Back and Forward buttons produce the problem.  Presumably, these are the VK_BROWSER_BACK and VK_BROWSER_FORWARD keycodes in Windows.

Also, it only occurs when the current location and the previous (or next, in the case of forward) location are different parts of the same page.  The page you end up on (+/- 2 locations) doesn't seem to matter.

Hopefully this information will help others reproduce the bug.

Comment 12

[Simon Bünzli]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208

Confirming. One further observation: When the issue happens, the load state of the jumped over page and the one following it isn't "Done" but "Stopped".

Comment 14

[Honza Bambas (:mayhemer)]

qgl6049: thanks, I can reproduce this. Assigning to me.

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v1

Problem is that we receive two events: WM_APPCOMMAND with lParam=APPCOMMAND_BROWSER_BACKWARD/FORWARD which is handled from PeekKeyAndIMEMessage and event WK_(SYS)KEYDOWN with code VK_BROWSER_BACK/FORWARD handled later in the same cycle. Both these events lead to call of nsSHistory::GoBack/GoForward which goes exactly to history item at nsSHistory::mIndex+1 or -1. In case of anchor navigation (moving on the same page) mIndex indicating on what item of the history list we currently are is updated synchronously (call to nsSHistory::UpdateIndex), before the second call to nsSHistory::GoBack/GoForward function. We actually always duplicate its call when using these back/forward keys but it is ignored because usually mIndex is not updated that soon and leads just to reload fo the same page, invisible to user.

The patch therefor ignores the key events because it seems that system always indicates its press by application command event type as well. I have no chance to test this with a mouse having back/forward buttons nor with such a keyboard.

Comment 16

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

Or perhaps OnKeyDown shouldn't just call DispatchCommandEvent at all?
So basically removing #ifdef VK_BROWSER_BACK...#endif.
(I don't have windows+right kind of keyboard to test)

Comment 17

[Ere Maijala (slow)]

Comment on attachment 348688 [details] [diff] [review]
v1

I believe Smaug is right, though it seems the only way to be sure is remove the whole block, check it in and hope nothing regresses. I don't think we need to prevent the keys from being dispatched, or then we should do that consistently to all such keys.

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v2

This patch removes the special handling of back and forward keys from both OnKeyDown/Up. It seems to work the same way as the previos patch.

Comment 19

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

I just posted the patch to TryServer so that anyone with right kind of
keyboard can test the patch even before it lands trunk.
https://build.mozilla.org/tryserver-builds/2008-11-19_03:[e-mail]-win_b_f/

The Windows build isn't ready yet.

Comment 20

[Samuel Sidler (old account; do not CC)]

Comment on attachment 348826 [details] [diff] [review]
v2

Pushing out nomination since 1.9.0.5 is basically code frozen.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #19)
> I just posted the patch to TryServer so that anyone with right kind of
> keyboard can test the patch even before it lands trunk.
> https://build.mozilla.org/tryserver-builds/2008-11-19_03:[e-mail]-win_b_f/
> 
> The Windows build isn't ready yet.

Did anyone do this? Do we know that this patch works?

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348826 [details] [diff] [review]
v2

Not approved until someone answers my question in the previous comment; feel free to renominate once that's done.

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 348826 [details] [diff] [review]
v2

Just confirmed with testing build and keyboard with BACK/FORWARD keys: FF 3.0.5 - skips by two, testing build - skips by one. Renominating, unfortunately too late for 3.0.6.

Comment 24

[Samuel Sidler (old account; do not CC)]

Comment on attachment 348826 [details] [diff] [review]
v2

Moving nomination to 1.9.0.7.

Comment 25

[Dão Gottwald [:dao]]

Why hasn't this landed on trunk yet?

Comment 26

[Honza Bambas (:mayhemer)]

(In reply to comment #25)
> Why hasn't this landed on trunk yet?

Landed on trunk as http://hg.mozilla.org/mozilla-central/rev/19757a81c07c.

Comment 27

[Samuel Sidler (old account; do not CC)]

(In reply to comment #26)
> Landed on trunk as http://hg.mozilla.org/mozilla-central/rev/19757a81c07c.

RESOLVED FIXED, then.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 348826 [details] [diff] [review]
v2

We'll wait until after 1.9.1 to approve.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348826 [details] [diff] [review]
v2

a191=beltzner

Can we get a followup test for this (and perhaps other keybindings as well) please?

Comment 30

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

This needs a litmus test, afaik.

Comment 31

[Honza Bambas (:mayhemer)]

I don't think I can create an automated test for this. If this needs a litmus test, please add it someone, I wasn't able to find a way to add a new test. I can provide exact and simple STR if needed.

Comment 32

[Honza Bambas (:mayhemer)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8effb4f50f25

Comment 33

[bzliu94]

I'm getting this on my Thinkpad T60P, Windows XP laptop, on Google search results.

Small note: Strangely, it works fine on google.com/firefox.

Comment 34

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

Have you tested it in a blank profile: 
http://support.mozilla.com/en-US/kb/Managing+profiles#Starting_the_Profile_Manager 
with the latest trunk: 
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/ 
?

Comment 35

[bzliu94]

Okay, it's good in the latest trunk.
I am running 3.0.11.