Bug 195608 - Scrolling tree should reposition non-stacking order descendents of overflow:scroll - step 1.
Summary: Scrolling tree should reposition non-stacking order descendents of overflow:s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-11 23:21 PDT by Simon Fraser (smfr)
Modified: 2019-03-13 20:34 PDT (History)
7 users (show)

See Also:


Attachments
Mostly ready patch, needs tests (93.62 KB, patch)
2019-03-11 23:28 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.60 MB, application/zip)
2019-03-12 00:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.62 MB, application/zip)
2019-03-12 08:40 PDT, Build Bot
no flags Details
Patch (104.58 KB, patch)
2019-03-12 11:10 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.80 MB, application/zip)
2019-03-12 12:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.81 MB, application/zip)
2019-03-12 14:16 PDT, Build Bot
no flags Details
Patch (122.45 KB, patch)
2019-03-12 20:26 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.84 MB, application/zip)
2019-03-12 21:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.52 MB, application/zip)
2019-03-13 01:37 PDT, Build Bot
no flags Details
Step 1: add positioning nodes (83.33 KB, patch)
2019-03-13 10:20 PDT, Simon Fraser (smfr)
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 2019-03-11 23:34:04 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2019-03-12 00:47:28 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2019-03-12 00:47:30 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-03-12 08:40:30 PDT Comment hidden (obsolete)
Comment 7 Build Bot 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 Build Bot 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 Build Bot 2019-03-12 12:49:22 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2019-03-12 12:49:24 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2019-03-12 14:16:07 PDT Comment hidden (obsolete)
Comment 14 Build Bot 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 Build Bot 2019-03-12 20:29:19 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2019-03-12 21:44:14 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2019-03-12 21:44:15 PDT Comment hidden (obsolete)
Comment 19 Build Bot 2019-03-13 01:37:08 PDT Comment hidden (obsolete)
Comment 20 Build Bot 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