Bug 149264

Summary: IFrame scrolling=yes is ignored in iOS Safari
Product: WebKit Reporter: Dima Voytenko <dvoytenko>
Component: FramesAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: ajuma, bdakin, benjamin, buildbot, danyao, dbates, fred.wang, mcatanzaro, rbyers, ries, simon.fraser, steve, thorton, tonikitoo, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari 9   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 171667, 173833, 179125, 179172, 133665, 171914, 171974, 172107, 173405, 173644, 173704, 173763, 174097, 174130, 174134, 178211    
Bug Blocks: 179794    
Attachments:
Description Flags
testcase
none
Do not flatten iframes
none
Do not flatten iframes with viewport units.
none
Do not flatten iframes with viewport units.
none
Small patch to disable frame flattening on iOS
none
Test Patch to disable frame flattening and enable async frame scrolling
none
Test Patch to disable frame flattening and enable async frame scrolling
buildbot: commit-queue-
Archive of layout-test-results from ews124 for ios-simulator-wk2 none

Description Dima Voytenko 2015-09-17 10:10:48 PDT
This bin reproduces the issue: http://jsbin.com/dedega

<iframe scrolling=yes> has no effect on iOS Safari (tested against iOS 9) - the iframe is not scrollable. This is supposed to work according to spec and works virtually on every browser/device I tried except iOS Safari, including desktop Safari.

Please advise.
Comment 1 Rick Byers 2015-09-18 08:28:43 PDT
Tested on iOS 9, still broken.

It looks like web developers have struggled with this for awhile, and claim it got worse in iOS 8 eg:
http://stackoverflow.com/questions/26046373/iframe-scrolling-ios-8
https://css-tricks.com/forums/topic/scrolling-iframe-on-ipad/

Simon, any chance your work on improving viewports/scrolling might help here?  It seems really confusing that an iframe should behave differently from a scrollable div in this regard, and that iOS would be different from desktop Safari.

Dima tells me that the obvious work-around of putting a scrollable div inside the iframe is problematic for him because he doesn't have control of the iframe content/styling.  He can make the iframe body overflow:auto (along with html) and that works, but then is burned by bug 106133 (can't get/set the scroll position because body.scrollTop actually refers to the viewport).  Ultimately he ends up having to use a dummy DIV and getBoundlingClientRect in order to detect the scroll position of the iframe, yuck!
Comment 2 Simon Fraser (smfr) 2015-09-18 08:35:59 PDT
iOS has always "flattened" (expanded) iframes. This was done to avoid the user getting trapped in a hierarchy of scrollable regions, which was thought to give a bad user experience on a small screen.

It may be time to reconsider this approach.
Comment 3 zalan 2015-09-18 08:38:08 PDT
(In reply to comment #2)
> iOS has always "flattened" (expanded) iframes. This was done to avoid the
> user getting trapped in a hierarchy of scrollable regions, which was thought
> to give a bad user experience on a small screen.
> 
> It may be time to reconsider this approach.
I second that!
Comment 4 Dima Voytenko 2015-09-18 09:13:08 PDT
Second it as well. I can see how this would be a problem with legacy sites overusing some of these tools like IFrames. But I think that time is gone or at least developers should be able to override this.
Comment 5 Simon Fraser (smfr) 2015-09-28 17:44:23 PDT
Ojan suggested not flattening frames on pages with viewport tags.
Comment 6 David Kilzer (:ddkilzer) 2015-10-02 20:39:32 PDT
<rdar://problem/22961085>
Comment 7 Ries 2016-12-22 14:21:32 PST
Don't know if I am supposed to here, but I'd like to upvote this issue. It's really a pain in 2016 that on iOS only the iframe changes its size, while it does not on other platforms.
Comment 8 Frédéric Wang (:fredw) 2017-04-20 03:22:52 PDT
Created attachment 307586 [details]
testcase
Comment 9 Frédéric Wang (:fredw) 2017-04-28 00:27:09 PDT
Created attachment 308506 [details]
Do not flatten iframes

This is a preliminary patch to remove flattening for iframe. However, it's not enough to make scrolling work (I'm still investigating this...).

