360731 - Back and Forward buttons on mouse no longer work

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

After updating from the 11-13-2006 to the 11-14-2006 trunk build, the back and forward buttons no longer work on my Logitech MX1000 mouse. I got confirmation over at Mozillazine from others with other brands as well.  I'll try to narrow down an hourly regression window.

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

I see this as well with today's build, using a Microsoft Internet Explorer optical 5 button mouse.  Forward/back buttons have not response, and I see nothing in the JS Console to indicate any errors.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061114 Minefield/3.0a1 Firefox ID:2006111405 [cairo]

Comment 2

[Glenn Scheithauer]

I see this as well on my Logitech MX 510 Optical Mouse.

Comment 3

[William Swanson]

Confirmed on Windows Vista RC2 with the Microsoft Trackball Optical.

Comment 4

[Barrie North]

Same problem on XP Pro SP2 with a generic 5 button mouse.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

The checkins in the regression range in comment 0 are:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&branchtype=match&date=explicit&mindate=2006-11-13+02%3A00&maxdate=2006-11-14+06%3A00

I don't see anything in that list that looks like it would have caused this, at first glance. Have the people who can reproduce this tried reproducing it in safe mode? With a new profile? Would it be possible to get a narrower regression range? Does it work in SeaMonkey? If not, does it have the same regression range?

Comment 6

[Glenn Scheithauer]

Still happens in safe mode with a new profile.

Comment 7

[Jim Jeffery not reading bug-mail 1/2/11]

I too have tried Safe-mode, and a new profile - no change.
I just installed the latest reflow-build and the back/forward work as they should.

Don't know if that's any help really ...

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061115 Minefield/3.0a1 Firefox ID:2006111504

Comment 8

[William Swanson]

