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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: annevk, Assigned: gsanderson)

Tracking

({fixed1.8.1, testcase})

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
Without 'overflow:auto' you can click the link, with 'overflow:auto' you can't.
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
(Reporter)

Comment 2

15 years ago
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
OS: All → Windows XP
Hardware: All → PC
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

15 years ago
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}

(Reporter)

Comment 5

14 years ago
Mark, could you attach the testcase to this bug?
(Reporter)

Comment 6

14 years ago
Making possible related bugs clickable: bug 247280, bug 255636, bug 266933 and
bug 267599 and restoring the testcase URI.
(Assignee)

Comment 7

13 years ago
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.
(Assignee)

Comment 8

13 years ago
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.
Assignee: nobody → gsanderson

Comment 9

13 years ago
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.
Let's just do option 1. Graham, do you have a patch?
(Assignee)

Comment 11

13 years ago
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?
(Assignee)

Comment 13

13 years ago
This is the patch against the 1.8 branch
Attachment #230080 - Flags: review?(roc)
Posted patch patch, ported to trunk (obsolete) — Splinter Review
Thanks for the patch!
I've ported it to trunk in case that's useful.
(Assignee)

Comment 15

13 years ago
Excellent, thanks Martijn
(Assignee)

Updated

13 years ago
Attachment #230080 - Flags: superreview?
Blocks: 318108
(Assignee)

Updated

13 years ago
Blocks: 246340
No longer blocks: 318108
Blocks: 342096
Comment on attachment 230114 [details] [diff] [review]
patch, ported to trunk

First, the trunk patch needs to be reviewed (the patch works great, afaics).
Attachment #230114 - Flags: review?(roc)
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?
(Assignee)

Comment 18

13 years ago
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

13 years ago
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.
(Assignee)

Comment 20

13 years ago
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 22

13 years ago
Posted patch 1.8.1 patchSplinter Review
(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.
Attachment #230114 - Attachment is obsolete: true
Attachment #230114 - Flags: review?(roc)
Attachment #230080 - Attachment is obsolete: true
Attachment #230080 - Flags: superreview?
Attachment #230080 - Flags: review?(roc)
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.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Attachment #230831 - Flags: approval1.8.1?
roc, could you comment about the risk of this for the 1.8 branch?
I think it's low risk. This should only affect scenarios that were already pretty broken.
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.
Attachment #230831 - Flags: approval1.8.1? → approval1.8.1+

Comment 29

13 years ago
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

13 years ago
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><
(Assignee)

Comment 31

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

Comment 32

13 years ago
(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 ...
(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

13 years ago
(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.

Updated

13 years ago
Blocks: 347895
No longer blocks: 347895
Blocks: 318108
Depends on: 357736
Nothing left to do.
Marking fixed.
You need to log in before you can comment on or make changes to this bug.