Bug 195608 – Scrolling tree should reposition non-stacking order descendents of overflow:scroll - step 1.

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

[Simon Fraser (smfr)]

Scrolling tree should reposition non-stacking order descendents of overflow:scroll

[Simon Fraser (smfr)]

Created attachment 364356 [details]
Mostly ready patch, needs tests

[Simon Fraser (smfr)]

rdar://problem/11642295

[EWS Watchlist]

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


ERROR: Source/WebCore/page/scrolling/mac/ScrollingTreeOverflowScrollingNodeMac.h:53:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:314:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:315:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:318:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:319:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 41 files


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

[EWS Watchlist]

Comment on attachment 364356 [details]
Mostly ready patch, needs tests

Attachment 364356 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11469647

New failing tests:
scrollingcoordinator/scrolling-tree/lose-scrolling-node-parent.html
scrollingcoordinator/scrolling-tree/reparent-with-layer-removal.html
scrollingcoordinator/scrolling-tree/remove-scrolling-role.html
scrollingcoordinator/scrolling-tree/reparent-across-compositing-layers.html
scrollingcoordinator/scrolling-tree/gain-scrolling-node-parent.html
scrollingcoordinator/scrolling-tree/overflow-in-fixed.html

[EWS Watchlist]

Created attachment 364365 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 364356 [details]
Mostly ready patch, needs tests

Attachment 364356 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11474453

New failing tests:
fast/scrolling/ios/change-scrollability-on-content-resize-nested.html
fast/scrolling/ios/change-scrollability-on-content-resize.html
scrollingcoordinator/scrolling-tree/lose-scrolling-node-parent.html
scrollingcoordinator/scrolling-tree/reparent-with-layer-removal.html
scrollingcoordinator/scrolling-tree/remove-scrolling-role.html
scrollingcoordinator/scrolling-tree/coordinated-frame-lose-scrolling-ancestor.html
scrollingcoordinator/scrolling-tree/reparent-across-compositing-layers.html
scrollingcoordinator/scrolling-tree/coordinated-frame-in-fixed.html
scrollingcoordinator/scrolling-tree/coordinated-frame.html
scrollingcoordinator/scrolling-tree/gain-scrolling-node-parent.html
scrollingcoordinator/scrolling-tree/overflow-in-fixed.html
scrollingcoordinator/scrolling-tree/coordinated-frame-gain-scrolling-ancestor.html

[EWS Watchlist]

Created attachment 364389 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Simon Fraser (smfr)]

Created attachment 364412 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/page/scrolling/mac/ScrollingTreeOverflowScrollingNodeMac.h:53:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:321:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:322:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:325:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:326:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 48 files


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

[Antti Koivisto]

Comment on attachment 364412 [details]
Patch

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

Looks good.

> Source/WebCore/page/scrolling/ScrollingTree.cpp:315
> +    auto changeRoot = rootOfScrollingImpactedNodes(changedNode);

auto*

> Source/WebCore/rendering/RenderLayerBacking.cpp:1833
> +    if (roles.contains(ScrollCoordinationRole::Positioning) && m_positioningNodeID) {
> +        LOG(Compositing, "Detaching Positioned node %" PRIu64, m_positioningNodeID);

There is some inconsistency (here and elsewhere) whether these are called "positioning nodes" or "positioned nodes".

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2891
> +            // FIXME: They should be composited alreayd, but may not have nodeIDs yet.

Typo: already

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2911
> +// We also have to be positioned, right?

indent

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3942
> +// FIXME: ordering.

indent

[EWS Watchlist]

Comment on attachment 364412 [details]
Patch

Attachment 364412 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11478860

New failing tests:
scrollingcoordinator/scrolling-tree/positioned-nodes.html
scrollingcoordinator/scrolling-tree/gain-scrolling-node-parent.html
scrollingcoordinator/scrolling-tree/lose-scrolling-node-parent.html

[EWS Watchlist]

Created attachment 364429 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 364412 [details]
Patch

Attachment 364412 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11479512

New failing tests:
fast/scrolling/ios/change-scrollability-on-content-resize-nested.html
scrollingcoordinator/scrolling-tree/positioned-nodes.html
fast/scrolling/ios/change-scrollability-on-content-resize.html
scrollingcoordinator/scrolling-tree/lose-scrolling-node-parent.html
scrollingcoordinator/scrolling-tree/reparent-with-layer-removal.html
scrollingcoordinator/scrolling-tree/remove-scrolling-role.html
scrollingcoordinator/scrolling-tree/coordinated-frame-lose-scrolling-ancestor.html
scrollingcoordinator/scrolling-tree/reparent-across-compositing-layers.html
scrollingcoordinator/scrolling-tree/coordinated-frame-in-fixed.html
scrollingcoordinator/scrolling-tree/coordinated-frame.html
scrollingcoordinator/scrolling-tree/gain-scrolling-node-parent.html
scrollingcoordinator/scrolling-tree/overflow-in-fixed.html
scrollingcoordinator/scrolling-tree/coordinated-frame-gain-scrolling-ancestor.html

[EWS Watchlist]

Created attachment 364445 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Simon Fraser (smfr)]

Created attachment 364502 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/page/scrolling/mac/ScrollingTreeOverflowScrollingNodeMac.h:53:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingConstraints.cpp:106:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:323:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:324:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:327:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.cpp:328:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 59 files


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

[EWS Watchlist]

Comment on attachment 364502 [details]
Patch

Attachment 364502 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11485471

New failing tests:
scrollingcoordinator/scrolling-tree/positioned-nodes.html
compositing/fixed-with-main-thread-scrolling.html
scrollingcoordinator/mac/multiple-fixed.html

[EWS Watchlist]

Created attachment 364510 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 364502 [details]
Patch

Attachment 364502 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11486774

New failing tests:
scrollingcoordinator/ios/nested-fixed-layer-positions.html
scrollingcoordinator/scrolling-tree/positioned-nodes.html
fast/scrolling/ios/change-scrollability-on-content-resize-nested.html
scrollingcoordinator/ios/ui-scroll-fixed.html
fast/scrolling/ios/change-scrollability-on-content-resize.html

[EWS Watchlist]

Created attachment 364519 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

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

Comment on attachment 364502 [details]
Patch

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

This patch is huge!

> Source/WebCore/page/scrolling/ScrollingCoordinatorTypes.h:41
> +    Positioned,

extra comma after "Positioned"

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2879
> +            // FIXME: They should be composited alreayd, but may not have nodeIDs yet.

typo: already

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2893
> +// This should be: is there an overflow scroll in the composting ancestor chain up to the containing block

typo: compositing

[Simon Fraser (smfr)]

Created attachment 364543 [details]
Step 1: add positioning nodes

[Simon Fraser (smfr)]

Comment on attachment 364543 [details]
Step 1: add positioning nodes

I decided to break up the patch.

[zalan]

Comment on attachment 364543 [details]
Step 1: add positioning nodes

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

> Source/WebCore/page/scrolling/ScrollingConstraints.h:38
> +    LayoutConstraints()

= default?

> Source/WebCore/page/scrolling/ScrollingConstraints.h:62
> +    void setLayerPositionAtLastLayout(FloatPoint p) { m_layerPositionAtLastLayout = p; }

p???

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/242913/webkit