Bug 142582 - Scroll snap points are not supported on iframe content
Summary: Scroll snap points are not supported on iframe content
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-11 09:22 PDT by Brent Fulgham
Modified: 2015-03-15 23:29 PDT (History)
6 users (show)

See Also:


Attachments
Manual test case for iframe scroll snap point (2.47 KB, text/html)
2015-03-11 09:23 PDT, Brent Fulgham
no flags Details
Updated example (2.42 KB, text/html)
2015-03-15 16:49 PDT, Brent Fulgham
no flags Details
Patch (1.95 KB, patch)
2015-03-15 18:34 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (820.57 KB, application/zip)
2015-03-15 19:32 PDT, Build Bot
no flags Details
Patch (25.15 KB, patch)
2015-03-15 20:56 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (728.16 KB, application/zip)
2015-03-15 21:38 PDT, Build Bot
no flags Details
Patch (v3 correct some test failures) (25.03 KB, patch)
2015-03-15 21:53 PDT, Brent Fulgham
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-03-11 09:22:35 PDT
See attached test case for an example. The <iframe> element should support the scroll snap point feature.
Comment 1 Brent Fulgham 2015-03-11 09:23:19 PDT
Created attachment 248423 [details]
Manual test case for iframe scroll snap point
Comment 2 Radar WebKit Bug Importer 2015-03-11 09:23:20 PDT
<rdar://problem/20121319>
Comment 3 Brent Fulgham 2015-03-15 16:49:21 PDT
Created attachment 248691 [details]
Updated example
Comment 4 Brent Fulgham 2015-03-15 18:22:37 PDT
The scroll snap points were not being applied to the iframe contents because the code that sets up the scroll snap point content is not called for iframes.

To correct this, we need to make sure the snap offsets are set during post-frame layout for iframes. We also need to make sure (on Mac) that the scroll animator and timers are updated.
Comment 5 Brent Fulgham 2015-03-15 18:34:55 PDT
Created attachment 248696 [details]
Patch
Comment 6 Build Bot 2015-03-15 19:32:54 PDT
Comment on attachment 248696 [details]
Patch

Attachment 248696 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4684014753415168

New failing tests:
http/tests/security/storage-blocking-loosened-plugin.html
http/tests/security/cross-origin-plugin.html
platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-in-fixed.html
http/tests/navigation/anchor-frames-gbk.html
http/tests/security/cross-origin-plugin-allowed.html
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
http/tests/security/mixedContent/insecure-plugin-in-iframe.html
http/tests/security/contentSecurityPolicy/object-src-none-blocked.html
plugins/plugin-remove-subframe.html
plugins/plugin-document-back-forward.html
fast/frames/sandboxed-iframe-navigation-allowed.html
http/tests/plugins/create-v8-script-objects.html
plugins/resize-from-plugin.html
http/tests/security/contentSecurityPolicy/object-src-none-allowed.html
platform/mac-wk2/tiled-drawing/scrolling/frames/fixed-inside-frame.html
fast/dynamic/paused-event-dispatch.html
platform/mac-wk2/tiled-drawing/scrolling/frames/scroll-region-after-frame-layout.html
http/tests/plugins/cross-frame-object-access.html
http/tests/security/xssAuditor/object-src-inject.html
fast/frames/sandboxed-iframe-about-blank.html
http/tests/plugins/third-party-cookie-accept-policy.html
fast/replaced/frame-removed-during-resize-smaller.html
platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame.html
http/tests/security/storage-blocking-strengthened-plugin.html
fast/events/resize-subframe.html
Comment 7 Build Bot 2015-03-15 19:32:58 PDT
Created attachment 248698 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 8 Brent Fulgham 2015-03-15 20:56:42 PDT
Created attachment 248701 [details]
Patch
Comment 9 Brent Fulgham 2015-03-15 21:08:03 PDT
I'm surprised by these test failures, given that the new code just sets scroll snap state in a few member variables.
Comment 10 Build Bot 2015-03-15 21:38:26 PDT
Comment on attachment 248701 [details]
Patch

Attachment 248701 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4747936214810624

New failing tests:
http/tests/security/storage-blocking-loosened-plugin.html
http/tests/security/cross-origin-plugin.html
platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-in-fixed.html
http/tests/navigation/anchor-frames-gbk.html
http/tests/security/cross-origin-plugin-allowed.html
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
http/tests/security/mixedContent/insecure-plugin-in-iframe.html
http/tests/security/contentSecurityPolicy/object-src-none-blocked.html
plugins/plugin-remove-subframe.html
plugins/plugin-document-back-forward.html
fast/frames/sandboxed-iframe-navigation-allowed.html
http/tests/plugins/create-v8-script-objects.html
plugins/resize-from-plugin.html
http/tests/security/contentSecurityPolicy/object-src-none-allowed.html
platform/mac-wk2/tiled-drawing/scrolling/frames/fixed-inside-frame.html
fast/dynamic/paused-event-dispatch.html
platform/mac-wk2/tiled-drawing/scrolling/frames/scroll-region-after-frame-layout.html
http/tests/plugins/cross-frame-object-access.html
http/tests/security/xssAuditor/object-src-inject.html
fast/frames/sandboxed-iframe-about-blank.html
http/tests/plugins/third-party-cookie-accept-policy.html
fast/replaced/frame-removed-during-resize-smaller.html
platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame.html
http/tests/security/storage-blocking-strengthened-plugin.html
fast/events/resize-subframe.html
Comment 11 Build Bot 2015-03-15 21:38:29 PDT
Created attachment 248705 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 12 Brent Fulgham 2015-03-15 21:53:24 PDT
Created attachment 248707 [details]
Patch (v3 correct some test failures)
Comment 13 Simon Fraser (smfr) 2015-03-15 22:11:43 PDT
Comment on attachment 248707 [details]
Patch (v3 correct some test failures)

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

