520462 - No wheel scrolling anymore after playing embedded movie.

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Ria Klaassen (not reading all bugmail)]

STR:

- Go to http://zaplog.nl/zaplog/article/draadloos_netwerksignalen_laten_je_door_muren_kijken#rss 
- Scroll down and start flash video
- Click Stop and click outside movie
- Attempt to scroll down with mouse wheel

Actual result: page doesn't scroll anymore.

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8cbc47eee659&tochange=67e02399d88a

Maybe caused by Bug 509693 ?

Comment 1

[[not reading bugmail]]

Confirming it happens on other flash based pages like www.youtube.com also.

Comment 3

[[not reading bugmail]]

Should also note that after you switch tabs, scroll wheel is functional again.

Comment 4

[Paul-Kenji Cahier]

Another symptom is that, on youtube for example, all keyboard input is stolen.
(ctrl+t,ctrl+n, etc do not work)

And as reported in my duplicate, clicking on any text field on the page gives back the focus to the browser. It's just clicking in any non-input control that does not return control(when it should, clicking on the page is a clear sign of wanting control back to the browser).

Comment 5

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

Is this Flash-only?

Comment 6

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

On Windows WM_MOUSEWHEEL events are directed to the focused window. A window that isn't handling the WM_MOUSEWHEEL event should call DefWindowProc to forward the message to its parent window, but Flash isn't doing that. However, the real problem here seems to be that clicking outside the Flash plugin isn't giving focus to the browser window.

Comment 7

[Ria Klaassen (not reading all bugmail)]

Yes, the cursor remains in the search field of the embedded movie if you focus the page, should have mentioned that in comment 0. 
The URL is gone for some reason, maybe someone can find a new one?

Comment 8

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

In nsFocusManager::Blur we have code
    // if an object/plug-in is being blurred, move the system focus to the
    // parent window, otherwise events will still get fired at the plugin.
But when we click outside the plugin, mFocusedContent is null, and the call to SetFocus on our content window doesn't happen.

I assume that Flash calls ::SetFocus on its own window to take focus when we click on it. But how does nsFocusManager know to update mFocusedContent? There are a few ways we could detect that we're losing focus to a plugin, for example by handling WM_KILLFOCUS and observing that the window that's becoming focused belongs to a plugin, and working backwards from that to the plugin element. But we don't do that. We certainly don't get a call to nsFocusManager::Blur. In fact if I focus a text input and then click in the plugin, focus is in the plugin but we still have a blinking caret in the text field since we don't know about the focus change.

I guess this used to work somehow, but I can't see how, so I don't know how any of my changes broke it.

I'll make a build before my changes and see how that works, but Neil, it would be helpful if you could just explain how it's supposed to work :-).

Comment 9

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

Er, Neil, see comment #8.

Comment 10

[Ria Klaassen (not reading all bugmail)]

The bug is NOT on this page, although it has an embedded movie: http://brabosh.com/2009/10/01/de-ondergang-en-verdrijving-van-de-joden-van-lybie/

Comment 11

[Paul-Kenji Cahier]

I would assume that this is because the flash in it does not intercept the events. But I think this should be irrelevant to the issue, as the issue is that focus isn't passed back to the browser when clicking anywhere else on the page.

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to comment #8)
> I assume that Flash calls ::SetFocus on its own window to take focus when we
> click on it.
*We* give the focus to the plug-in here by firing NS_PLUGIN_ACTIVATE event.
http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/src/nsPluginNativeWindowWin.cpp#306
Then nsObjectFrame will catch the event and call the focus manager.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#1832

Comment 13

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

Ah, thanks! Unfortunately my Windows build is already half-clobbered so I'll have to resume debugging this in the morning.

Comment 14

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

Ria, are you sure that bug 513299 wasn't in the regression window? That also landed on October 1 and changed the handling of focus events...

Comment 15

[Ria Klaassen (not reading all bugmail)]

Robert - that is possible. The regression window reflects only the time span when the original URL stopped working properly, but the real reason may be further in the past.

Comment 16

[[not reading bugmail]]

Also note: this is not bug 483136 or bug 78414.  Where those two bugs are almost the opposite problem as this: user cannot scroll wheel pages or execute keyboard shortcuts while plugins have hover or focus and the user has to click on the page or reload/switch tabs for it to work again.

Comment 17

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

This is a regression from bug 508495.

Comment 18

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

The problem is that in bug 508495 we start associating the plugin's widget with an "inner view" which is not directly attached to the nsObjectFrame. Then in nsPresShell::HandleEvent, an NS_PLUGIN_ACTIVATE event can't find a frame for the view, and this is not one of the event types that nsPresShell::HandleEvent will search up the view hierarchy to find a frame for, so it gets dropped on the floor. Adding NS_PLUGIN_ACTIVATE as an event type that it will search up the view hierarchy for fixes the bug.

Comment 19

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

Attachment: fix

This is the fix. It's trivial and safe. I need someone to review it, but I can't find anyone...

Comment 20

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

I think this is clearly a blocker and deserves to be P1...

Comment 21

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

Comment on attachment 404990 [details] [diff] [review]
fix

Mats, can you review this too?

Comment 22

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

Landed the fix on trunk and 1.9.2:
http://hg.mozilla.org/mozilla-central/rev/ccc6dad9010c
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d53ec8e6a079
Leaving open for the test, which I think I have nearly working..

Comment 23

[[not reading bugmail]]

Fixed with latest trunk nightly.

Comment 24

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

Attachment: test

This patch implements nsWindow::SynthesizeNativeMouseEvent for Windows and adds a test for plugin focus transfer on mouse clicks that uses that facility (which Markus originally added to test some Mac stuff). The test passes on Mac as well. The test won't run on X since SynthesizeNativeMouseEvent isn't implemented there, it should just bail out with a todo.

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 405206 [details] [diff] [review]
test

Looks like ce does support that api call, not sure if we run these types of tests though on that platform.

Comment 26

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

Alright, I'll remove the #ifdefs from that section.

Comment 27

[Damon Sicore (:damons)]

This is next in line for landing, right after smichaud's OJI patches, right?

Comment 28

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

This was only open for the landing of the test. The actual bug is fixed on trunk and branch. I'll close this to avoid confusion.

Comment 29

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

Checked in test:
http://hg.mozilla.org/mozilla-central/rev/4c52df6ba5ad