Bug 154399 – Position fixed is buggy with overflow:auto scrolling inside iframes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dima Voytenko]

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.

[Radar WebKit Bug Importer]

<rdar://problem/24742251>

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

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

[Simon Fraser (smfr)]

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.

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

(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?

[Simon Fraser (smfr)]

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.

[Javier Fernandez]

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.

[Dima Voytenko]

Created attachment 311845 [details]
Visual effects with position:fixed

[Dima Voytenko]

Created attachment 311846 [details]
More visual effects with position:fixed

[Dima Voytenko]

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.

[Simon Fraser (smfr)]

Thanks for the data.

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

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?

[Simon Fraser (smfr)]

Yes, I can.

[Simon Fraser (smfr)]

Created attachment 316469 [details]
Testcase that works

This doesn't happen with all position:fixed inside overflow scroll. This attachment works fine.

[Simon Fraser (smfr)]

So both jsbin examples suffer from being inside an <iframe>. Do you have content that shows this bug outside of an <iframe>?

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

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.

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

(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?

[Dima Voytenko]

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.

[Simon Fraser (smfr)]

WebKit does support passive event listener registration.

[Dima Voytenko]

I'll retest with passive then. Not 100% sure that'd help yet.

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

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

[Simon Fraser (smfr)]

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.

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

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

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

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.

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

Comment on attachment 316959 [details]
Patch (fixedPositionRect ScrollingTreeOverflowScrollingNodeIOS::updateChildNodesAfterScroll)

This part has been handled in bug 175135.

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

OK, we now have ScrollingTreeOverflowScrollingNodeIOS::updateChildNodesAfterScroll, ScrollingTreeFrameScrollingNodeIOS::updateLayersAfterAncestorChange and ScrollingStateNode::reconcileLayerPositionForViewportRect properly implemented. Hopefully this bug is fixed now :-)

@Dima: Are you able to provide more test cases? In particular, in bug 176243 comment 9 you mentioned you had more cases that don't involve RAF.

[Dima Voytenko]

Same sample: https://output.jsbin.com/zacikor/quiet. Takes some effort, but it's reproduces with some regularity.

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

(In reply to Dima Voytenko from comment #26)
> Same sample: https://output.jsbin.com/zacikor/quiet. Takes some effort, but
> it's reproduces with some regularity.

Thanks! Indeed, I'm able to reproduce this issue on the simulator but it is fixed by https://trac.webkit.org/changeset/237754

I think we can close this bug then.

[James]

Hello. I'm experiencing something very similar to this. Has this been fixed in the latest iOS 12.1?

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

(In reply to James from comment #28)
> Hello. I'm experiencing something very similar to this. Has this been fixed
> in the latest iOS 12.1?

The usual answer is https://trac.webkit.org/wiki/FAQ#Releases but you can try iOS beta ( https://beta.apple.com/sp/betaprogram/ ). Last time I checked, the fix was not included yet in iOS 12.1.1 beta. I'll try and leave a comment on the bug if I notice any change.

[James]

(In reply to Frédéric Wang (:fredw) from comment #29)
> (In reply to James from comment #28)
> > Hello. I'm experiencing something very similar to this. Has this been fixed
> > in the latest iOS 12.1?
> 
> The usual answer is https://trac.webkit.org/wiki/FAQ#Releases but you can
> try iOS beta ( https://beta.apple.com/sp/betaprogram/ ). Last time I
> checked, the fix was not included yet in iOS 12.1.1 beta. I'll try and leave
> a comment on the bug if I notice any change.

Ok. Thanks for the reply. Will keep an eye on this thread

[Antti Koivisto]

The test case still flickers.

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

(In reply to Antti Koivisto from comment #31)
> The test case still flickers.

Is it a regression from the recent scroll refactoring? Can you please also check the related bug 176243, which is a bit more complex.

[Antti Koivisto]

Created attachment 371495 [details]
patch

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

Comment on attachment 371495 [details]
patch

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

> LayoutTests/scrollingcoordinator/ios/fixed-frame-overflow-swipe.html:41
> +<!--            await UIHelper.immediateScrollElementAtContentPointToOffset(50, 50, 0, 200, scrollUpdatesDisabled);-->

I guess you don't want these comments?

[Antti Koivisto]

Created attachment 371497 [details]
patch

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

Comment on attachment 371497 [details]
patch

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

> Source/WebKit/ChangeLog:13
> +        the scrolling tree. Instead call the new applyScrollingTreeLayerPositions() unconditionally.

applyScrollingTreeLayerPositionsAfterCommit

[Antti Koivisto]

Created attachment 371499 [details]
patch

patch

[WebKit Commit Bot]

Comment on attachment 371499 [details]
patch

Rejecting attachment 371499 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 371499, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/12395961

[Antti Koivisto]

Created attachment 371500 [details]
patch

[WebKit Commit Bot]

Comment on attachment 371500 [details]
patch

Clearing flags on attachment: 371500

Committed r246156: <https://trac.webkit.org/changeset/246156>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

*** Bug 195729 has been marked as a duplicate of this bug. ***