Bug 28914 – Add a test for iframe shims and windowed plugin

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Victor Wang]

Add a test to test iframe shims can be used to overlay html above a windows plugin.

[Victor Wang]

Created attachment 38935 [details]
Add layout test

[Victor Wang]

Created attachment 38936 [details]
Proposed patch

Remove the incorrect entry in Changlog in previous attachment

[Victor Wang]

Created attachment 38956 [details]
Updated patch (indent 4 spaces instead of 2)

[Victor Wang]

Created attachment 38996 [details]
Updated patch

[Dimitri Glazkov (Google)]

Comment on attachment 38996 [details]
Updated patch

Quite a few of style issues below:

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 47984)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> [phone]  Victor Wang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=28914
> +
> +        Add layout test to test iframe shim can be used
> +        to overlay html above a windowed plugin. It checks
> +        premutations of iframe shimes in relationship to 

shimes => shims.

> +    var HEIGHT = 100;
width

> +    var WIDTH = 100;
height

> +        i.style.filter='progid:DXImageTransform.Microsoft.Alpha(style=0,opacity=0)';

This is IE-specific. Why do we need it here?

> +        var case_id = items;

caseId

> +        var expect_clickable = tags.expect && tags.expect.indexOf('UNDER') == -1;

expectClickable

> +        expected_clicks[case_id] = expect_clickable;

expectedClicks

> +        var plugin_div_z;

pluginDivZ

> +        var overlay_div_z_iframe;

overlayDivZIframe

> +        var overlay_div_z_overlay;

overlayDivZOverlay, and below, lots of underscore_delimited_var_names. WebKit convention is camelCase.

[Victor Wang]

Created attachment 39000 [details]
Updated patch with style fixes

[Sam Weinig]

What is an iframe shim?  I am not familiar with this term in this context.

[Victor Wang]

Created attachment 39004 [details]
Updated patch with style fixes

[Ojan Vafai]

iframe-shims are a fairly common hack to get around z-ordering issues with windowed plugins. For example, for a menu that needs to overlay on top of windowed flash. Since on Windows, windowed plugins are in their own HWND, they don't respect z-index and go on top of the menu. Iframes in IE/FF historically have been implemented using an HWND, so if you put an iframe behind the menu, it will block the paint of the plugin and allow your menu to show on top.

A google search for iframe shim shows a lot of hits for this sort of thing. For a real-world example, look at any html dialog in any Google product (e.g. gmail, docs, etc.).

Thatcher added support for this behavior to Chromium by clipping plugins when there's an iframe above them. There's a FIXME to move that logic into platform-agnostic code. This is the layout-test for that behavior (matches IE).

I don't remember the details, but I think making it platform-agnostic was non-trivial, which is why it wasn't done that way to start with.

[Thatcher Ulrich]

A couple notes to add to what Ojan said:

On Linux+FF, iframe shims work very similarly and fill the same need as on the Windows browsers.

On Mac, iframe shims are largely irrelevant -- there is no such thing as a windowed plugin so the HTML compositing model generally does the right thing.  (There remain many complications when working with an OpenGL plugin, but iframe shims never helped with that on Mac, so the Google Earth Plugin has to use even worse workarounds.)

There's an early description of the technique here: http://www.macridesweb.com/oltest/IframeShim.html

I have a test page that explores some aspects of the technique: http://tulrich.com/iframe_shims/

[Victor Wang]

Thanks Ojan and Thatcher on providing more background about this test.

Dimitri Glazkov,
I fixed the style issue (sorry, I thought the webkit style guide is for c/c++ code, not layout test). The IE specific filter is not required so I removed it. Please take another look and let me know if you have more comments.

[Dimitri Glazkov (Google)]

Comment on attachment 39004 [details]
Updated patch with style fixes

Almost there!

> +                if (!tags.pluginNorelative) {
> +                    parentdiv.style.position = 'relative';
> +                }

No braces around one-liners.

> +
> +                if (pluginDivZ) {
> +                    parentdiv.style.zIndex = pluginDivZ;
> +                }

Ditto.

