Closed Bug 239201 Opened 20 years ago Closed 19 years ago

Flash links not clickable in 1.7b, work in 1.6

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: fxper, Assigned: roc)

References

(
URL
)

Details

(Keywords: qawanted, top100, Whiteboard: patch on the trunk needs testing)

Attachments

(3 files)

screen shot of blank areas
99.38 KB, image/png
Details
fix
2.14 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
Screenshot of painting problems (incorrect z-ordering)
134.67 KB, image/jpeg
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113
Build Identifier: http://ftp.mozilla.org/pub/mozilla.org/mozilla/releases/mozilla1.7b/mozilla-win32-1.7b-stub-installer.exe

I was using 1.6 and browsing this page:
http://www.cnet.com/4520-7382_1-5110181-1.html?tag=dh.lrh.nav

And decided to try out 1.7b, and went back to the page. Upon return I found that
I was unable to click the images in the "MORE TOP PICKS" section. 

I then installed 1.6 again, and that fixed the problem again. 

Reproducible: Always
Steps to Reproduce:
1. go to http://www.cnet.com/4520-7382_1-5110181-1.html?tag=dh.lrh.nav with 1.7b
2. click a TV in the "more top picks" section
3. notice that you cannot click on them

Actual Results:  
nothing

Expected Results:  
gone to the link

Shockwave Flash

    File name: NPSWF32.dll
    Shockwave Flash 7.0 r14
Was the xpt file for flash properly installed with 1.7?  (You may need to copy
it over or something.)
Flags: blocking1.7?
Keywords: qawanted
This worksforme with Mozilla 1.7 beta on windows XP with Shockwave Flash 7.0 r14. 
Ok, I can reproduce this in Mozilla 1.7/winxp using Shockwave Flash 7.0 r19

Steps to reproduce:

1. have another window open
1. in a new window load page. ;-)
2. first do an alt-tab to hide the page with the flash, then alt tab back. note
that part of the flash does not redraw. 
3. click on the table radio
4. alt tab to hide page then alt tab to show it again. note that part of the 
   flash did not redraw. that part is the portion that does not activate as a 
   click.
5. mouse over far right edge of the far right radio (the flat looking one). Note
   the part that was not hidden in step 4 is clickable. When you mouse over it 
   the part of the flash that did not redisplay is shown again. 
6. the more top picks which were not redrawn are not clickable
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: top100
WFM WinMe mozilla 1.7RC1 - 'about:plugins' doesn't work with this ver of
mozilla, but NPSWF32.dll properties says "Shockwave Flash 6.0  r79"
Still does not work for me for the parts of "More Top Picks" where the flash
does repaint. Note that MSIE does not have this problem.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040424
Shockwave Flash 7.0 r19
jst, can you look at this for us? It seems like a pretty unpleasant regression
to ship in 1.7.
This actually doesn't look like a *regression* per se. Here's what's happening
on this site.

The main chunk of the site is a big hunk of flash content in the middle of the
page. Below that, there's a smaller piece of flash content (with the text
"advertisement" above it). Right above that lower piece of flash content, the
page places an iframe with visibility hidden, and that iframe is what shows
through sometimes as a white box. When you mouse over the small piece of flash
content (the advertisement) the site sets style.visibility="visible" on the
iframe above the ad and changes the src of the iframe from a blank image, to
another piece of flash content. When you mouse out of the add and/or iframe, the
site changes the src of the iframe back to the blank image and sets
style.visibility="hidden" on the iframe.

Now the problem is now that even though the iframe gets visibility "hidden",
it's still there, and sometimes shows up, sometimes doesn't, but even when it
doesn't, it still eats all the events in the area it occupies, and thus the
"select-like-thing" in the big piece of flash just above the ad never gets any
events in the area that's coverd by the hidden iframe.

I don't know why we don't see quite the same thing in Mozilla <=1.6, but it
appears like there's some other problem in older builds that prevents the iframe
from ever appearing, so I don't know if older builds were broken in some way and
didn't show the iframe at all, or if it did show up, but it happened to be
*below* the big piece of flash...

