Closed Bug 531810 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect)

1.9.2 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta5-fixed

People

(Reporter: rickmastfan67, Assigned: masayuki)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
Forgot to add this, but this doesn't happen in Firefox 3.5.5 as it works correctly in that version.
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.
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → 1.9.2 Branch
Flags: blocking1.9.2?
Seems to not be a problem on Linux. On Mac I never get a right-click menu under any circumstances. Probably Windows-only.
I see the problem as well on Windows, on 3.6b4.  (And it also works for me on Linux.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Right clicking on Google Maps broken after leaving StreetView. → Right clicking on Google Maps broken after leaving Street View (Windows only)
Flags: blocking1.9.2? → blocking1.9.2+
(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.
Who owns this?
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.
Assignee: nobody → roc
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.
It looks like the Street View plugin is run windowless.

Spy++ shows that the right-button click is going to the browser window OK.
We seem to be dispatching the DOM mouse event to the image element on right-click.
And then we're off into a nasty mess of obfuscated JS :-(
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.
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.
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.
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...
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...
Attached patch fix (obsolete) — Splinter Review
This fixes it. nsIMEStateManager::OnRemoveContent needs to call SetIMEState to actually update the IME state to reflect that no node is focused.
Attachment #415564 - Flags: review?
Attachment #415564 - Flags: review?(masayuki) → review+
Whiteboard: [needs review] → [needs landing]
Ah, isn't the same process needed in nsIMEStateManager::OnDestroyPresContext?
# but looks fine.
We probably do need something there too... Can you make a separate patch for that?
(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.
Attached patch Patch (obsolete) — Splinter Review
We need null check of widget before we call SetIMEState.
Attachment #415564 - Attachment is obsolete: true
Attachment #415570 - Flags: review?(roc)
Whiteboard: [needs landing] → [needs review]
Attached patch Patch (obsolete) — Splinter Review
oops, fix a nit.
Attachment #415570 - Attachment is obsolete: true
Attachment #415571 - Flags: review?(roc)
Attachment #415570 - Flags: review?(roc)
Whiteboard: [needs review] → [needs landing]
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?
(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.
Attached patch Patch + testsSplinter Review
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.
Assignee: roc → masayuki
Attachment #415571 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #415858 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/65c1582465ef

I'll land it later.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 192 landing]
Whiteboard: [needs 192 landing]
You need to log in before you can comment on or make changes to this bug.