> +                root.appendChild(parentdiv);
> +            } else {
> +                if (!tags.pluginNorelative) {
> +                    pd.style.position = 'relative';

Ditto.


> +            if (overlayDivZIframe) {
> +                id.style.zIndex = overlayDivZIframe;

Ditto.


> +            if (overlayDivZOverlay) {
> +            od.style.zIndex = overlayDivZOverlay;
> +            }

No braces + indent.

> +                if (overlayDivZOverlay) {
> +                    parentdiv.style.zIndex = overlayDivZOverlay;
> +                }

No braces.

> +        for (k in expectedClicks) {
> +            if (expectedClicks[k] && !clicks[k]) {
> +                waitingForMoreClicks = true;
> +	    } else if (!expectedClicks[k] && clicks[k]) {

Indent issue here. Should be one more to indent w/if?

[Victor Wang]

Created attachment 39279 [details]
Patch with style fixes

[Victor Wang]

Created attachment 39280 [details]
More style fixes

[Victor Wang]

done. please take another look. thanks!
(In reply to comment #12)
> (From update of attachment 39004 [details])
> Almost there!
> 
> > +                if (!tags.pluginNorelative) {
> > +                    parentdiv.style.position = 'relative';
> > +                }
> 
> No braces around one-liners.
> 
> > +
> > +                if (pluginDivZ) {
> > +                    parentdiv.style.zIndex = pluginDivZ;
> > +                }
> 
> Ditto.
> 
> > +                root.appendChild(parentdiv);
> > +            } else {
> > +                if (!tags.pluginNorelative) {
> > +                    pd.style.position = 'relative';
> 
> Ditto.
> 
> 
> > +            if (overlayDivZIframe) {
> > +                id.style.zIndex = overlayDivZIframe;
> 
> Ditto.
> 
> 
> > +            if (overlayDivZOverlay) {
> > +            od.style.zIndex = overlayDivZOverlay;
> > +            }
> 
> No braces + indent.
> 
> > +                if (overlayDivZOverlay) {
> > +                    parentdiv.style.zIndex = overlayDivZOverlay;
> > +                }
> 
> No braces.
> 
> > +        for (k in expectedClicks) {
> > +            if (expectedClicks[k] && !clicks[k]) {
> > +                waitingForMoreClicks = true;
> > +	    } else if (!expectedClicks[k] && clicks[k]) {
> 
> Indent issue here. Should be one more to indent w/if?

[Dimitri Glazkov (Google)]

Comment on attachment 39280 [details]
More style fixes

r=me.

[Eric Seidel (no email)]

Strange.

fatal: pathspec 'LayoutTests/plugins/resources/simple_blank.swf' did not match any files

But yet svn-apply exit'd 0.  I think that something is broken with svn-apply!

[Eric Seidel (no email)]

Comment on attachment 39280 [details]
More style fixes

Filed bug 29100.  But the commit-queue can't handle this patch, and I think I'm going to have to turn it off until I can fix bug 29100.

[Eric Seidel (no email)]

Did you modify this patch by hand, or is this a patch produced straight from svn-create-patch?  However the binary file was added seems to have confused svn-apply.

[Victor Wang]

(In reply to comment #19)
> Did you modify this patch by hand, or is this a patch produced straight from
> svn-create-patch?  However the binary file was added seems to have confused
> svn-apply.

The patch was created by svn-create-patch. The binary file has a prop set to svnLmime-type application/octet-stream, should this be changed?

[Eric Seidel (no email)]

No, I think our scripts are just broken.

[Victor Wang]

(In reply to comment #21)
> No, I think our scripts are just broken.

OK. Thanks for your help! Let me know of anything need from me.

[Eric Seidel (no email)]

Comment on attachment 39280 [details]
More style fixes

OK.  I've fixed bug 29100 and am now running the commit-queue with the patch applied.  This should land properly now.

[Eric Seidel (no email)]

Comment on attachment 39280 [details]
More style fixes

Never mind.  I don't have a patched copy running locally.  We'll just wait for bug 29100 to be resolved.

[WebKit Commit Bot]

Comment on attachment 39280 [details]
More style fixes

Clearing flags on attachment: 39280

Committed r48638: <http://trac.webkit.org/changeset/48638>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.