Roc, I'm afraid I'll have to pass this on to you too, it looks related to bug
242122, at least in some ways... And I see this same problem on Linux, and it
appears to be even worse there than on WinXP (the iframe always appears to show
on Linux, even if it's hidden).
Assignee: peterlubczynski-bugs → roc
The underlying problem is that plugins don't paint in our normal paint path, so
they don't obey our z-ordering, and when we have a transparent element (like an
hidden IFRAME) with a widget that covers the plugin, the view manager doesn't
know what to paint in that widget to make it look like the background shines
through.

We had a workaround that visibility:hidden IFRAMEs had their widgets hidden to
improve performance and avoid this particular kind of problem. That workaround
stopped working when I refactored the IFRAME frame hierarchy. The problem is
that now there is one frame, the nsSubDocumentFrame, which contains an inner
view which actually carries the widget. The inner view is not attached to any
particular frame (because it's sized to the content area of the IFRAME, which
doesn't correspond to the dimensions of any frame now). There's nothing to
synchronize the inner view's view properties with any frame style, so it never
gets hidden.

To fix this bug, we need to hide the inner view so its widget gets hidden. We
need to hide it in response to SyncFrameViewProperties on its container. A
rather hacky approach would be to have nsViewManager::SetViewVisibility
propagate visiblity changes to any children which are anonymous (no ClientData).
We really should have the invariant that the children of a hidden view are hidden...
Attached patch fix — — Splinter Review 
This fixes it.

It's basically what I said. In addition to propagating a parent view's
visibility change to any anonymous view children, we also make sure that the
IFRAME's anonymous child view is initialized with the correct inherited
visibility.
Comment on attachment 149089 [details] [diff] [review]
fix

The fix will be a bit risky. Dunno if we want to push this into 1.7 final.
Attachment #149089 - Flags: superreview?(dbaron)
Attachment #149089 - Flags: review?(dbaron)
this totally fixes the page on Linux, BTW.
Awesome, thanks for looking, roc! I tried the patch on WinXP, and it fixes the
problem there as well. I now don't ever see the hidden iframe, but I'm fairly
sure that's an unrelated issue.
> I now don't ever see the hidden iframe

what do you mean? That sounds bad
Yeah, it's kinda bad, but that's what I mostly see w/o this patch as well.
Whether I ever see the iframe on WinXP seems to depend on unknown things. I've
never seen it work reliably, only rarely and randomly at best. I'll play some
more and report back... Are you saying that with this patch the iframe is
shown/hidden as expected when you mouse in/out of the add at the bottom?
Ok, so something's definately wrong on WinXP with this patch. I studied this a
bit more and what seems to happen is... When I mouse over the ad, an all-white
window (~300x200px, the iframe) flashes above the ad only to disappear. If I
cover the whole browser window with another window and expose the whole window
at once (i.e. Alt+Tab back to the window with the flash in it) then the white
window appears (regardless of where the mouse happens to be), and stays there
until I move the mouse over it and out again. I've only ever seen the actual
flash load into the iframe at random times, and I'm now unable to reproduce that
at all (w or w/o your change).

With this patch, none of the above happens, I never see a white window appear,
and I never see the hidden iframe. But the problem with events being eaten by
the hidden window is fixed by this patch.
If you move the mouse in from the right, slowly, you should be able to trigger
the popup just before the mouse moves into the Flash area. The problem is that
once the mouse is over the Flash area, Flash eats all the mouse events so we
never trigger the mouseover DOM event. Since the Flash area covers almost all of
the DIV it's in, you're not likely to ever be over the DIV but not the Flash
area, so I'm not sure how this is supposed to work.
w/o your patch I sortof see that, I see the blank white window flash when I move
in slowly from the right or from the top, and when that happens, the status bar
changes to indicate that we start loading some content somewhere. But with your
change, I see now window flash or stay, nor do I see any change in the status
bar, so I'm guessing no iframe is ever created such that it even starts to load
its content.

So I guess there's more to this, at least on windows.
Can you try it on Linux just to make sure we're on the same page?
Flags: blocking1.7? → blocking1.7+
Whiteboard: investigating possible fix
jst says that this patch is good. Just need review.
Whiteboard: investigating possible fix → investigating possible fix. mail sent to dbaron asking for reviews.
Comment on attachment 149089 [details] [diff] [review]
fix

>+    // Any child views not associated with frames might not get their visibility
>+    // updated, so propagate our visibility to them. This is important because
>+    // hidden views should have all hidden children.

Hmmm.  I was under the impression that this wasn't the case, but rather that
making a view hidden made all its descendants hidden.  I do have some local
changes in nsViewManager.cpp that I made after a discussion with roc that did
things like:

+static PRBool IsViewVisible(nsView *view)
+{
+  for (; view; view = view->GetParent())
+    if (view->GetVisibility() == nsViewVisibility_kHide)
+      return PR_FALSE;
+  return PR_TRUE;
+}
+

-      if (nsViewVisibility_kHide != child->GetVisibility())
+      if (IsViewVisible(child))

etc.

Which way is it supposed to be?
Right now the only views that are marked hidden are leaves, except for the views
which contain subdocuments, which can be hidden even though the views in the
subdocument are visible. So you're right, that comment is incorrect. It should
read something like this:

// Any child views not associated with frames might not get their visibility
// updated by layout, so propagate our visibility to them. This is important
// because such a child view might have a widget which we want to hide for
// performance and for correctness with plugins.
Comment on attachment 149089 [details] [diff] [review]
fix

OK, but what says that there aren't views somewhere deep in that subtree that
should be hidden either way that this should be causing to be shown?
Comment on attachment 149089 [details] [diff] [review]
fix

OK, but what says that there aren't views somewhere deep in that subtree that
should be hidden either way that this should be causing to be shown?
I'm not sure what you're asking.

A hidden view can't have an arbitrary deep subtree under it. Either it has no
children, or in the case of FRAME/IFRAME, it has one child which has its
subdocument's entire tree of views under it. In the latter case the hierarchy
always looks like this:

nsSubDocumentFrame <-> view
                         |
                    anonymous view
                         |   <--------- document boundary
   nsViewportFrame <-> rootview
                        /  \
                      ...  ...

A change in the visibility of the nsSubDocumentFrame's view can only be
propagated to the anonymous view and no further, because its child will have a
frame --- the viewport frame for the subdocument. Even if that frame hasn't been
constructed yet, construction of that frame will eventually happen and that will
reset the visibility of the root view to the correct value.

The more general argument is that this code can never change the visibility of a
view that has a frame associated with it. The only thing it could break is if
layout is twiddling the visibility of frameless views directly, and wants them
to be different from their parents, and I don't think we do that. (In the edge
case where a root view exists but hasn't had its viewport frame constructed yet,
construction of the viewport frame should reset the properties of the view.)
Attachment #149089 - Flags: superreview?(dbaron)
Attachment #149089 - Flags: superreview+
Attachment #149089 - Flags: review?(dbaron)
Attachment #149089 - Flags: review+
Checked in that patch. But there's more to do here, on Windows at least.
is the patch ready for the 1.7 branch?
It needs testing, especially on Windows. Providing it gets that testing I think
it's reasonable for the branch.
Whiteboard: investigating possible fix. mail sent to dbaron asking for reviews. → patch on the trunk needs testing
I still see odd paint problems with a trunk firefox build from today on WinXP.
While it seems like things are maybe a bit better after this change, this
appears to not be fixed yet on WinXP.
Can you get it into that state and then use Spy++ to see what the configuration
of the native windows is?
Do you think this is ready to land on the branch?  jst seems to think this fixes
the problem on Linux but doesn't help much on Windows.  However, it seems like,
given the amount of time left for 1.7, this might not be worth the risk of
taking on the branch.  It doesn't seem like the problem is very common.
Flags: blocking1.7+ → blocking1.7-
This will never make it to the branch.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: