Bug 154399 - Position fixed is buggy with overflow:auto scrolling inside iframes
Summary: Position fixed is buggy with overflow:auto scrolling inside iframes
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari 9
Hardware: iPhone / iPad iOS 9.3
: P2 Major
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on: 176243 179946 175135
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-18 10:35 PST by Dima Voytenko
Modified: 2017-11-22 07:18 PST (History)
7 users (show)

See Also:


Attachments
Visual effects with position:fixed (21.71 MB, video/quicktime)
2017-06-02 12:50 PDT, Dima Voytenko
no flags Details
More visual effects with position:fixed (11.41 MB, video/quicktime)
2017-06-02 12:50 PDT, Dima Voytenko
no flags Details
Testcase that works (6.56 KB, text/html)
2017-07-26 13:25 PDT, Simon Fraser (smfr)
no flags Details
Reduced testcase (flickering) (640 bytes, text/html)
2017-07-27 09:12 PDT, Frédéric Wang (:fredw)
no flags Details
Patch (fixedPositionRect ScrollingTreeOverflowScrollingNodeIOS::updateChildNodesAfterScroll) (2.42 KB, patch)
2017-08-02 07:31 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dima Voytenko 2016-02-18 10:35:22 PST
To reproduce:

1. Open http://output.jsbin.com/nayoxe/quiet on iOS
2. Try to scroll fast

Observe:

Top and bottom elements styled with `position:fixed` jump and flicker.

Expected:

`position:fixed` elements should stay put.

Description:

This is a somewhat specific example. It's uses `overflow:auto` scrolling inside of an iframed document, which is a workaround for the fact that iOS doesn't allow iframes to scroll.

Source code: http://jsbin.com/nayoxe/edit?html,css,output

Please advise.
Comment 1 Radar WebKit Bug Importer 2016-02-19 09:12:34 PST
<rdar://problem/24742251>
Comment 2 Frédéric Wang (:fredw) 2017-05-24 04:23:41 PDT
Important note: it seems that this bug is much more obvious with real device than with the simulator.

I was wondering whether bug 172349 could be involved here, but I don't really see much difference on the simulator (again it would be good to try it on real device to be really sure).
Comment 3 Simon Fraser (smfr) 2017-05-24 15:33:21 PDT
I don't see severe jumping or flicking of the top and bottom bars in that testcase, on 10.3.2. I do see some subpixel jittering, which is because WebCore rounds scroll positions to integral values, and when the page has a non-unit scale there's some rounding going on.

On what device and iOS version do you see the more severe flickering?

I don't think this is related to bug 172349.
Comment 4 Frédéric Wang (:fredw) 2017-05-30 22:42:59 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> I don't see severe jumping or flicking of the top and bottom bars in that
> testcase, on 10.3.2. I do see some subpixel jittering, which is because
> WebCore rounds scroll positions to integral values, and when the page has a
> non-unit scale there's some rounding going on.
> 
> On what device and iOS version do you see the more severe flickering?

OK, I was actually testing with the simulator version 10.3.2 (where I see the behavior you describe) and the testing on a real device was done by dvoytenko and jfernandez (where they see the bug). So maybe it's actually related to the iOS version?
Comment 5 Simon Fraser (smfr) 2017-05-30 23:04:43 PDT
This code changed when I implemented visual viewports in 10.3, so it's quite possible it changed.

Please re-open if you can reproduce on 10.3 or later.
Comment 6 Javier Fernandez 2017-06-01 00:28:41 PDT
I've been trying to reproduce the issue in iOS 10.3.2 and, even that there are some artifacts, the big jumps seem to have gone. I can't be sure about some subtle flickering, but that could be a different issue.
Comment 7 Dima Voytenko 2017-06-02 12:50:02 PDT
Created attachment 311845 [details]
Visual effects with position:fixed
Comment 8 Dima Voytenko 2017-06-02 12:50:26 PDT
Created attachment 311846 [details]
More visual effects with position:fixed
Comment 9 Dima Voytenko 2017-06-02 12:51:27 PDT
I'll reopen. There are still a fair amount of visual effects. I've put together an example in http://output.jsbin.com/hatega/quiet. The screen recordings are attached.
Comment 10 Simon Fraser (smfr) 2017-06-02 13:33:51 PDT
Thanks for the data.
Comment 11 Frédéric Wang (:fredw) 2017-07-21 00:30:53 PDT
I just tested Dima's test cases:

http://output.jsbin.com/hatega/quiet
https://output.jsbin.com/tegomaj/quiet

