531810 - Right clicking on Google Maps broken after leaving Street View (Windows only)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[James]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2b4) Gecko/20091124 Firefox/3.6b4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2b4) Gecko/20091124 Firefox/3.6b4

Every time I leave StreetView on Google Maps, the right click menu that you would get on the map itself will not show up.  As long as I don't go into StreetView, it works just fine.

Reproducible: Always

Steps to Reproduce:
1. Load a map in Google Maps (use the URL I supplied above as an example).
2. Enter into StreetView via the top left of the map.
3. Now, exit StreetView by clicking the "x" inside of StreetView.
4. Attempt to right click anywhere on the map itself.
Actual Results:  
Once you right click in the map area, the map's special right click menu will not load at all.

Expected Results:  
The map's special right click menu should have worked right away when leaving StreetView.

There is a minor workaround to fix this, but it's an annoying one.

Once you come out of StreetView, if you right click outside of the map and then right click again on the map, the map's right click menu will start to work again.

Comment 1

[James]

Forgot to add this, but this doesn't happen in Firefox 3.5.5 as it works correctly in that version.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Confirmed with latest trunk on Windows Vista. 
This appears to be caused by one of the compositor changes around 21 July 2009 but that should be verified.

Comment 3

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

Seems to not be a problem on Linux. On Mac I never get a right-click menu under any circumstances. Probably Windows-only.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I see the problem as well on Windows, on 3.6b4.  (And it also works for me on Linux.)

Comment 5

[(no longer active)]

(In reply to comment #3)
> Seems to not be a problem on Linux. On Mac I never get a right-click menu under
> any circumstances. Probably Windows-only.

I get a context menu by right-clicking on Mac both before and after the street view, so WFM on Mac.

Comment 6

[Damon Sicore (:damons)]

Who owns this?

Comment 7

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

I suspect a focus problem, so Neil Deakin perhaps, but since I think he's swamped and I have a debug build ready for debugging this, I'll take it.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The regression range in nightly Windows mozilla-central builds gives this one-day pushlog range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2a58ffcd00c&tochange=02f8bf10f441
which includes the compositor part 1 landing.

Comment 9

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

It looks like the Street View plugin is run windowless.

Spy++ shows that the right-button click is going to the browser window OK.

Comment 10

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

We seem to be dispatching the DOM mouse event to the image element on right-click.

Comment 11

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

And then we're off into a nasty mess of obfuscated JS :-(

Comment 12

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

Definitely focus-related. If you trigger the bug, so right-click isn't working, but then transfer focus to something by pressing tab, shift-tab, or clicking in a textbox, the problem stops.

Comment 13

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

When the bug is not happening, setting a breakpoint on nsGenericElement::AppendChild, when I right-click it gets hit with a stack showing a few nested js_Invokes below nsGlobalWindow::RunTimeout. We're appending one DIV to another DIV.

When the bug is happening, that breakpoint gets hit with a different stack, where we're setting innerHTML. I see this stack in later hits when the bug is not happening, so I suspect the first nsGenericElement::AppendChild break when the bug is not happening is part of showing the menu.

I can't get DumpJSStack() to work from Visual Studio, so I can't tell what even the obfuscated JS is doing.

Comment 14

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

Aha! I got DumpJSStack() working (yay
https://developer.mozilla.org/En/Debugging_Mozilla_on_Windows_FAQ#Debugging_JavaScript
), and that showed me that the menu is shown in response to a DOM contextmenu event, not a click event.

A bit more debugging shows that when things are working, we dispatch the contextmenu event to the image element under the mouse, but when things aren't working, we dispatch it somewhere in XUL. I suspect a problem with nsEventStateManager.

Comment 15

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

OK, when things aren't working, it's because although the Streetview plugin is not visible, PluginHasFocus is returning true in nsWindow::ProcessMessage for the content window. So we try to the WM_CONTEXTMENU as a plugin event, and that doesn't handle it, so we then pass the WM_CONTEXTMENU to the DefWindowProc which forwards it to our parent window (a XUL window), which does handle it.

Clearly PluginHasFocus should be returning false here...

Comment 16

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

PluginHasFocus is returning true because nsIMEStateManager::OnChangeFocus simply isn't being called when you dismiss the Streetview.

BTW, we probably shouldn't be simply passing all WM_CONTEXTMENU events to the focused plugin, we should be checking the coordinates and only passing keyboard WM_CONTEXTMENU events directly, and synthesizing a WM_CONTEXTMENU event in nsPluginInstanceOwner::ProcessEvent for mouse-based events. But that's another story...

Comment 17

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

Attachment: fix

This fixes it. nsIMEStateManager::OnRemoveContent needs to call SetIMEState to actually update the IME state to reflect that no node is focused.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, isn't the same process needed in nsIMEStateManager::OnDestroyPresContext?
# but looks fine.

Comment 19

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

We probably do need something there too... Can you make a separate patch for that?

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #19)
> We probably do need something there too... Can you make a separate patch for
> that?

ok, but I found a bug in your patch. I merge your patch to my patch.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

We need null check of widget before we call SetIMEState.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

oops, fix a nit.

Comment 23

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

Masayuki, it seems to me that we could easily write a mochitest here by creating a windowless instance of the test plugin, focusing it, removing it from the DOM, and then testing nsIDOMWindowUtils::IMEStatus. Can you do that?

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #23)
> Masayuki, it seems to me that we could easily write a mochitest here by
> creating a windowless instance of the test plugin, focusing it, removing it
> from the DOM, and then testing nsIDOMWindowUtils::IMEStatus. Can you do that?

Yeah, o.k.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch + tests

Roc:

Unfortunately, the tests cannot pass on Mac because nsChildView of Mac never returns the plug-in state. I'll fix it in another bug.

The new tests are tested only on Win and Linux.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/65c1582465ef

I'll land it later.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bde2af1feb1c