271442 - Flash element behaves weird if inside a floated element with overflow:auto

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Anne (:annevk)]

Without 'overflow:auto' you can click the link, with 'overflow:auto' you can't.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This worksforme in a 2004-12-05-09 Linux SeaMonkey trunk build...  Is this
windows-only?  Or am I doing something wrong?  What are the _exact_ steps to
reproduce

Comment 2

[Anne (:annevk)]

I tested this with |Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5)
Gecko/20041123| on Windows. I'll download a new build later to test again.

Steps are quite simple:
 1. Make sure you have Flash
 2. Open the testcase
 3. Check if you can click on every (Flash) link on the page

Results:
 *  Only one link is clickable

Expected results:
 *  All three links are clickable

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, those steps work for me on Linux (all links are clickable).  Please don't
file bugs as "All" unless they've actually been demonstrated on all platforms
(by testing or code inspection).

Comment 4

[Mark Wubben]

The bug does not occur when setting the wmode attribute on the embed element to
"window" or "". Similar (in that they use this solution) bug reports are:
  * bug{247280}
  * bug{255636}
  * bug{266933}
  * bug{267599}

Comment 5

[Anne (:annevk)]

Mark, could you attach the testcase to this bug?

Comment 6

[Anne (:annevk)]

Making possible related bugs clickable: bug 247280, bug 255636, bug 266933 and
bug 267599 and restoring the testcase URI.

Comment 7

[graham sanderson]

We are also hitting this bug and need it fixed ASAP. Note, I think bugs 318108 and 246340 are duplicates of this bug.

To summarize the problem as shown by the testcase:

On Windows, windowless plugins (read Flash with wmode=transparent or wmode=opaque) get the wrong coordinates for mouse events if that plugin is contained within a scrollable DIV on the page.

This prohibits the use of flash mixed with overlayed DHTML content in our app (where the main document is in a scrollable area of the page).

The testcase in this issue shows three instances of the same flash movie, and in each case moving over the link should turn it blue and give you a hand cursor.

Comment 8

[graham sanderson]

Could someone assign this bug to me; this is my first time fixing a bug, and don't seem to have the authority.

Basically the problem is this: (I'm CCing peterl who fixed a related problem and surely knows more, and Ali Berger who reported that issue)

In windowless mode, the mouse events forwarded to the plugin are relative to the nsWindow that contains the plugin. On Windows a WM_WINDOWPOSCHANGED message is sent to the plugin if the windows visible region change is detected during paint.

The plugin can subtract the origin given in WM_WINDOWPOSCHANGED from the mouse events to get coordinates relative to the plugin. 

Some plugins (especially flash for rollover/rollout etc) also want to determine the mouse coordinates at other times, and generally seem to get the mouse position on the screen, convert it to the window's local coordinates using ScreenToClient passing the HWND retrieved from getValue(NPNVnetscapeWindow). These coordinates can then be re-origined by the same amount to get the correct mouse position.

----

The problem with the current implementation is that the coordinates sent in the WM_WINDOWPOSCHANGED are relative to the document window and not the enclosing window of the plugin (which are not the same for example if we're in a scrollable DIV). The upshot is that the plugin subtracts too much off the coordinates when adjusting (namely the coordinates are wrong by -X, -Y where X,Y is the origin of the enclosing window relative to the document window).

I considered two possible solutions:

1) Return the HWND of the innermost enclosing window for NPNVnetscapeWindow (this is arguably what the documentation implies - this is really the window you're drawing in) and adjust the WM_WINDOWPOSCHANGED call to work relative to this window (not the document window)
2) Leave the code as it is (i.e. coordinates relative to document window), but translate all coordinates by X,Y before passing them to the windowless plugin

In either case this only effects the windowless plugin on Windows code paths.

Option 2) seemed like the less risky option (in case someone was making illicit use of the HWND from NPNVnetscapeWindow and would care if it wasn't the document window), or if someone used the contents of WM_WINDOWPOSCHANGED for anything other than mouse event correction - Note; flash doesn't seem to.

This option caused mouse events forwarded to Flash to be interpreted correctly, but for some reason Flash's internal mouse calculations don't work with this method.

Option 1) DOES fix the problem correctly at least for Flash.

I would therefore propose that option 1) should be implemented but with NO changes to the current behaviour unless the plugin is:

a) windowless
b) on Windows
c) currently located such that mouse coordinates are incorrect (as determined by whether a coordinate correction would have been necessary as per option 2).

This limits the scope of the fix to pages which must currently be broken anyway, so reduces the risk a lot.

Comment 9

[Peter Kasting]

I doubt very much this can make it into Fx2.  I'd suggest fixing the problem "correctly" on trunk, and if it can be done fast and well, then try and consider how to make a scaled-back patch suitable for branch.

I would not suggest coding the "scaled-down" version from the start.  Let's just do the right thing.

Comment 10

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

Let's just do option 1. Graham, do you have a patch?

Comment 11

[graham sanderson]

I have a patch for 1.8, I haven't changed and tested on the trunk yet, since I need to get and install the newer Visual C++.
Do you want me to post the 1.8 patch?

Comment 12

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

Sure, that'd be useful.

Comment 13

[graham sanderson]

Attachment: Always use coordinate system relative to window containing a windowless plugin

This is the patch against the 1.8 branch

Comment 14