(In reply to comment #5)
> Does it work in SeaMonkey? If not, does it have the same regression range?

The mouse buttons work fine on SeaMonkey 2006-11-13-09, but fail on SeaMonkey 2006-11-14-09. I don't use SeaMonkey on a regular basis, so these are both fresh installs with new profiles.

Comment 9

[Adam Guthrie]

The only thing that I see that remotely deals with this is Ere's patch for bug 311709. Any ideas, Ere?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

I just went back and did a bunch of cvs checkouts from various times over that date range. As best I can tell, it's roc's patch for bug 130078 which broke it.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I have to leave now, but I'll try reverting the four updated files tonight on the current trunk to confirm that finding.

Comment 12

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

That seems likely. But how to test this?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

If you have an idea of what's wrong, I'd be happy to test any patches you want on my system since I can reproduce it.

Comment 14

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

I don't know how those buttons are supposed to work, do you Ere?

Comment 15

[Ere Maijala (slow)]

I think they are just virtual key codes on Windows (VK_BROWSER_BACK and VK_BROWSER_FORWARD). Just realized I should be able to emulate them and I might even have a suitable mouse somewhere at home.

Comment 16

[Ere Maijala (slow)]

I just noticed that the navigation keys around cursor keys on my laptop send VK_BROWSER_BACK/FORWARD but they work properly. So I guess the Logitech mouse is doing something else. 

Comment 17

[William Swanson]

(In reply to comment #16)
> So I guess the Logitech mouse is doing something else.

I did some quick tests, and it seems that these buttons are generating WM_XBUTTONDOWN messages. See http://msdn2.microsoft.com/en-us/library/ms646245.aspx for more info.

Comment 18

[Ere Maijala (slow)]

Ok, so it's probably WM_APPCOMMAND (see http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/userinput/keyboardinput/keyboardinputreference/keyboardinputmessages/wm_appcommand.asp). I'll try to find a way to test it without a mouse.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Also, comment #3 pointed out that it affects MS mice too.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Sorry for the bugspam, but I think Ere is right about WM_APPCOMMAND. I just noticed that my MS Natural Keyboard Pro's Back and Forward buttons also don't work. So Ere, if you can get ahold of a keyboard which has back/forward buttons (many do these days), that may work as well.

Comment 21

[Ere Maijala (slow)]

Nah, I hacked together an app to emulate the buttons. Perhaps I messed up something as sending NS_APPCOMMAND message to foreground window makes it go to nsWebShellWindow, which doesn't handle NS_APPCOMMAND at all. nsEventStateManager would, but doesn't get a chance. I wonder how the MS keyboard driver chooses the window.

Comment 22

[Ere Maijala (slow)]

Perhaps GetGUIThreadInfo will do the trick.

Comment 23

[Ere Maijala (slow)]

Ok, here's the deal: the whole process starts at nsWindow.cpp where DispatchAppCommandEvent() is called and ends up in chrome's event state manager. Chrome of course doesn't have history and can't go back. I think this would also affect reload and stop (someone mentioned earlier that it's now possible to reload chrome which kind of breaks everything). 

Comment 24

[Ere Maijala (slow)]

Ok, here's the plan after a quick chat: all the events are dispatched from widget layer as nsAppCommandEvents (including VK_BROWSER...). Then they are directed to chrome and handled there in a way that makes sense for the app. 

Comment 25

[Ere Maijala (slow)]

Attachment: Patch v1 for Windows widget part of the fix

Here is a possible patch for the Windows widget layer. Can't be tested before the underlying changes are done.

Comment 26

[Ere Maijala (slow)]

Attachment: Patch v1.1 for Windows widget part of the fix

Of course the virtual keys must be handled in OnKeyUp too..

Comment 27

[Ere Maijala (slow)]

Attachment: Lazarus source for utility emulating the special mouse buttons

Here is a Lazarus/Free Pascal source for the simple utility I created for testing the special buttons for reference if nothing else. To use just click a button and focus the window you want to receive the app command message.

Comment 28

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

I think I'll remove nsAppCommandEvent and
convert .appCommands to real events which can then be handled in chrome.

The handling of those commands could be removed from ESM, I think.
In the case of several <browser>s it would be up to the chrome code
to decide which one has the focus, or something.

Comment 29

[Bill Gianopoulos [:WG9s]]

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

Comment 30

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

Attachment: a test for upcoming patch

Comment 31

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

Attachment: possible patch, adds nsIDOMCommandEvent

Ere, could you test/look at the Windows part.
It includes your patch.

Comment 32

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

(In reply to comment #31)
> Ere, could you test/look at the Windows part.
> It includes your patch.
> 
Because I haven't even compiled it on windows ;)
Using Linux and the test I attached, things seem to work ok.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Olli - your patch fails to compile quite spectacularly on nsWindow.cpp line 1166. I'm assuming you meant "AssignLiteral" and not "AssingLiteral" ;-). I'm doing a test build with that typo corrected now to see if it compiles and fixes the bug.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

OK, I have good news and I have bad news.

The Good: With the typo fixed, it compiles :-).  Also, hitting the refresh button my keyboard no longer results in an empty browser window (in other words, 330938 appears to be fixed by it).

The Bad: The Back/Forward buttons on both my keyboard and mouse still don't work.

I'm going out of town for the Thanksgiving holiday until Sunday the 26th, so I won't be able to test any further patches until I get back.

Comment 35

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

(In reply to comment #33)
> Olli - your patch fails to compile quite spectacularly on nsWindow.cpp line
> 1166. I'm assuming you meant "AssignLiteral" and not "AssingLiteral" ;-). 


Copy-pasting a typo isn't always a good idea :)
Thanks for testing.

Comment 36

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

(In reply to comment #23)
> Ok, here's the deal: the whole process starts at nsWindow.cpp where
> DispatchAppCommandEvent() is called and ends up in chrome's event state
> manager.

So which ESM is this? Document->GetShellAt(XXX)->PresContext()->EventStateManager(), where Document is the chrome document ?
Hmm, I wonder if presShell->GetCurrentEventFrame() is null...
Well, anyway, need to hack something so that I can test this using Linux too.

Comment 37

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

I this still a problem now that Bug 130078 is backed out?
I'll anyway do a patch for this, so that 'back', 'forward' etc commands can be handled in chrome.

Comment 38

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

Attachment: better patch, may even compile

Feel free to test ;)

Comment 39

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

The immediate bug is fixed, but we still need this.

Comment 40

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

Comment on attachment 246369 [details] [diff] [review]
better patch, may even compile

Ere, want to try this and check the windows part of the code?

Comment 41

[Tom]

the foward and back buttons are working on a microsoft desktop elite optical bluetooth set!

Comment 42

[Ere Maijala (slow)]

Comment on attachment 246369 [details] [diff] [review]
better patch, may even compile

Yes, looks good and the Windows part seems fine.
r=emaijala for Windows part.

One thing is a bit unclear to me: are these now just "command" events or "appcommand" events? You changed the Windows part to be just command, but the UI part is using appcommand.

Comment 43

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

The 'event class' is nsCommandEvent, type of the event is "AppCommand".
Later there might be some other types too, for example
"AudioCommand", where .command could be then "Play", "Stop" etc.

Comment 44

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

Comment on attachment 246369 [details] [diff] [review]
better patch, may even compile

Roc, what do you think about this?

Comment 45

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

Why not make nsCommandEvent::command an nsIAtom*?

This exposes a new DOM API, and one that's actually accessible to Web authors, right? Is there a way we can make sure it's not visible to Web authors?

Comment 46

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

(In reply to comment #45)
> Why not make nsCommandEvent::command an nsIAtom*?

Perhaps better, yes.

> 
> This exposes a new DOM API, and one that's actually accessible to Web authors,
> right? Is there a way we can make sure it's not visible to Web authors?
> 

Why shouldn't it be visible to web authors. Chrome handles only 
trusted events which web content can't dispatch. And if .stopPropagation is used during capture phase in chrome window, 
the event doesn't propagate anywhere else.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Olli, I'll try patch 246369 on my local tree with 130078 still applied to it to make sure that this bug is truly fixed for when that gets checked into the trunk again after 1.9a1 ships once I get home again on Sunday.

Comment 48

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

> Why shouldn't it be visible to web authors.

No particular reason except that it's a unilateral extension to the DOM API which we aren't marking as experimental.

Comment 49

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

Well, we have also for example nsIDOMPopupBlockedEvent.
Perhaps we should add @unfrozen or something to those interfaces.

Comment 50

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

> Well, we have also for example nsIDOMPopupBlockedEvent.

True.

> Perhaps we should add @unfrozen or something to those interfaces.

That wouldn't help.

Let's go ahead with the nsIAtom* change.

Comment 51

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

Attachment: using nsIAtom as command

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Great news, Olli! Attachment 246468 [details] [diff] works beautifully on my tree with 130078 still applied to it :) Assuming it passes review, I think you've got a winner.

Comment 53

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

Attachment: for thunderbird bustage

Comment 54

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

Attachment: or this one

Will file a bug that WINNT 5.2 tb-win32-tbox Depend
uses winver 0x400.

Comment 55

[[e-mail]]

Surely this will lead to confusion with ns[IDOM]XULCommandEvent?

Comment 56

[Ryan VanderMeulen [:RyanVM]]

Whoops, sorry about that (and the bugspam). Probably shouldn't have resolved the bug when there's still unresolved issues in it :)

BTW, the original bug is verified fixed.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061201 Firefox/3.0a1

Comment 57

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 246468 [details] [diff] [review]
using nsIAtom as command

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

>+function HandleAppCommandEvent(evt)

>+  switch (evt.command) {

>+  case "Reload":
>+    BrowserReloadSkipCache();

Why BrowserReloadSkipCache() instead of BrowserReload()?

Comment 58

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

Just because of 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventStateManager.cpp&rev=1.672&mark=2481#2480

Comment 59

[Mattias Jiderhamn]

Any plans on backporting this to Firefox 2.0?
I can handly wait to make Firefox my default browser, but this is a show stopper.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

This bug was for a regression which only occurred on trunk (Minefield, 3.0 alpha) builds. If you're having issues with back/forward on a 2.0 build, please file a new bug for it.

Comment 61

[Mattias Jiderhamn]

(In reply to comment #60)
> This bug was for a regression which only occurred on trunk (Minefield, 3.0
> alpha) builds. If you're having issues with back/forward on a 2.0 build, please
> file a new bug for it.

Actually, bug #230717 refers this bug, claiming it would fix the problem of the Web/Home key not working properly (in 1.5 or 2.0).
So what does this mean?
Is bug #230717 not fixed by this issue?
Or will I have to wait for 3.0?

Comment 62

[Ryan VanderMeulen [:RyanVM]]

Seems to me that bug 230717 comment #10 makes it pretty clear that it's trunk only.
https://bugzilla.mozilla.org/show_bug.cgi?id=230717#c10

Smaug, is this bug at all feasible for some sort of Gecko 1.8.1 landing?

Comment 63

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

The fix for this bug was non-trivial and added new features etc, and bug 230717 is not critical, so I doubt there are any chances to get this to branch.