with a development build of Safari ( https://trac.webkit.org/changeset/219679/webkit ) and I see the flickering and big jumps with the simulator. See https://people.igalia.com/fwang/ios/2017-07-21/

@Simon: Are you able to reproduce the issue too?
Comment 12 Simon Fraser (smfr) 2017-07-26 12:51:26 PDT
Yes, I can.
Comment 13 Simon Fraser (smfr) 2017-07-26 13:25:36 PDT
Created attachment 316469 [details]
Testcase that works

This doesn't happen with all position:fixed inside overflow scroll. This attachment works fine.
Comment 14 Simon Fraser (smfr) 2017-07-26 13:33:47 PDT
So both jsbin examples suffer from being inside an <iframe>. Do you have content that shows this bug outside of an <iframe>?
Comment 15 Frédéric Wang (:fredw) 2017-07-27 09:12:01 PDT
Created attachment 316551 [details]
Reduced testcase (flickering)

This is a reduced testcase for flickering. Indeed, if I don't embed the content inside an iframe this flickering bug goes away, so the iframe seems involved here.

Note: For now I have not been able to find which content added by AMP is causing the big jumps.
Comment 16 Frédéric Wang (:fredw) 2017-07-28 03:33:49 PDT
(In reply to Frédéric Wang (:fredw) from comment #15)
> Note: For now I have not been able to find which content added by AMP is
> causing the big jumps.

I tried reducing the test further this morning, removing all the unnecessary generated CSS and markup. This is the best I was able to get for now:

https://people.igalia.com/fwang/ios/position-fixed-bugs/flicker-and-jump.html

IIUC, AMP creates a second HTML element which is used as a workaround for bug 5991 without breaking CSS selectors. However, it is very difficult to work on the minified AMP file in order to remove or simplify this element or even just understand when its scroll* properties are read/set (which might confuse the scrolling).

@Dima: Are you able to provide a testcase for the jumps that does not involve the minified AMP script (similar to attachment 316551 [details] for flickering)? Or at least where can I find a non-minified version of the AMP script?
Comment 17 Dima Voytenko 2017-07-28 17:02:50 PDT
From what I can tell, this is because of a touch event on the body. See reduced example on http://output.jsbin.com/fusijum/quiet

All our touch events are always passive, but I don't think we can communicate this to WebKit yet.
Comment 18 Simon Fraser (smfr) 2017-07-28 17:44:43 PDT
WebKit does support passive event listener registration.
Comment 19 Dima Voytenko 2017-07-28 18:41:42 PDT
I'll retest with passive then. Not 100% sure that'd help yet.
Comment 20 Frédéric Wang (:fredw) 2017-07-29 01:19:29 PDT
(In reply to Dima Voytenko from comment #17)
> From what I can tell, this is because of a touch event on the body. See
> reduced example on http://output.jsbin.com/fusijum/quiet
> 
> All our touch events are always passive, but I don't think we can
> communicate this to WebKit yet.

Thanks. I just tested it and I can reproduce the "big jumps" with a release build, but not with a nightly build. So maybe more things from the js file of
https://people.igalia.com/fwang/ios/position-fixed-bugs/flicker-and-jump.html are involved here...
Comment 21 Simon Fraser (smfr) 2017-07-31 12:03:33 PDT
At least part of the problem here is that constraints passed to ScrollingStateFixedNode::updateConstraints() are computed using the fixed layout rect for the iframe (which is correct), but ScrollingTreeOverflowScrollingNodeIOS::updateChildNodesAfterScroll() just grabs scrollingTree().fixedPositionRect() which is for the main frame.
Comment 22 Frédéric Wang (:fredw) 2017-08-02 07:28:37 PDT
(In reply to Simon Fraser (smfr) from comment #21)
> At least part of the problem here is that constraints passed to
> ScrollingStateFixedNode::updateConstraints() are computed using the fixed
> layout rect for the iframe (which is correct), but
> ScrollingTreeOverflowScrollingNodeIOS::updateChildNodesAfterScroll() just
> grabs scrollingTree().fixedPositionRect() which is for the main frame.

So trying to understand what you mean: Basically, the fixed layout rect for the iframe is passed to the fixed node with the following trace:

ScrollingStateFixedNode::updateConstraints
AsyncScrollingCoordinator::updateNodeViewportConstraints
RenderLayerCompositor::updateScrollCoordinatedLayer

Then ScrollingTreeOverflowScrollingNodeIOS::updateChildNodesAfterScroll recursively calls updateLayersAfterAncestorChange, passing the scrollingTree().fixedPositionRect() which for fixed nodes ends up at the following trace:

FixedPositionViewportConstraints::layerPositionForViewportRect
ScrollingTreeFixedNode::updateLayersAfterAncestorChange
...

Naturally, the new fixed rect and position can be different from the ScrollingConstraints::m_*AtLastLayout values and are updated by updateLayersAfterAncestorChange.

But as you said, the passed value scrollingTree().fixedPositionRect() refers to the main frame not the iframe. I'll upload a patch that tries to copy what is done in ScrollingTreeFrameScrollingNodeIOS::updateChildNodesAfterScroll.
Comment 23 Frédéric Wang (:fredw) 2017-08-02 07:31:05 PDT
Created attachment 316959 [details]
Patch (fixedPositionRect ScrollingTreeOverflowScrollingNodeIOS::updateChildNodesAfterScroll)

This patch tries to use the fixed rect of the first the ancestor iframe, copying the logic in ScrollingTreeFrameScrollingNodeIOS::updateChildNodesAfterScroll. This seems to get rid of the "flickering" (and "disappear") effect but does not solve the big jumps.
Comment 24 Frédéric Wang (:fredw) 2017-08-04 07:33:55 PDT
Comment on attachment 316959 [details]
Patch (fixedPositionRect ScrollingTreeOverflowScrollingNodeIOS::updateChildNodesAfterScroll)

This part has been handled in bug 175135.