[Martijn Wargers (dead)]

Attachment: patch, ported to trunk

Thanks for the patch!
I've ported it to trunk in case that's useful.

Comment 15

[graham sanderson]

Excellent, thanks Martijn

Comment 16

[Martijn Wargers (dead)]

Comment on attachment 230114 [details] [diff] [review]
patch, ported to trunk

First, the trunk patch needs to be reviewed (the patch works great, afaics).

Comment 17

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

Comment on attachment 230114 [details] [diff] [review]
patch, ported to trunk

Looks good!!

GetNearestViewWithWindow is not needed. Just call GetWindow() on the frame.

+           pluginEvent.event = 0x0047;

Why did you replace WM_WINDOWPOSCHANGED with its value?

Comment 18

[graham sanderson]

In the (unpatched) 1.8 version of the code the numeric value is used for WM_WINDOWPOSHCANGED, presumably because windows headers aren't included.

Indeed, we should use the #define on the trunk; I'll try compiling with it on 1.8 too to see if it is actually included now, and remove the duplicate function

Comment 19

[graham sanderson]

I have made a new patch for the trunk, which looks OK; 

however I have noticed that on the trunk (with or without the patch) actually clicking on the links in the Flash does nothing (it looks like the mouse coordinates for mouse-down are off). 

This would seem to be a somewhat unrelated bug in the trunk, but I'm gonna port my new patch for the existing issue back to 1.8 to verify everything works there, before posting both patches, and also start looking into what the deal with mouse-down is on the trunk.

Comment 20

[graham sanderson]

Attachment: Another testcase for this issue; including windowed and windowless versions of the same plugin

Here is another (albeit) ugly testcase for this issue.
Note that in Fx 1.5 and 2.0 only the top-left plugin had issues.
In the trunk clicking the button on the 2nd and 3rd plugins has no effect (or rather it has the wrong effect; flash thinks you have clicked OUTSIDE the button).

Comment 21

[graham sanderson]

Attachment: trunk patch incorporating robert's changes

Comment 22

[graham sanderson]

Attachment: 1.8.1 patch

Comment 23

[Martijn Wargers (dead)]

(In reply to comment #19)
> however I have noticed that on the trunk (with or without the patch) actually
> clicking on the links in the Flash does nothing (it looks like the mouse
> coordinates for mouse-down are off). 

I think you're seeing bug 311622 here, which is trunk only and unrelated to your patch.

Comment 24

[Martijn Wargers (dead)]

Checking in nsObjectFrame.cpp;
/cvsroot/mozilla/layout/generic/nsObjectFrame.cpp,v  <--  nsObjectFrame.cpp
new revision: 1.565; previous revision: 1.564
done

Checked in on trunk.
Thanks a ton for the patch, Graham!
In a few days, please nominate the 1.8.1 patch for approval1.8.1, if there are no known regression from the trunk.

Comment 25

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

roc, could you comment about the risk of this for the 1.8 branch?

Comment 26

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

I think it's low risk. This should only affect scenarios that were already pretty broken.

Comment 27

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

Comment on attachment 230831 [details] [diff] [review]
1.8.1 patch

a=dbaron on behalf of drivers.  Please land on MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword once you have done so.

Comment 28

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

fixed on 1.8.1 branch.

Comment 29

[Marian POPESCU]

Something is wrong with it:
on http://stuff.novemberborn.net/sifr/overflow-bug.html, try to right click on the flash objects!
The browser goes down without a warning.
There is also no talkback !!
What kind of fix is this?

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060808 BonEcho/2.0b1 ID:2006080804 with a fresh new profile, no extensions installed.
Shockwave Flash 7.0 r63

Comment 30

[Marian POPESCU]

The relevant output from strace:
><snip><
The program 'Gecko' received an X Window System error.
This probably reflects a bug in the program.
The error was 'BadValue (integer parameter out of range for operation)'.
  (Details: serial 438 error_code 2 request_code 12 minor_code 0)
  (Note to programmers: normally, X errors are reported asynchronously;
   that is, you will receive the error a while after causing it.
   To debug your program, run it with the --sync command line
   option to change this behavior. You can then get a meaningful
   backtrace from your debugger if you break on the gdk_x_error() function.)
[{WIFEXITED(s) && WEXITSTATUS(s) == 1}], 0) = 15434
rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
><snip><

Comment 31

[graham sanderson]

The patch fixes a Win32 bug on Win32 only codepaths; Is this a regression on Linux?

Comment 32

[Marian POPESCU]

(In reply to comment #31)
> The patch fixes a Win32 bug on Win32 only codepaths; Is this a regression on
> Linux?
> 

I guess it is !! And it's definitely new: with the previous nightly I could right click any flash object ...

Comment 33

[Martijn Wargers (dead)]

(In reply to comment #32) 
> I guess it is !! And it's definitely new: with the previous nightly I could
> right click any flash object ...

Marian, please file a new bug (and mention the bug number here).

Comment 34

[Marian POPESCU]

(In reply to comment #33)
> (In reply to comment #32) 
> > I guess it is !! And it's definitely new: with the previous nightly I could
> > right click any flash object ...
> 
> Marian, please file a new bug (and mention the bug number here).
> 

Ok, I filled bug 347895.

Comment 35

[Karl Tomlinson (:karlt)]

Nothing left to do.
Marking fixed.