Bug 3778 - Plugins not occluded by fixed position objects with higher z-index
Summary: Plugins not occluded by fixed position objects with higher z-index
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Robert Hogan
URL: http://www.sonicyouthmedia.com/alt-ma...
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2005-06-30 14:07 PDT by chris habib
Modified: 2013-09-01 10:54 PDT (History)
13 users (show)

See Also:


Attachments
Reduction of the problem (1.08 KB, text/xhtml)
2005-07-04 12:19 PDT, David Storey
no flags Details
Related Reduction (1.22 KB, text/html)
2006-04-23 17:43 PDT, Seth A. Roby
no flags Details
Patch (14.88 KB, patch)
2011-07-16 13:13 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (20.15 KB, patch)
2011-08-04 12:52 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
rebase prev patch to HEAD (19.21 KB, patch)
2012-11-22 00:11 PST, Matt Falkenhagen
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris habib 2005-06-30 14:07:28 PDT
on the right side of the page (tour journal 2005: turkey) if you scroll in
safari, the ebedded quicktime renders above the page elements at the top of the
page while the text correctly hides beneath it. this bug does not exist in
firefox or IE on os x or windows.
Comment 1 Oliver Hunt 2005-06-30 23:20:40 PDT
This is reproducable in current ToT
Comment 2 Joost de Valk (AlthA) 2005-07-01 05:35:19 PDT
it's the left side of the page here, but i can reproduce it :). confirming.
Comment 3 David Storey 2005-07-04 12:19:26 PDT
Created attachment 2794 [details]
Reduction of the problem

Text, images and the movies control bar seem to work correctly, it's just the
movie window itself
Comment 4 Alice Liu 2005-11-17 18:47:04 PST
<rdar://problem/4338918>
Comment 5 Seth A. Roby 2006-04-23 17:43:31 PDT
Created attachment 7930 [details]
Related Reduction

This is a very similar case where the QuickTime movie overlaps a fixed area that it should scroll under. It appears that QuickTime fails to properly inherit the z-index.
Comment 6 river 2006-12-10 14:02:44 PST
the same thing happens with flash movies. when they are positioned in a div with scrolling overflow the browser continues to render the flash element once it's outside of the div and should not be visible.

http://dev.exit3a.com/main.html
Comment 7 Robert Hogan 2011-07-16 13:13:55 PDT
Created attachment 101095 [details]
Patch
Comment 8 Robert Hogan 2011-07-16 13:20:34 PDT
A fixed position element should occlude a plugin if it has a higher index. Though firefox and opera both have some odd rules for this. The fixed position element will only occlude the plugin, regardless of z index, if:

 - It has a non-transparent background
 - It's not a form control, e.g. an input element

