Add heuristic to avoid flattening "fullscreen" iframes

• Component: Frames
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

Some authors implement fullscreen popups as out-of-flow iframes with size set to full viewport (using vw/vh units). We should not flatten such iframes or they could become larger than the viewport. Simon suggested to add it under a preference option (disabled by default for now).

[Frédéric Wang (:fredw)]

Created attachment 309587 [details]
Patch

[Frédéric Wang (:fredw)]

@Simon: I had two questions about this:

1) Maybe (as suggested by Antonio) the settings should be called FrameFlatteningExtraHeuristicsEnabled in case more heuristics are added?

2) Is there a convenient way to enable/disable the option during development? Is it necessary to add it to the context menu of experimental features for that purpose?

[Simon Fraser (smfr)]

(In reply to Frédéric Wang (:fredw) from comment #2)
> @Simon: I had two questions about this:
> 
> 1) Maybe (as suggested by Antonio) the settings should be called
> FrameFlatteningExtraHeuristicsEnabled in case more heuristics are added?

I don't like "FrameFlatteningExtraHeuristicsEnabled" because it doesn't indicate that frames are NOT flattened according to the heuristics.

Maybe we should have:

frameFlatteningEnabled
partialFrameFlatteningEnabled

where frameFlatteningEnabled is the current behavior, and partialFrameFlatteningEnabled is new behavior with heuristics.
frameFlatteningEnabled would trump partialFrameFlatteningEnabled.

> 2) Is there a convenient way to enable/disable the option during
> development? Is it necessary to add it to the context menu of experimental
> features for that purpose?

It would be nice to have this an an Experimental Feature (in the WebKit2 sense, like WebGPU).

[Frédéric Wang (:fredw)]

Created attachment 309897 [details]
Patch (use an enum for FrameFlattening)

[Frédéric Wang (:fredw)]

Created attachment 309898 [details]
Patch (use an enum for FrameFlattening)

[Build Bot]

Attachment 309898 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/C/WKAPICast.h:344:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/UIProcess/API/C/WKAPICast.h:344:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/C/WKAPICast.h:346:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/C/WKAPICast.h:348:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 309898 [details]
Patch (use an enum for FrameFlattening)

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

New failing tests:
fast/frames/flattening/iframe-flattening-crash.html

[Build Bot]

Created attachment 309904 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309898 [details]
Patch (use an enum for FrameFlattening)

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

New failing tests:
platform/mac/fast/frames/flattening/set-preference.html
fast/frames/flattening/iframe-flattening-crash.html

[Build Bot]

Created attachment 309906 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309898 [details]
Patch (use an enum for FrameFlattening)

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

New failing tests:
platform/mac/fast/frames/flattening/set-preference.html
fast/frames/flattening/iframe-flattening-crash.html

[Build Bot]

Created attachment 309910 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309898 [details]
Patch (use an enum for FrameFlattening)

Attachment 309898 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3726797

New failing tests:
fast/frames/flattening/iframe-flattening-crash.html

[Build Bot]

Created attachment 309912 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Simon Fraser (smfr)]

Comment on attachment 309898 [details]
Patch (use an enum for FrameFlattening)

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

> LayoutTests/plugins/frameset-with-plugin-frame.html:6
> +        internals.settings.setFrameFlattening(2); // Full Frame Flattening

Can we put some consts in the IDL, or use an IDL enum?

[Frédéric Wang (:fredw)]

Created attachment 310019 [details]
Patch (use an enum for FrameFlattening)

[Frédéric Wang (:fredw)]

Created attachment 310083 [details]
Patch (use an enum for FrameFlattening)

[Frédéric Wang (:fredw)]

Created attachment 310112 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 310112 [details]
Patch

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

> LayoutTests/fast/frames/flattening/iframe-flattening-fullscreen.html:22
> +        test(function() { assert_true(!isUnflatten("iframe2")); }, "iframe with vw/vh units");

note to self: these should probably be assert_false(isUnflatten)

[Simon Fraser (smfr)]

Comment on attachment 310112 [details]
Patch

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

> Source/WebCore/page/Settings.h:93
> +    FrameFlatteningPartiallyEnabled,

Can we qualify "partially"? What kind of frames still get flattened?

> Source/WebCore/rendering/RenderIFrame.cpp:89
> +            if (style().hasOutOfFlowPosition() && style().hasViewportUnits())
> +                return false;

Maybe move this code into a separate function, since I imagine these heuristics will evolve. The name of the function should reflect the enum value that is currently "FrameFlatteningPartiallyEnabled".

> Source/WebKit/mac/WebView/WebPreferencesPrivate.h:163
> +- (unsigned)frameFlattening;
> +- (void)setFrameFlattening:(unsigned)flag;

unsigned -> WebKitFrameFlattening.

Actually I think you need to leave the existing functions alone, for API compatibility, but still add the new ones.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:906
> +    [preferences setFrameFlattening:NO];

This should use the enum, not a BOOL.

[Frédéric Wang (:fredw)]

Created attachment 313265 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 313265 [details]
Patch for landing

Clearing flags on attachment: 313265

Committed r218480: <http://trac.webkit.org/changeset/218480>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.