Bug 154399 - Position fixed is buggy with overflow:auto scrolling inside iframes
Summary: Position fixed is buggy with overflow:auto scrolling inside iframes
Status: RESOLVED FIXED
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
: 195729 (view as bug list)
Depends on: 175135 176243 179946
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-18 10:35 PST by Dima Voytenko
Modified: 2019-06-06 09:50 PDT (History)
18 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
patch (16.93 KB, patch)
2019-06-06 07:43 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (16.76 KB, patch)
2019-06-06 07:57 PDT, Antti Koivisto
fred.wang: review+
Details | Formatted Diff | Diff
patch (16.80 KB, patch)
2019-06-06 08:56 PDT, Antti Koivisto
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch (16.82 KB, patch)
2019-06-06 08:57 PDT, Antti Koivisto
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.
Comment 25 Frédéric Wang (:fredw) 2018-11-05 01:55:59 PST
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.
Comment 26 Dima Voytenko 2018-11-05 18:21:39 PST
Same sample: https://output.jsbin.com/zacikor/quiet. Takes some effort, but it's reproduces with some regularity.
Comment 27 Frédéric Wang (:fredw) 2018-11-06 00:34:28 PST
(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.
Comment 28 James 2018-11-19 21:55:01 PST
Hello. I'm experiencing something very similar to this. Has this been fixed in the latest iOS 12.1?
Comment 29 Frédéric Wang (:fredw) 2018-11-20 01:25:44 PST
(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.
Comment 30 James 2018-11-20 15:00:34 PST
(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
Comment 31 Antti Koivisto 2019-06-06 06:58:02 PDT
The test case still flickers.
Comment 32 Frédéric Wang (:fredw) 2019-06-06 07:07:42 PDT
(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.
Comment 33 Antti Koivisto 2019-06-06 07:43:00 PDT
Created attachment 371495 [details]
patch
Comment 34 Frédéric Wang (:fredw) 2019-06-06 07:45:37 PDT
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?
Comment 35 Antti Koivisto 2019-06-06 07:57:04 PDT
Created attachment 371497 [details]
patch
Comment 36 Frédéric Wang (:fredw) 2019-06-06 08:07:27 PDT
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
Comment 37 Antti Koivisto 2019-06-06 08:56:45 PDT
Created attachment 371499 [details]
patch

patch
Comment 38 WebKit Commit Bot 2019-06-06 08:57:51 PDT
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
Comment 39 Antti Koivisto 2019-06-06 08:57:53 PDT
Created attachment 371500 [details]
patch
Comment 40 WebKit Commit Bot 2019-06-06 09:39:40 PDT
Comment on attachment 371500 [details]
patch

Clearing flags on attachment: 371500

Committed r246156: <https://trac.webkit.org/changeset/246156>
Comment 41 WebKit Commit Bot 2019-06-06 09:39:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Antti Koivisto 2019-06-06 09:50:21 PDT
*** Bug 195729 has been marked as a duplicate of this bug. ***