Bug 142582

Summary:

Scroll snap points are not supported on iframe content

Product:

WebKit

Reporter:

Brent Fulgham <bfulgham>

Component:

Layout and Rendering

Assignee:

Brent Fulgham <bfulgham>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, buildbot, mitz, rniwa, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

All

Attachments:

Description	Flags
Manual test case for iframe scroll snap point	none
Updated example	none
Patch	none
Archive of layout-test-results from ews106 for mac-mavericks-wk2	none
Patch	none
Archive of layout-test-results from ews105 for mac-mavericks-wk2	none
Patch (v3 correct some test failures)	simon.fraser: review+

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