> Source/WebCore/page/FrameView.cpp:2986
> +#if PLATFORM(MAC)
> +        if (ScrollAnimator* scrollAnimator = existingScrollAnimator())
> +            return scrollAnimator->updateScrollAnimatorsAndTimers();
> +#endif

Why is this Mac-specific?
Comment 14 Brent Fulgham 2015-03-15 22:12:57 PDT
Comment on attachment 248707 [details]
Patch (v3 correct some test failures)

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

>> Source/WebCore/page/FrameView.cpp:2986
>> +#endif
> 
> Why is this Mac-specific?

You are right; we should make sure it's up-to-date for iOS as well.
Comment 15 Simon Fraser (smfr) 2015-03-15 22:15:44 PDT
Comment on attachment 248707 [details]
Patch (v3 correct some test failures)

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

> LayoutTests/css3/scroll-snap/resources/iframe-content.html:236
> +        top.succeeded = top.succeeded && shouldMatch("window.getComputedStyle(noInitial).getPropertyValue('-webkit-scroll-snap-points-x')", "repeat(100%)");
> +        top.succeeded = top.succeeded && shouldMatch("window.getComputedStyle(noInitial).getPropertyValue('-webkit-scroll-snap-points-y')", "repeat(100%)");
> +        top.succeeded = top.succeeded && shouldMatch("noInitial.style['-webkit-scroll-snap-destination']", "100% 100%");
> +        top.succeeded = top.succeeded && shouldMatch("window.getComputedStyle(noInitial).getPropertyValue('-webkit-scroll-snap-destination')", "100% 100%");
> +        top.succeeded = top.succeeded && shouldMatch("noInitial.style['-webkit-scroll-snap-coordinate']", "0% 0%");
> +        top.succeeded = top.succeeded && shouldMatch("window.getComputedStyle(noInitial).getPropertyValue('-webkit-scroll-snap-coordinate')", "0% 0%");
> +
> +        var initialType = document.getElementById('initialType');
> +        top.succeeded = top.succeeded && shouldMatch("initialType.style['-webkit-scroll-snap-type']", "initial");
> +        top.succeeded = top.succeeded && shouldMatch("window.getComputedStyle(initialType).getPropertyValue('-webkit-scroll-snap-type')", "none");
> +        top.succeeded = top.succeeded && shouldMatch("window.getComputedStyle(initialType).getPropertyValue('-webkit-scroll-snap-points-x')", "repeat(100%)");
> +        top.succeeded = top.succeeded && shouldMatch("window.getComputedStyle(initialType).getPropertyValue('-webkit-scroll-snap-points-y')", "repeat(100%)");
> +        top.succeeded = top.succeeded && shouldMatch("initialType.style['-webkit-scroll-snap-destination']", "0% 0%");
> +        top.succeeded = top.succeeded && shouldMatch("window.getComputedStyle(initialType).getPropertyValue('-webkit-scroll-snap-destination')", "0% 0%");
> +        top.succeeded = top.succeeded && shouldMatch("initialType.style['-webkit-scroll-snap-coordinate']", "100% 100%");
> +        top.succeeded = top.succeeded && shouldMatch("window.getComputedStyle(initialType).getPropertyValue('-webkit-scroll-snap-coordinate')", "100% 100%");
> +
> +        var initialXPoints = document.getElementById('initialXPoints');
> +        top.succeeded = top.succeeded && shouldMatch("window.getComputedStyle(initialXPoints).getPropertyValue('-webkit-scroll-snap-type')", "mandatory");
> +        top.succeeded = top.succeeded && shouldMatch("initialXPoints.style['-webkit-scroll-snap-points-x']", "initial");

Does this subframe really need all of this? Please reduce it to the minimum required to test the bug (probably one line).
Comment 16 Brent Fulgham 2015-03-15 22:34:58 PDT
Committed r181522: <http://trac.webkit.org/changeset/181522>
Comment 17 mitz 2015-03-15 23:29:38 PDT
(In reply to comment #14)
> Comment on attachment 248707 [details]
> Patch (v3 correct some test failures)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248707&action=review
> 
> >> Source/WebCore/page/FrameView.cpp:2986
> >> +#endif
> > 
> > Why is this Mac-specific?
> 
> You are right; we should make sure it's up-to-date for iOS as well.

updateScrollAnimatorsAndTimers() is Mac-only. Removing the conditional in the version of the patch committed in r181522 caused the iOS build to break. I restored it in <http://trac.webkit.org/r181524>.