Bug 194433 – Compute fixed position rect based on scrolling tree view of the current scroll position

• Status: NEW
• Component: Scrolling

[Antti Koivisto]

We currently use the scroll position at the last layout and then try to apply adjustments based on layer position.

[Antti Koivisto]

Created attachment 361500 [details]
patch

[Antti Koivisto]

Created attachment 361501 [details]
test case with busy main thread

[Antti Koivisto]

Created attachment 361502 [details]
test case with busy main thread

[Antti Koivisto]

Created attachment 361503 [details]
patch

[Antti Koivisto]

Created attachment 361504 [details]
patch

[EWS Watchlist]

Attachment 361504 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Does this fix <rdar://problem/47755552>?

[Antti Koivisto]

Created attachment 361515 [details]
patch

[Antti Koivisto]

(In reply to Simon Fraser (smfr) from comment #7)
> Does this fix <rdar://problem/47755552>?

No, this patch doesn't affect the main frame. That still only updates the scroll position on layer tree commit (not synchronizing to UIScrollView) and may use stale position.

It is bit of a separate issue and probably better handled separately.

[Simon Fraser (smfr)]

Comment on attachment 361515 [details]
patch

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

> Source/WebCore/ChangeLog:12
> +        This patch removes code that that passes the last committed scroll position via the state tree and modifies its only
> +        client (in ScrollingTreeScrollingNodeDelegateIOS) to use the UIScrollView scrolling position.

How does this work with FixedPositionViewportConstraints::layerPositionForViewportRect((), which uses viewportRectAtLastLayout and layerPositionAtLastLayout, which you haven't changed? ScrollingTreeFrameScrollingNode::fixedPositionRect() uses the committed scroll position, which should match viewportRectAtLastLayout. Or maybe these are never used in the same code path?

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

Comment on attachment 361515 [details]
patch

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

> Source/WebCore/ChangeLog:14
> +        Test by Frederic Wang.

@Antii: Now that you fixed bug 194886, please don't forget to import the three remaining tests from programmatic-scroll-iframe.html, attachment 359723 [details]:

// This checks scrolling behavior for position "fixed" and "sticky".
document.getElementById("positionFixed").contentWindow.window.scrollTo(100, 100);
document.getElementById("positionStickyBegin").contentWindow.window.scrollTo(0, 50);
document.getElementById("positionStickyEnd").contentWindow.window.scrollTo(0, 150);

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

Created attachment 362606 [details]
Patch (tests)

Just moving my old tests here, so that they are not lost.

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

Created attachment 362990 [details]
Patch (tests)

Tentative to split new tests into independent files.

[Simon Fraser (smfr)]

Comment on attachment 362990 [details]
Patch (tests)

These all pass other than fast/scrolling/ios/programmatic-scroll-iframe-012.html which times out.

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

(In reply to Simon Fraser (smfr) from comment #14)
> Comment on attachment 362990 [details]
> Patch (tests)
> 
> These all pass other than
> fast/scrolling/ios/programmatic-scroll-iframe-012.html which times out.

Do you know if it's a bug in our code (e.g. bug 196290) or a bug in the test?
Also, do we still want to take these tests now that we have new APIs?

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

Comment on attachment 362990 [details]
Patch (tests)

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

> LayoutTests/fast/scrolling/ios/programmatic-scroll-iframe-012.html:38
> +            </body>" onload="newFrameLoaded()">

OK, this is the silly mistake: it should be runTest()

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

(In reply to Frédéric Wang (:fredw) from comment #15)
> (In reply to Simon Fraser (smfr) from comment #14)
> > Comment on attachment 362990 [details]
> > Patch (tests)
> > 
> > These all pass other than
> > fast/scrolling/ios/programmatic-scroll-iframe-012.html which times out.
> 
> Do you know if it's a bug in our code (e.g. bug 196290) or a bug in the test?
> Also, do we still want to take these tests now that we have new APIs?

I'll move the tests to bug 196394 as it seems we no longer need Antti's patch to make them pass.