I cannot find any source to explain this behaviour so instead of mimicking it I've implemented what seems the correct thing to do in the absence of a spec - allow fixed position elements to always occlude a plugin if they have a higher z index. If the z-index is equal, the plugin will also get occluded - based on the fact that the use-case of fixed position elements is to keep them always in view.
Comment 9 Robert Hogan 2011-07-16 13:22:01 PDT
(In reply to comment #7)
> Created an attachment (id=101095) [details]
> Patch

Note that IFrameShimSupport needs to be changed to something like PluginOcclusions - but I did not rename in this patch as it would obscure the changes to the file in the patch.
Comment 10 Simon Fraser (smfr) 2011-07-18 10:48:28 PDT
Comment on attachment 101095 [details]
Patch

The code should be walking the RenderLayer z-order tree to determine stacking, rather than trying to do it itself.
Comment 11 Robert Hogan 2011-08-04 12:52:32 PDT
Created attachment 102961 [details]
Patch
Comment 12 Robert Hogan 2011-08-08 12:19:26 PDT
(In reply to comment #10)
> (From update of attachment 101095 [details])
> The code should be walking the RenderLayer z-order tree to determine stacking, rather than trying to do it itself.

Hi Simon,

I've taken your advice and use the RenderLayer to implement occluding plugins by all elements, be they iframes or otherwise. It results in substantially less code. Could you take another look when you get the chance?

Darin, since this rewrites the iframe shim support, could you also take a look?
Thanks,
Robert
Comment 13 Simon Fraser (smfr) 2011-08-16 10:47:34 PDT
Comment on attachment 102961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102961&action=review

Is the RenderLayer tree guaranteed to be current when getPluginOcclusions() is called? Are those plugin methods called during layout?

> Source/WebCore/plugins/IFrameShimSupport.cpp:65
> +        layer->dirtyZOrderLists();

Why are you dirtying the z-order lists here?

> Source/WebCore/plugins/IFrameShimSupport.cpp:79
> +            if (layer == element->renderer()->enclosingLayer()) {

You should put element->renderer()->enclosingLayer() in a local variable. It's called to get the initial 'sc' too.

> Source/WebCore/plugins/IFrameShimSupport.cpp:100
> +            for (RenderLayer* child = layer->firstChild(); child; child = child->nextSibling())
> +                if (child->renderer()->hasBackground())
> +                    appendOcclusion(toRenderBox(child->renderer()), occlusions);

Why is this looking at layer children, not renderer children?
Comment 14 Robert Hogan 2011-08-16 11:32:39 PDT
(In reply to comment #13)
> (From update of attachment 102961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102961&action=review
> 
> Is the RenderLayer tree guaranteed to be current when getPluginOcclusions() is called? Are those plugin methods called during layout?

getPluginOcclusions() is called during layout, but I'm not sure that the RenderLayer tree is current when it is called. How can I ensure that it is apart from calling updateZOrderLists()?

> 
> > Source/WebCore/plugins/IFrameShimSupport.cpp:65
> > +        layer->dirtyZOrderLists();
> 
> Why are you dirtying the z-order lists here?

A leftover from when I was getting the hang of the zorder list.

> > Source/WebCore/plugins/IFrameShimSupport.cpp:100
> > +            for (RenderLayer* child = layer->firstChild(); child; child = child->nextSibling())
> > +                if (child->renderer()->hasBackground())
> > +                    appendOcclusion(toRenderBox(child->renderer()), occlusions);
> 
> Why is this looking at layer children, not renderer children?

Simply because iterating through the layers was the first thing that occurred to me and it worked. Why is using the renderer children more correct?
Comment 15 Robert Hogan 2011-08-16 12:53:47 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 102961 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=102961&action=review
> > 
> > Is the RenderLayer tree guaranteed to be current when getPluginOcclusions() is called? Are those plugin methods called during layout?
> 
> getPluginOcclusions() is called during layout...

This is incorrect, it's called when the plugin is getting painted, so at that point layout is meant to be complete. I've updated the patch per your other comments.
Comment 16 Robert Hogan 2011-08-21 11:55:56 PDT
When revising the patch I noticed a problem with the layout test, and in an effort to fix it discovered that the assumptions behind the current plugins/iframe-shims.html and my new test are wrong. The current iframe-shims.html test passes even when no attempt is made to occlude the plugin (by commenting out the call to getPluginOcclusions()). 

Both tests work fine manually (using QtTestBrowser for example), but the behaviour of the hit testing when run from DumpRenderTree does not depend at all on whether the plugins occlude the elements above/below them or not.

It's not due to using flash plugins in the tests as the same problem exhibits with the test netscape plugin. I suspect the problem is the use of windowed plugins run from DumpRenderTree without displaying the webview.

So it's possible to fix this bug, but currently not possible to write a test for it, and the existing plugins/iframe-shims.html test will continue to pass even if a regression happens.

Any suggestions how to proceed? I'm stuck.
Comment 17 Eric Seidel (no email) 2012-01-09 13:19:10 PST
Can we make TestNetscapePlugin not windowed?  Or have a not-windowed plugin?  We certainly use flash in other layout tests, seems we could use it here too.
Comment 18 Eric Seidel (no email) 2012-01-09 13:19:29 PST
Comment on attachment 102961 [details]
Patch

Based on smfr's comments, seems this needs at least one more round.
Comment 19 Cem Kocagil 2012-07-06 01:49:13 PDT
Any progress on this bug?
Comment 20 Robert Hogan 2012-07-06 10:23:23 PDT
(In reply to comment #19)
> Any progress on this bug?

I think the code-change is good - but it's extremely hard to create a valid test. The current z-pos/plugin testing coverage ( plugins/iframe-shims.html) is not valid either and will happily pass even plugin occlusions regress.

I would love a manual-test pass on this one as the effort/investigation required to get the testing working (some kind of fu required in TestNetscapePlugin) has eluded me. It's not windowed/not-windowed  - it is more to do with the need to get a painted background going in TestNetscapePlugin so that the hit-testing can work properly.
Comment 21 Matt Falkenhagen 2012-11-22 00:11:38 PST
Created attachment 175605 [details]
rebase prev patch to HEAD
Comment 22 Matt Falkenhagen 2012-11-22 00:20:55 PST
I updated Robert's patch for a current checkout. Two changes were needed:
- In getPluginOcclusions, replaced
    layer->dirtyZOrderLists();
    layer->updateZOrderLists();  // this is now private
with
    layer->updateLayerListsIfNeeded();
- PluginViewQt.cpp cannot be patched, as the getPluginOcclusions() call was removed in http://trac.webkit.org/changeset/124879

It's too bad a layout test is difficult due to hit testing for plugins run in DumpRenderTree. I guess just a pixel test is also hard?
Comment 23 Build Bot 2012-11-22 03:36:48 PST
Comment on attachment 175605 [details]
rebase prev patch to HEAD

Attachment 175605 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14959291

New failing tests:
plugins/fixed-position-object-occludes-plugin.html
Comment 24 Simon Fraser (smfr) 2013-02-07 12:47:17 PST
Comment on attachment 175605 [details]
rebase prev patch to HEAD

Ist this change Chromium-only? If so, the bug title should be prepended with [Chromium]
Comment 25 Eric Seidel (no email) 2013-02-07 13:07:35 PST
This is an ancient 4-digit bug. :)  Robert was reporting that this occurs for him on Chromium Linux with NPAPI flash.  But I'm not able to reproduce this bug on either Safari or Chrome (with built-in PPAPI flash) on my Mac.
Comment 26 Eric Seidel (no email) 2013-02-07 13:08:05 PST
This original reporter used the QuickTime plugin in Safari.
Comment 27 Anders Carlsson 2013-09-01 10:54:09 PDT
This bug was reported against Mac, and this has been working correctly on Mac since at least Snow Leopard.