Bug 179946

Summary:

ScrollingStateNode::reconcileLayerPositionForViewportRect is only called on direct children of the root

Product:

WebKit

Reporter:

Frédéric Wang (:fredw) <fred.wang>

Component:

Frames

Assignee:

Frédéric Wang (:fredw) <fred.wang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, darin, dvoytenko, ews-watchlist, jamesr, luiz, simon.fraser, tonikitoo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=180002

Bug Depends on:

Bug Blocks:

154399

Attachments:

Description	Flags
Testcase	none
WIP Patch	none
Patch	none
Patch	simon.fraser: review+

Frédéric Wang (:fredw)

Reported 2017-11-22 07:18:27 PST

Created attachment 327452 [details] Testcase AsyncScrollingCoordinator::reconcileViewportConstrainedLayerPositions has the following comment: // FIXME: We'll have to traverse deeper into the tree at some point. for (auto& child : *children) child->reconcileLayerPositionForViewportRect(viewportRect, action); I'm not sure whether this is affecting bug 154399 but the attached testcase shows that reconcileLayerPositionForViewportRect is not called on the fixed node when one scrolls the main frame.

Attachments
Testcase (1.52 KB, text/html) 2017-11-22 07:18 PST, Frédéric Wang (:fredw)	no flags	Details
WIP Patch (5.36 KB, patch) 2017-11-22 07:27 PST, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (12.46 KB, patch) 2018-01-15 07:03 PST, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Patch (17.20 KB, patch) 2018-01-16 01:44 PST, Frédéric Wang (:fredw)	simon.fraser: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Frédéric Wang (:fredw)

Comment 1 2017-11-22 07:27:20 PST

Created attachment 327453 [details] WIP Patch This is patch to recursively calls reconcileLayerPositionForViewportRect but I'm not sure how to emulate non-programmatic scroll on iOS in order to write a test.

Frédéric Wang (:fredw)

Comment 2 2018-01-15 07:03:12 PST

Created attachment 331334 [details] Patch

Darin Adler

Comment 3 2018-01-15 14:55:18 PST

Comment on attachment 331334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331334&action=review > Source/WebCore/page/scrolling/ScrollingStateNode.cpp:103 > +void ScrollingStateNode::reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction action) ScrollingStateStickyNode::reconcileLayerPositionForViewportRect and ScrollingStateFixedNode::reconcileLayerPositionForViewportRect don’t call through to ScrollingStateNode::reconcileLayerPositionForViewportRect. Can those nodes have children? Is it OK to not call reconcileLayerPositionForViewportRect on those children?

Frédéric Wang (:fredw)

Comment 4 2018-01-16 01:44:11 PST

Created attachment 331376 [details] Patch

Frédéric Wang (:fredw)

Comment 5 2018-01-16 01:46:13 PST

Comment on attachment 331334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331334&action=review >> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:103 >> +void ScrollingStateNode::reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction action) > > ScrollingStateStickyNode::reconcileLayerPositionForViewportRect and ScrollingStateFixedNode::reconcileLayerPositionForViewportRect don’t call through to ScrollingStateNode::reconcileLayerPositionForViewportRect. > > Can those nodes have children? Is it OK to not call reconcileLayerPositionForViewportRect on those children? Yes, I guess sticky/fixed nodes should also call reconcileLayerPositionForViewportRect on their descendants. I modified the code and extended the tests accordingly.

Frédéric Wang (:fredw)

Comment 6 2018-01-16 01:47:20 PST

For the record, here is the diff of the graphic layers of the test before and after the patch: diff --git a/LayoutTests/fast/scrolling/ios/reconcile-layer-position-recursive-expected.txt b/LayoutTests/fast/scrolling/ios/reconcile-layer-position-recursive-expected.txt index fc9b542a67d..1b6f6dbeaba 100644 --- a/LayoutTests/fast/scrolling/ios/reconcile-layer-position-recursive-expected.txt +++ b/LayoutTests/fast/scrolling/ios/reconcile-layer-position-recursive-expected.txt @@ -70,44 +70,48 @@ (contentsScale 2.00) (children 2 (GraphicsLayer - (position 7.00 -993.00) + (position 7.00 28.00) + (approximate position 7.00 28.00) (bounds 110.00 110.00) (contentsOpaque 1) (drawsContent 1) - (visible rect 0.00, 0.00 0.00 x 0.00) - (coverage rect -7.00, 993.00 200.00 x 245.00) + (visible rect 0.00, 0.00 110.00 x 110.00) + (coverage rect -7.00, -28.00 200.00 x 245.00) (intersects coverage rect 1) (contentsScale 2.00) (children 1 (GraphicsLayer (position 55.00 55.00) + (approximate position 55.00 55.00) (bounds 110.00 110.00) (contentsOpaque 1) (drawsContent 1) - (visible rect 0.00, 0.00 0.00 x 0.00) - (coverage rect -62.00, 938.00 200.00 x 245.00) + (visible rect 0.00, 0.00 110.00 x 110.00) + (coverage rect -62.00, -83.00 200.00 x 245.00) (intersects coverage rect 1) (contentsScale 2.00) ) ) ) (GraphicsLayer - (position 105.00 105.00) + (position 105.00 395.00) + (approximate position 105.00 395.00) (bounds 110.00 110.00) (contentsOpaque 1) (drawsContent 1) - (visible rect 0.00, 0.00 95.00 x 110.00) - (coverage rect -105.00, -105.00 200.00 x 245.00) + (visible rect 0.00, 0.00 0.00 x 0.00) + (coverage rect -105.00, -395.00 200.00 x 245.00) (intersects coverage rect 1) (contentsScale 2.00) (children 1 (GraphicsLayer (position 55.00 55.00) + (approximate position 55.00 55.00) (bounds 110.00 110.00) (contentsOpaque 1) (drawsContent 1) - (visible rect 0.00, 0.00 40.00 x 85.00) - (coverage rect -160.00, -160.00 200.00 x 245.00) + (visible rect 0.00, 0.00 0.00 x 0.00) + (coverage rect -160.00, -450.00 200.00 x 245.00) (intersects coverage rect 1) (contentsScale 2.00) )

Frédéric Wang (:fredw)

Comment 7 2018-01-24 02:48:40 PST

@Simon: Can you please review that patch?

Frédéric Wang (:fredw)

Comment 8 2018-01-24 23:59:12 PST

Committed r227596: <https://trac.webkit.org/changeset/227596>

Radar WebKit Bug Importer

Comment 9 2018-01-25 00:00:37 PST

<rdar://problem/36857233>

Note You need to log in before you can comment on or make changes to this bug.