Bug 3778 – Plugins not occluded by fixed position objects with higher z-index

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[chris habib]

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.

[Oliver Hunt]

This is reproducable in current ToT

[Joost de Valk (AlthA)]

it's the left side of the page here, but i can reproduce it :). confirming.

[David Storey]

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

[Alice Liu]

<rdar://problem/4338918>

[Seth A. Roby]

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.

[river]

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

[Robert Hogan]

Created attachment 101095 [details]
Patch

[Robert Hogan]

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.

[Robert Hogan]

(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.

[Simon Fraser (smfr)]

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.

[Robert Hogan]

Created attachment 102961 [details]
Patch

[Robert Hogan]

(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

[Simon Fraser (smfr)]

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?

[Robert Hogan]

(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?

[Robert Hogan]

(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.

[Robert Hogan]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 102961 [details]
Patch

Based on smfr's comments, seems this needs at least one more round.

[Cem Kocagil]

Any progress on this bug?

[Robert Hogan]

(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.

[Matt Falkenhagen]

Created attachment 175605 [details]
rebase prev patch to HEAD

[Matt Falkenhagen]

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?

[Build Bot]

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

[Simon Fraser (smfr)]

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]

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

This original reporter used the QuickTime plugin in Safari.

[Anders Carlsson]

This bug was reported against Mac, and this has been working correctly on Mac since at least Snow Leopard.