In particular, we have an issue similar to bug 163911. A quick workaround to make scrolling work with the Find operation is:

--- a/Source/WebCore/editing/Editor.cpp
+++ b/Source/WebCore/editing/Editor.cpp
@@ -3122,2 +3122,2 @@ bool Editor::findString(const String& target, FindOptions options)
-    if (!(options & DoNotRevealSelection))
-        m_frame.selection().revealSelection();
+    // if (!(options & DoNotRevealSelection))
+    m_frame.selection().revealSelection();
--- a/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
+++ b/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
@@ -1370,3 +1370,4 @@ void WebFrameLoaderClient::transitionToCommittedForNewPage()
 #if PLATFORM(IOS)
-    m_frame->coreFrame()->view()->setDelegatesScrolling(true);
+    if (m_frame->coreFrame()->isMainFrame())
+        m_frame->coreFrame()->view()->setDelegatesScrolling(true);
 #endif
Comment 10 Simon Fraser (smfr) 2017-04-28 09:05:44 PDT
Turning off frame flattening on iOS is a big usability change and needs to be done with caution.
Comment 11 zalan 2017-04-28 09:07:28 PDT
(In reply to Simon Fraser (smfr) from comment #10)
> Turning off frame flattening on iOS is a big usability change and needs to
> be done with caution.
and I don't see the point of doing it for iframes only.
Comment 12 Frédéric Wang (:fredw) 2017-04-28 09:26:01 PDT
(In reply to Simon Fraser (smfr) from comment #10)
> Turning off frame flattening on iOS is a big usability change and needs to
> be done with caution.

(In reply to zalan from comment #11)
> (In reply to Simon Fraser (smfr) from comment #10)
> > Turning off frame flattening on iOS is a big usability change and needs to
> > be done with caution.
> and I don't see the point of doing it for iframes only.

@Simon, @Zalan: This was really an experimental patch to avoid flattening and be able to test a bit things. I think it would be good to agree on how to provide this non-flattening option to Web developers. Is the suggestion of comment 5 enough? Any other idea?

Scrolling with touch events does not work at the moment with iframes even with that patch. As I see, 'overflow:auto' content creates ScrollingTreeOverflowScrollingNodeIOS nodes in order to capture these events. Should we add such mechanism for iframes too?
Comment 13 Simon Fraser (smfr) 2017-04-28 10:56:18 PDT
(In reply to Frédéric Wang (:fredw) from comment #12)
> Is the suggestion of comment 5 enough? Any other idea?

No, it's much too late to use the viewport tag as a key to not frame flatten.

> Scrolling with touch events does not work at the moment with iframes even
> with that patch. As I see, 'overflow:auto' content creates
> ScrollingTreeOverflowScrollingNodeIOS nodes in order to capture these
> events. Should we add such mechanism for iframes too?

There's a bunch of work that has to happen to make frames/iframes scrollable, involving educating the scrolling state tree & scrolling tree about scrollable frames (we don't even do that on Mac yet), making latching work correctly etc. Then the UI process has to be fixed to make UIScrollViews for scrollable frames.

I think the best course of action would be to make iframes fast-scrollable on Mac first, to set up that infrastructure.
Comment 14 Frédéric Wang (:fredw) 2017-04-28 11:22:14 PDT
(In reply to Simon Fraser (smfr) from comment #13)
> There's a bunch of work that has to happen to make frames/iframes
> scrollable, involving educating the scrolling state tree & scrolling tree
> about scrollable frames (we don't even do that on Mac yet), making latching
> work correctly etc. Then the UI process has to be fixed to make
> UIScrollViews for scrollable frames.

Yes, that more or less matches my understanding after a first investigation of the code. Would that mean creating a new ScrollingNodeType for iframe/frame? Or re-using an existing one?

> I think the best course of action would be to make iframes fast-scrollable
> on Mac first, to set up that infrastructure.

I see. Please cc' me if there are Bugzilla entries regarding this or similar TODO.
Comment 15 Rick Byers 2017-04-28 19:17:17 PDT
> Turning off frame flattening on iOS is a big usability change and needs to be done with caution.

Simon, I know we chatted briefly in Tokyo about this, but I'm curious to better understand your thinking here.  Why is this a greater usability risk than supporting overflow:scroll divs?

As I mentioned, I don't think I've ever heard a single user complain in Chrome about getting stuck in an iframe scroll chain.  We do occasionally hear complaints of users getting stuck in regions that cancel all the touch events (eg. it's possible to fling-pinch to zoom in on an area completely covered by a map, and now be unable to zoom/pan the viewport with no recourse other than reload).

Certainly nested scrollers are generally a bad UX paradigm, but virtually all mobile-optimized sites already avoid them, as do many desktop sites using scrollable iframes (eg. think of the gmail compose window or JSBin.com).  App-like layouts are increasingly common where the main document isn't scrollable at all, but there are some single elements (div or iframe) which are.  If it would help I could add some metrics to Chrome to track how often users encounter such nested scroller scenarios on Android and (after giving some time for the metrics to reach a non-trivial fraction of our users) report numbers here.
Comment 16 Frédéric Wang (:fredw) 2017-05-03 09:35:03 PDT
(In reply to Simon Fraser (smfr) from comment #13)
> There's a bunch of work that has to happen to make frames/iframes
> scrollable, involving educating the scrolling state tree & scrolling tree
> about scrollable frames (we don't even do that on Mac yet), making latching
> work correctly etc. Then the UI process has to be fixed to make
> UIScrollViews for scrollable frames.
> 
> I think the best course of action would be to make iframes fast-scrollable
> on Mac first, to set up that infrastructure.

@Simon: I did more analysis on this (see https://people.igalia.com/fwang/ios/class-hierarchy/ for my notes) and I have some questions/comments:

0) Can you elaborate on what you mean by "iframe fast-scrollable on Mac"? I understand it means creating scrolling node for the iframe in order to handle user interaction in a separate process/thread. Is that correct?

1) There are RenderFrame/RenderIFrame::requiresLayer and RenderLayerCompositor::requiresCompositingForFrame functions involved in the decision of whether the frame will have a RenderLayer or RenderLayerBacking attached to the iframe/frame renderer. I believe we should change these functions somehow to force that attachment?

2) Similarly, it seems that the condition RenderLayer::hasTouchScrollableOverflow (iOS) or RenderLayer::needsCompositedScrolling (Mac) to create a scrolling node should be changed to take into consideration the frame/iframe.

3) Repeating comment 14, should we add another ScrollingNodeType for iframe/frame? Or re-using an existing one?

4) One hesitation I have is whether we want the scrolling node for the iframe/frame's renderer itself, or for RenderView attached to the document loaded into that iframe/frame?

Thanks
Comment 17 Simon Fraser (smfr) 2017-05-03 13:06:59 PDT
(In reply to Frédéric Wang (:fredw) from comment #16)
> (In reply to Simon Fraser (smfr) from comment #13)
> > There's a bunch of work that has to happen to make frames/iframes
> > scrollable, involving educating the scrolling state tree & scrolling tree
> > about scrollable frames (we don't even do that on Mac yet), making latching
> > work correctly etc. Then the UI process has to be fixed to make
> > UIScrollViews for scrollable frames.
> > 
> > I think the best course of action would be to make iframes fast-scrollable
> > on Mac first, to set up that infrastructure.
> 
> @Simon: I did more analysis on this (see
> https://people.igalia.com/fwang/ios/class-hierarchy/ for my notes) and I
> have some questions/comments:
> 
> 0) Can you elaborate on what you mean by "iframe fast-scrollable on Mac"? I
> understand it means creating scrolling node for the iframe in order to
> handle user interaction in a separate process/thread. Is that correct?

This involves:
1. Creating the right layer hierarchy with tiled backing store for the iframe contents, as we do for the main frame.
2. Creating ScrollingStateFrameScrollingNodes for them
3. Making sure the scrolling tree handles nested ScrollingStateFrameScrollingNodes properly
4. Not putting iframe's ScrollableAreas into the non-fast scrollable region
4. Teaching the scrolling thread to direct a mouse wheel at the appropriate ScrollingTreeFrameScrollingNode
5. Making sure that scroll latching works
6. Probably more

> 1) There are RenderFrame/RenderIFrame::requiresLayer and
> RenderLayerCompositor::requiresCompositingForFrame functions involved in the
> decision of whether the frame will have a RenderLayer or RenderLayerBacking
> attached to the iframe/frame renderer. I believe we should change these
> functions somehow to force that attachment?

Yes, those fast-scrollable frames/iframes will need to be composited in their parent document.

> 2) Similarly, it seems that the condition
> RenderLayer::hasTouchScrollableOverflow (iOS) or
> RenderLayer::needsCompositedScrolling (Mac) to create a scrolling node
> should be changed to take into consideration the frame/iframe.

Those are more for overflow:scroll. I think scrollable frames/iframes would not use this code.

> 3) Repeating comment 14, should we add another ScrollingNodeType for
> iframe/frame? Or re-using an existing one?

Scrolling{State|Tree}FrameScrollingNode should suffice.

> 4) One hesitation I have is whether we want the scrolling node for the
> iframe/frame's renderer itself, or for RenderView attached to the document
> loaded into that iframe/frame?

The scrolling node should get hooked up with the layers inside the frame/iframe, just like for the main page.
Comment 18 Frédéric Wang (:fredw) 2017-05-04 09:58:47 PDT
(In reply to Simon Fraser (smfr) from comment #13)
> I think the best course of action would be to make iframes fast-scrollable
> on Mac first, to set up that infrastructure.

I'm moving this preliminary step to bug 171667.
Comment 19 Frédéric Wang (:fredw) 2017-05-05 11:34:16 PDT
(In reply to Simon Fraser (smfr) from comment #13)
> (In reply to Frédéric Wang (:fredw) from comment #12)
> > Is the suggestion of comment 5 enough? Any other idea?
> 
> No, it's much too late to use the viewport tag as a key to not frame flatten.
> 

@Simon: What about checking the "-webkit-overflow-scrolling: touch;" property on the iframe/frame elements?
Comment 20 Simon Fraser (smfr) 2017-05-05 13:06:04 PDT
(In reply to Frédéric Wang (:fredw) from comment #19)
> @Simon: What about checking the "-webkit-overflow-scrolling: touch;"
> property on the iframe/frame elements?

I really don't want to propagate use of that property.
Comment 21 Frédéric Wang (:fredw) 2017-05-09 02:52:45 PDT
Created attachment 309487 [details]
Do not flatten iframes with viewport units.
Comment 22 Frédéric Wang (:fredw) 2017-05-09 02:55:04 PDT
Created attachment 309488 [details]
Do not flatten iframes with viewport units.
Comment 23 Frédéric Wang (:fredw) 2017-06-15 08:12:23 PDT
Created attachment 312982 [details]
Small patch to disable frame flattening on iOS
Comment 24 Frédéric Wang (:fredw) 2017-06-26 08:07:27 PDT
Created attachment 313838 [details]
Test Patch to disable frame flattening and enable async frame scrolling
Comment 25 Frédéric Wang (:fredw) 2017-07-05 07:53:03 PDT
So just to follow-up here, I have a first experimental set of patches to make iframe scrollables:

attachment 314570 [details]
attachment 314559 [details]
attachment 314574 [details]
attachment 314603 [details]
attachment 173644 [details]
attachment 313838 [details]
Comment 26 Frédéric Wang (:fredw) 2017-07-27 02:02:07 PDT
Created attachment 316536 [details]
Test Patch to disable frame flattening and enable async frame scrolling

Rebasing.
Comment 27 Build Bot 2017-07-31 02:52:01 PDT
Comment on attachment 316536 [details]
Test Patch to disable frame flattening and enable async frame scrolling

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

New failing tests:
fast/images/low-memory-decode.html
Comment 28 Build Bot 2017-07-31 02:52:02 PDT
Created attachment 316760 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

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