Bug 194433 - Compute fixed position rect based on scrolling tree view of the current scroll position
Summary: Compute fixed position rect based on scrolling tree view of the current scrol...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 195050
Blocks: 194289
  Show dependency treegraph
 
Reported: 2019-02-08 04:47 PST by Antti Koivisto
Modified: 2019-03-29 03:15 PDT (History)
7 users (show)

See Also:


Attachments
patch (45.75 KB, patch)
2019-02-08 05:20 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
test case with busy main thread (45.75 KB, patch)
2019-02-08 05:23 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
test case with busy main thread (5.98 KB, text/html)
2019-02-08 05:25 PST, Antti Koivisto
no flags Details
patch (48.34 KB, patch)
2019-02-08 05:50 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (49.01 KB, patch)
2019-02-08 06:53 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (49.01 KB, patch)
2019-02-08 10:30 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
Patch (tests) (5.33 KB, patch)
2019-02-21 08:05 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (tests) (14.79 KB, patch)
2019-02-26 09:27 PST, 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 Antti Koivisto 2019-02-08 04:47:28 PST
We currently use the scroll position at the last layout and then try to apply adjustments based on layer position.
Comment 1 Antti Koivisto 2019-02-08 05:20:20 PST
Created attachment 361500 [details]
patch
Comment 2 Antti Koivisto 2019-02-08 05:23:27 PST
Created attachment 361501 [details]
test case with busy main thread
Comment 3 Antti Koivisto 2019-02-08 05:25:50 PST
Created attachment 361502 [details]
test case with busy main thread
Comment 4 Antti Koivisto 2019-02-08 05:50:02 PST
Created attachment 361503 [details]
patch
Comment 5 Antti Koivisto 2019-02-08 06:53:43 PST
Created attachment 361504 [details]
patch
Comment 6 EWS Watchlist 2019-02-08 06:55:36 PST
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.
Comment 7 Simon Fraser (smfr) 2019-02-08 10:00:23 PST
Does this fix <rdar://problem/47755552>?
Comment 8 Antti Koivisto 2019-02-08 10:30:10 PST
Created attachment 361515 [details]
patch
Comment 9 Antti Koivisto 2019-02-09 00:34:32 PST
(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.
Comment 10 Simon Fraser (smfr) 2019-02-11 11:41:03 PST
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?
Comment 11 Frédéric Wang (:fredw) 2019-02-21 07:39:17 PST
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);
Comment 12 Frédéric Wang (:fredw) 2019-02-21 08:05:27 PST
Created attachment 362606 [details]
Patch (tests)

Just moving my old tests here, so that they are not lost.
Comment 13 Frédéric Wang (:fredw) 2019-02-26 09:27:24 PST
Created attachment 362990 [details]
Patch (tests)

Tentative to split new tests into independent files.
Comment 14 Simon Fraser (smfr) 2019-03-26 09:56:37 PDT
Comment on attachment 362990 [details]
Patch (tests)

These all pass other than fast/scrolling/ios/programmatic-scroll-iframe-012.html which times out.
Comment 15 Frédéric Wang (:fredw) 2019-03-27 08:01:39 PDT
(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?
Comment 16 Frédéric Wang (:fredw) 2019-03-29 03:14:23 PDT
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()
Comment 17 Frédéric Wang (:fredw) 2019-03-29 03:15:33 PDT
(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.