Bug 195608

Summary: Scrolling tree should reposition non-stacking order descendents of overflow:scroll - step 1.
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, fred.wang, koivisto, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=195710
Attachments:
Description Flags
Mostly ready patch, needs tests
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews105 for mac-highsierra-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Step 1: add positioning nodes zalan: review+

Description Simon Fraser (smfr) 2019-03-11 23:21:29 PDT
Scrolling tree should reposition non-stacking order descendents of overflow:scroll
Comment 1 Simon Fraser (smfr) 2019-03-11 23:28:49 PDT
Created attachment 364356 [details]
Mostly ready patch, needs tests
Comment 2 Simon Fraser (smfr) 2019-03-11 23:29:44 PDT
rdar://problem/11642295
Comment 3 EWS Watchlist 2019-03-11 23:34:04 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-03-12 00:47:28 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-03-12 00:47:30 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-03-12 08:40:30 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-03-12 08:40:31 PDT Comment hidden (obsolete)
Comment 8 Simon Fraser (smfr) 2019-03-12 11:10:14 PDT
Created attachment 364412 [details]
Patch
Comment 9 EWS Watchlist 2019-03-12 11:13:51 PDT Comment hidden (obsolete)
Comment 10 Antti Koivisto 2019-03-12 12:22:04 PDT
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
Comment 11 EWS Watchlist 2019-03-12 12:49:22 PDT Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-03-12 12:49:24 PDT Comment hidden (obsolete)
Comment 13 EWS Watchlist 2019-03-12 14:16:07 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2019-03-12 14:16:10 PDT Comment hidden (obsolete)
Comment 15 Simon Fraser (smfr) 2019-03-12 20:26:01 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2019-03-12 20:29:19 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2019-03-12 21:44:14 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2019-03-12 21:44:15 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2019-03-13 01:37:08 PDT Comment hidden (obsolete)
Comment 20 EWS Watchlist 2019-03-13 01:37:10 PDT Comment hidden (obsolete)
Comment 21 Frédéric Wang (:fredw) 2019-03-13 03:37:43 PDT
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
Comment 22 Simon Fraser (smfr) 2019-03-13 10:20:08 PDT
Created attachment 364543 [details]
Step 1: add positioning nodes
Comment 23 Simon Fraser (smfr) 2019-03-13 10:20:33 PDT
Comment on attachment 364543 [details]
Step 1: add positioning nodes

I decided to break up the patch.
Comment 24 zalan 2019-03-13 10:43:35 PDT
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???
Comment 25 Simon Fraser (smfr) 2019-03-13 15:35:08 PDT
https://trac.webkit.org/changeset/242913/webkit