Bug 158342 - Overlay with -webkit-overflow-scrolling:touch doesn't become scrollable after added text makes it taller
Summary: Overlay with -webkit-overflow-scrolling:touch doesn't become scrollable after...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari 9
Hardware: iPhone / iPad iOS 10
: P2 Normal
Assignee: Simon Fraser (smfr)
URL: http://output.jsbin.com/tuxahu/quiet
Keywords: InRadar
: 150974 (view as bug list)
Depends on: 90342 191398
Blocks: 159753
  Show dependency treegraph
 
Reported: 2016-06-03 00:04 PDT by Chris Rebert
Modified: 2019-05-02 16:19 PDT (History)
15 users (show)

See Also:


Attachments
Simple testcase (745 bytes, text/html)
2018-10-26 00:17 PDT, Frédéric Wang (:fredw)
no flags Details
Repro case (position: fixed) (968 bytes, text/html)
2018-10-31 04:05 PDT, Frédéric Wang (:fredw)
no flags Details
Test case (position: absolute) (971 bytes, text/html)
2018-10-31 04:06 PDT, Frédéric Wang (:fredw)
no flags Details
Repro case (translate transform) (1.16 KB, text/html)
2018-11-02 08:15 PDT, tahoon
no flags Details
Patch (749 bytes, patch)
2018-11-05 07:40 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.48 MB, application/zip)
2018-11-05 08:48 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (3.36 MB, application/zip)
2018-11-05 09:24 PST, EWS Watchlist
no flags Details
Patch (9.21 KB, patch)
2018-11-05 09:45 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (7.42 KB, patch)
2018-11-12 17:53 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (17.80 KB, patch)
2018-11-14 03:09 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (19.87 KB, patch)
2018-11-15 11:42 PST, 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 Chris Rebert 2016-06-03 00:04:38 PDT
Steps to reproduce:
1. Open http://output.jsbin.com/tuxahu/quiet in iOS 9.3 Safari.
2. Tap the "Go" button.
3. Observe that a modal dialog appears with text indicating that its content is loading.
4. Wait for the "loading" text to go away and be replaced with lot of other text,
     causing the modal to become taller than the screen.
5. Attempt to scroll the modal downward to read the rest of the text.

Actual result:
The underlying <body> below the modal scrolls temporarily (despite being overflow:hidden),
and the modal within the topmost position:fixed "layer" doesn't move at all.

Expected result:
The modal should move upward, revealing more of the text.

Original Bootstrap issue:
https://github.com/twbs/bootstrap/issues/17695

Further details:
This sounds similar to https://bugs.webkit.org/show_bug.cgi?id=150974
The bug seems to be related to `-webkit-overflow-scrolling:touch`.
Setting `-webkit-overflow-scrolling:auto` on the `.modal` fixes the problem.
I also observed that merely toggling the `.modal-open .modal { overflow-y: auto; }` style
off and back on again in Safari DevTools (while connected to the Simulator,
and leaving `-webkit-overflow-scrolling:touch` intact) is sufficient to cause the modal to become scrollable again.
Comment 1 Radar WebKit Bug Importer 2016-06-06 10:51:09 PDT
<rdar://problem/26652811>
Comment 2 Chris Rebert 2016-11-30 08:58:52 PST
Had to disable buttery-smooth Bootstrap modal scrolling in iOS because of this:
https://github.com/twbs/bootstrap/commit/1ca6c9d7f1d15017c549dd1375ed98bf64404b33
Comment 3 Simon Fraser (smfr) 2017-11-09 18:46:35 PST
I see the same behavior with http://output.jsbin.com/tuxahu/quiet on macOS in Safari and Chrome, as well as on iOS. That is that when the content of the modal becomes long (taller than the view size), then the <div class="modal-dialog"> itself becomes taller, rather than it starting to have overflow scrolling. So I don't think there's a bug here (or the testcase doesn't reproduce the bug).
Comment 4 Ali G 2018-10-23 18:56:57 PDT
Also running into this for simple scrollable modal dialogs.

See easy repro on https://s.codepen.io/aghassemi/debug/pxxaJj 

If content was initially tall enough to overflow, no issue. Only happens if content overflows "later"

We are looking for a decent work-around for AMP's overlay (amp-lightbox), any suggestions? (I am forced to put an initially visibility:hidden long 1px div to force the overlay to be scrollable from the beginning :( )
Comment 5 Ali G 2018-10-24 09:44:33 PDT
Simon,

Any chance this can become a P0/P1?

It really comes down to: "If a scrollable container does not initially have enough content to overflow, when it overflows later, it won't be scrollable" and that's a pretty bad bug IMO. 

We can try to detect when content starts overflowing and toggle '-webkit-overflow-scrolling' on and off which forces the scrolling logic to run again and fixes the issue but that's hard and non-performant thing to do in JS land.

Repro: https://s.codepen.io/aghassemi/debug/pxxaJj per previous comment
Comment 6 Frédéric Wang (:fredw) 2018-10-26 00:17:03 PDT
Created attachment 353160 [details]
Simple testcase

This is a very simple testcase based on the bug description. However, when the content becomes taller, the div indeed becomes scrollable on iOS. So there might be something else involved here...
Comment 7 Frédéric Wang (:fredw) 2018-10-31 04:05:32 PDT
Created attachment 353483 [details]
Repro case (position: fixed)
Comment 8 Frédéric Wang (:fredw) 2018-10-31 04:06:05 PDT
Created attachment 353484 [details]
Test case (position: absolute)
Comment 9 Frédéric Wang (:fredw) 2018-10-31 04:36:26 PDT
Attachment 353483 [details] is a reduced version of Ali's testcase. The first div already has tall content at page load and is indeed scrollable. The second div should become scrollable after 2 seconds but that's not the case on iOS.

Attachment 353484 [details] is exactly the same but with position: absolute instead of position: fixed. The overflow nodes are all scrollable, as expected.

So it seems both dynamic resizing and "position: fixed" are important here.

Note: In general scroll chaining might make things confusing, see bug 176454 comment 7.
Comment 10 Frédéric Wang (:fredw) 2018-11-01 09:37:11 PDT
(In reply to Frédéric Wang (:fredw) from comment #7)
> Created attachment 353483 [details]
> Repro case (position: fixed)

In debug build this is hitting the ASSERT in ScrollingTreeScrollingNodeDelegateIOS. From a quick debug, it seems the resize is triggering update of the scrolling tree but the m_scrollingLayer of the RenderLayerBacking remains null. So there is clearly something wrong with the update.
Comment 11 tahoon 2018-11-02 08:15:49 PDT
Created attachment 353703 [details]
Repro case (translate transform)

Scrolling box should take into account transforms of descendants but the element is not scrollable as expected.
Comment 12 tahoon 2018-11-02 08:26:29 PDT
I managed to repro this without using "position: fixed" in attachment 353703 [details]. The symptom is the same, but I am not sure if it is the same root cause. I've tested on devices as early as iOS 9.3.5. Reproduces up to latest version.

Does not repro if top and left CSS properties are used to position the child element instead of translate transform.
Comment 13 Frédéric Wang (:fredw) 2018-11-05 05:59:11 PST
(In reply to tahoon from comment #12)
> I managed to repro this without using "position: fixed" in attachment 353703 [details]
> [details]. The symptom is the same, but I am not sure if it is the same root
> cause. I've tested on devices as early as iOS 9.3.5. Reproduces up to latest
> version.
> 
> Does not repro if top and left CSS properties are used to position the child
> element instead of translate transform.

Great, thanks for the test case!

As I see the error is very similar, both test cases hit the following code where RenderLayerBacking's scrolling layer is unset. This is making the UIProcess hit the ASSERT in ScrollingTreeScrollingNodeDelegateIOS in debug build.

#0	0x000000072f310a96 in WebCore::RenderLayerCompositor::updateScrollCoordinatedLayer(WebCore::RenderLayer&, WTF::OptionSet<WebCore::LayerScrollCoordinationRole>, WTF::OptionSet<WebCore::RenderLayerCompositor::ScrollingNodeChangeFlags>) at /Users/fred/WebKit/Source/WebCore/./rendering/RenderLayerCompositor.cpp:3848
#1	0x000000072f2f9b91 in WebCore::RenderLayerCompositor::updateScrollCoordinatedStatus(WebCore::RenderLayer&, WTF::OptionSet<WebCore::RenderLayerCompositor::ScrollingNodeChangeFlags>) at /Users/fred/WebKit/Source/WebCore/./rendering/RenderLayerCompositor.cpp:3551
#2	0x000000072f2f61fb in WebCore::RenderLayerBacking::updateGeometry() at /Users/fred/WebKit/Source/WebCore/./rendering/RenderLayerBacking.cpp:1226
#3	0x000000072f2f39eb in WebCore::RenderLayerCompositor::updateCompositingDescendantGeometry(WebCore::RenderLayer&, WebCore::RenderLayer&) at /Users/fred/WebKit/Source/WebCore/./rendering/RenderLayerCompositor.cpp:1841
#4	0x000000072f2f3bbe in WebCore::RenderLayerCompositor::updateCompositingDescendantGeometry(WebCore::RenderLayer&, WebCore::RenderLayer&) at /Users/fred/WebKit/Source/WebCore/./rendering/RenderLayerCompositor.cpp:1864
#5	0x000000072f2f3bbe in WebCore::RenderLayerCompositor::updateCompositingDescendantGeometry(WebCore::RenderLayer&, WebCore::RenderLayer&) at /Users/fred/WebKit/Source/WebCore/./rendering/RenderLayerCompositor.cpp:1864
#6	0x000000072f2c9fe4 in WebCore::RenderLayerBacking::updateAfterLayout(WTF::OptionSet<WebCore::RenderLayerBacking::UpdateAfterLayoutFlags>) at /Users/fred/WebKit/Source/WebCore/./rendering/RenderLayerBacking.cpp:655
#7	0x000000072f2c892b in WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) at /Users/fred/WebKit/Source/WebCore/./rendering/RenderLayer.cpp:907
#8	0x000000072f2c7fbb in WebCore::RenderLayer::updateLayerPositionsAfterLayout(WebCore::RenderLayer const*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) at /Users/fred/WebKit/Source/WebCore/./rendering/RenderLayer.cpp:805
#9	0x000000072eacb586 in WebCore::FrameView::didLayout(WTF::WeakPtr<WebCore::RenderElement>) at /Users/fred/WebKit/Source/WebCore/./page/FrameView.cpp:1301
#10	0x000000072eac11da in WebCore::FrameViewLayoutContext::layout() at /Users/fred/WebKit/Source/WebCore/./page/FrameViewLayoutContext.cpp:237
#11	0x000000072eab80cc in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive() at /Users/fred/WebKit/Source/WebCore/./page/FrameView.cpp:4313
#12	0x0000000101774025 in WebKit::WebPage::layoutIfNeeded() at /Users/fred/WebKit/Source/WebKit/WebProcess/WebPage/WebPage.cpp:1461
Comment 14 Frédéric Wang (:fredw) 2018-11-05 07:40:19 PST
Created attachment 353864 [details]
Patch

This patch fixes the test cases attached to this bug (and the ASSERT) by ensuring the scrolling layer is properly set. Let's see if I can come up with some tests.
Comment 15 EWS Watchlist 2018-11-05 08:48:33 PST
Comment on attachment 353864 [details]
Patch

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

New failing tests:
compositing/plugins/large-to-small-composited-plugin.html
Comment 16 EWS Watchlist 2018-11-05 08:48:35 PST
Created attachment 353866 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 17 EWS Watchlist 2018-11-05 09:24:53 PST
Comment on attachment 353864 [details]
Patch

Attachment 353864 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9865696

New failing tests:
compositing/plugins/large-to-small-composited-plugin.html
Comment 18 EWS Watchlist 2018-11-05 09:24:55 PST
Created attachment 353869 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 19 Frédéric Wang (:fredw) 2018-11-05 09:45:19 PST
Created attachment 353880 [details]
Patch
Comment 20 Frédéric Wang (:fredw) 2018-11-05 09:46:40 PST
@Simon: What do you think of this? This patch makes all the test cases pass but I'm not sure it's really optimal.
Comment 21 Simon Fraser (smfr) 2018-11-05 11:27:11 PST
Comment on attachment 353880 [details]
Patch

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

> LayoutTests/fast/scrolling/ios/update-scroll-coordinated-status.html:5
> +    <title>Update scroll coordinated status</title>
> +    <meta charset="utf-8"/>

You can remove these.

> LayoutTests/fast/scrolling/ios/update-scroll-coordinated-status.html:14
> +	 position: fixed;
> +         width: 200px;
> +         height: 200px;
> +         overflow: auto;
> +         -webkit-overflow-scrolling: touch;
> +         border: 5px solid orange;
> +         margin: 5px;

Bad indentation.

> LayoutTests/fast/scrolling/ios/update-scroll-coordinated-status.html:23
> +      .scrollable-content {
> +        width: 100px;
> +        height: 100px;
> +        background: linear-gradient(135deg, yellow, cyan);
> +      }
> +      .overflowing {
> +        height: 400px;
> +      }

CSS should have 4-space indentation
Comment 22 Frédéric Wang (:fredw) 2018-11-06 00:20:05 PST
Committed r237849: <https://trac.webkit.org/changeset/237849>
Comment 23 Simon Fraser (smfr) 2018-11-07 14:20:16 PST
This causes the MotionMark test to not paint anything, and therefore report erroneously high results (https://browserbench.org/MotionMark1.1/).
Comment 24 Simon Fraser (smfr) 2018-11-07 14:25:35 PST
Rolled out.
Comment 25 Simon Fraser (smfr) 2018-11-12 17:53:41 PST
Created attachment 354616 [details]
Patch
Comment 26 Simon Fraser (smfr) 2018-11-12 17:54:43 PST
This patch uses the new compositing dirty bits to update the compositing state after layout, which removes any possibility of triggering updates with stale geometry (which is why I think the previous patch failed).
Comment 27 Simon Fraser (smfr) 2018-11-12 18:15:25 PST
*** Bug 150974 has been marked as a duplicate of this bug. ***
Comment 28 Frédéric Wang (:fredw) 2018-11-13 02:54:13 PST
Comment on attachment 354616 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:3579
> +#endif

This version fixes repro cases from comment 0, comment 4, attachment 353483 [details] but the issue remains for attachment 353703 [details].

> LayoutTests/fast/scrolling/ios/become-scrollable-on-content-resize.html:56
> +      </div>

OK, maybe I should have tried harder to integrate the repro case from attachment 353703 [details], sorry about that :-/
Comment 29 Frédéric Wang (:fredw) 2018-11-13 06:56:06 PST
Comment on attachment 354616 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:3579
>> +#endif
> 
> This version fixes repro cases from comment 0, comment 4, attachment 353483 [details] but the issue remains for attachment 353703 [details].

Compared to attachment 353880 [details], this is lacking the call to RenderLayerBacking::updateConfiguration() in order to instantiate the m_scrollingLayer. Not sure why it was enough for most of the repro cases, but for attachment 353703 [details] it seems we really need to call setNeedsCompositingConfigurationUpdate() if we really want RenderLayerCompositor::updateBackingAndHierarchy to do the necessary work.

>> LayoutTests/fast/scrolling/ios/become-scrollable-on-content-resize.html:56
>> +      </div>
> 
> OK, maybe I should have tried harder to integrate the repro case from attachment 353703 [details], sorry about that :-/

We need to convert this repro case into a regression test.
Comment 30 Simon Fraser (smfr) 2018-11-13 07:03:48 PST
(In reply to Frédéric Wang (:fredw) from comment #29)
> Comment on attachment 354616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354616&action=review
> 
> >> Source/WebCore/rendering/RenderLayer.cpp:3579
> >> +#endif
> > 
> > This version fixes repro cases from comment 0, comment 4, attachment 353483 [details] but the issue remains for attachment 353703 [details].
> 
> Compared to attachment 353880 [details], this is lacking the call to
> RenderLayerBacking::updateConfiguration() in order to instantiate the
> m_scrollingLayer. Not sure why it was enough for most of the repro cases,
> but for attachment 353703 [details] it seems we really need to call
> setNeedsCompositingConfigurationUpdate() if we really want
> RenderLayerCompositor::updateBackingAndHierarchy to do the necessary work.
> 
> >> LayoutTests/fast/scrolling/ios/become-scrollable-on-content-resize.html:56
> >> +      </div>
> > 
> > OK, maybe I should have tried harder to integrate the repro case from attachment 353703 [details], sorry about that :-/
> 
> We need to convert this repro case into a regression test.

Ah, I guess that's why your test case had position:fixed. I was confused about that.

We need tests for both strollers that are already composited, and those that are not, and the content size change. I suggest not using position:fixed to make things composited; that just complicates the test. The best compositing trigger that doesn't have side effects is to put a composited div behind the scroller.

You should be able to just add a call to setNeedsCompositingConfigurationUpdate() in the if (isComposited()) clause.
Comment 31 Frédéric Wang (:fredw) 2018-11-13 07:14:40 PST
(In reply to Simon Fraser (smfr) from comment #30)
> Ah, I guess that's why your test case had position:fixed. I was confused
> about that.
> We need tests for both strollers that are already composited, and those that
> are not, and the content size change. I suggest not using position:fixed to
> make things composited; that just complicates the test. The best compositing
> trigger that doesn't have side effects is to put a composited div behind the
> scroller.
> 
> You should be able to just add a call to
> setNeedsCompositingConfigurationUpdate() in the if (isComposited()) clause.

To be honest, I just tried to reduce https://s.codepen.io/aghassemi/debug/pxxaJj and I was not able to reproduce it without keeping the position: fixed. OK I'll try to rewrite tests and to add the missing setNeedsCompositingConfigurationUpdate().

So I assume you are ok if I assign myself the bug again ;-)
Comment 32 Simon Fraser (smfr) 2018-11-13 09:46:31 PST
The current patch works for composited overflow, because the new compositing code calls setNeedsCompositingConfigurationUpdate() from setContentsNeedDisplayInRect(). However, adding an explicit setNeedsCompositingConfigurationUpdate() from updateScrollInfoAfterLayout() is probably a good idea.
Comment 33 Simon Fraser (smfr) 2018-11-13 09:48:12 PST
I agree that we also need a test case like https://bug-158342-attachments.webkit.org/attachment.cgi?id=353703, which triggers an assertion that none of the existing tests do. It has nested overflow divs.
Comment 34 Frédéric Wang (:fredw) 2018-11-14 01:49:27 PST
(In reply to Simon Fraser (smfr) from comment #32)
> The current patch works for composited overflow, because the new compositing
> code calls setNeedsCompositingConfigurationUpdate() from
> setContentsNeedDisplayInRect(). However, adding an explicit
> setNeedsCompositingConfigurationUpdate() from updateScrollInfoAfterLayout()
> is probably a good idea.
(In reply to Simon Fraser (smfr) from comment #33)
> I agree that we also need a test case like
> https://bug-158342-attachments.webkit.org/attachment.cgi?id=353703, which
> triggers an assertion that none of the existing tests do. It has nested
> overflow divs.

Attachment 353483 [details] (position: fixed) used to trigger the same assertion but it does not anymore probably because as you say the new code calls setNeedsCompositingConfigurationUpdate(). So let's forget about position: fixed and just try to have an additional test for the case in attachment 353703 [details].
Comment 35 Frédéric Wang (:fredw) 2018-11-14 03:09:56 PST
Created attachment 354790 [details]
Patch
Comment 36 Frédéric Wang (:fredw) 2018-11-14 03:13:54 PST
(In reply to Frédéric Wang (:fredw) from comment #29)
> > This version fixes repro cases from comment 0, comment 4, attachment 353483 [details] but the issue remains for attachment 353703 [details].
> 
> Compared to attachment 353880 [details], this is lacking the call to
> RenderLayerBacking::updateConfiguration() in order to instantiate the
> m_scrollingLayer. Not sure why it was enough for most of the repro cases,
> but for attachment 353703 [details] it seems we really need to call
> setNeedsCompositingConfigurationUpdate() if we really want
> RenderLayerCompositor::updateBackingAndHierarchy to do the necessary work.

So it turns out that attachment 354616 [details] actually does change anything since most of the test cases are fixed by the refactoring of bug 90342. We only need the call to setNeedsCompositingConfigurationUpdate() in order to fix the issue exhibited by attachment 353703 [details].
Comment 37 Simon Fraser (smfr) 2018-11-14 11:35:55 PST
Comment on attachment 354790 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:3577
> -    if (isComposited())
> +    if (isComposited()) {
>          setNeedsCompositingGeometryUpdate();
> +        setNeedsCompositingConfigurationUpdate();
> +    }

It's odd that this works for a non-composited overflow with -webkit-overflow-scrolling: touch; in my testing, a setNeedsPostLayoutCompositingUpdate() was necessary to trigger compositing (as per my earlier patch).
Comment 38 Frédéric Wang (:fredw) 2018-11-14 11:50:06 PST
Comment on attachment 354790 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:3577
>> +    }
> 
> It's odd that this works for a non-composited overflow with -webkit-overflow-scrolling: touch; in my testing, a setNeedsPostLayoutCompositingUpdate() was necessary to trigger compositing (as per my earlier patch).

Yes, I'm also confused why it works without setNeedsPostLayoutCompositingUpdate() because it was needed in my previous testing too. Do you have a test case that shows it is necessary? I was no longer able to use the test cases on this bug.
Comment 39 Frédéric Wang (:fredw) 2018-11-14 13:40:32 PST
Comment on attachment 354790 [details]
Patch

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

>>> Source/WebCore/rendering/RenderLayer.cpp:3577
>>> +    }
>> 
>> It's odd that this works for a non-composited overflow with -webkit-overflow-scrolling: touch; in my testing, a setNeedsPostLayoutCompositingUpdate() was necessary to trigger compositing (as per my earlier patch).
> 
> Yes, I'm also confused why it works without setNeedsPostLayoutCompositingUpdate() because it was needed in my previous testing too. Do you have a test case that shows it is necessary? I was no longer able to use the test cases on this bug.

I tried again with a fresh build at https://trac.webkit.org/changeset/238191/webkit and repro cases from comment 0, comment 4, attachment 353483 [details] are fixed.  attachment 353703 [details] only needs setNeedsCompositingConfigurationUpdate() to pass.
Comment 40 Ali Juma 2018-11-15 08:48:38 PST
(In reply to Frédéric Wang (:fredw) from comment #39)
> I tried again with a fresh build at
> https://trac.webkit.org/changeset/238191/webkit and repro cases from comment
> 0, comment 4, attachment 353483 [details] are fixed. 

Fwiw, these test cases all work for me too.
Comment 41 Frédéric Wang (:fredw) 2018-11-15 08:52:04 PST
(In reply to Ali Juma from comment #40)
> (In reply to Frédéric Wang (:fredw) from comment #39)
> > I tried again with a fresh build at
> > https://trac.webkit.org/changeset/238191/webkit and repro cases from comment
> > 0, comment 4, attachment 353483 [details] are fixed. 
> 
> Fwiw, these test cases all work for me too.

Thanks for the confirmation Ali! No idea what fixed them...

@smfr: Do you think it's still worth to add the call to setNeedsPostLayoutCompositingUpdate() or is the current patch ok?
Comment 42 Simon Fraser (smfr) 2018-11-15 11:22:33 PST
Comment on attachment 354790 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderLayer.cpp:3577
>>>> +    }
>>> 
>>> It's odd that this works for a non-composited overflow with -webkit-overflow-scrolling: touch; in my testing, a setNeedsPostLayoutCompositingUpdate() was necessary to trigger compositing (as per my earlier patch).
>> 
>> Yes, I'm also confused why it works without setNeedsPostLayoutCompositingUpdate() because it was needed in my previous testing too. Do you have a test case that shows it is necessary? I was no longer able to use the test cases on this bug.
> 
> I tried again with a fresh build at https://trac.webkit.org/changeset/238191/webkit and repro cases from comment 0, comment 4, attachment 353483 [details] are fixed.  attachment 353703 [details] only needs setNeedsCompositingConfigurationUpdate() to pass.

If you load change-scrollability-on-content-resize.html in the iOS sim, and comment out all but the nonCompositedBecomeScrollable case, it does not work with the patch. In fact, you can see this in the layout test result; that last container doesn't have any scrolling layers.
Comment 43 Simon Fraser (smfr) 2018-11-15 11:42:45 PST
Created attachment 354962 [details]
Patch
Comment 44 Frédéric Wang (:fredw) 2018-11-15 12:08:30 PST
Comment on attachment 354962 [details]
Patch

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

OK thanks!

> Source/WebCore/ChangeLog:24
> +        RenderLayerCompositor::updateBackingAndHierarchy will later instantiate

Also setNeedsCompositingConfigurationUpdate then.

> Source/WebCore/rendering/RenderLayerBacking.cpp:-721
> -    // Requires layout.

Is this change intentional?
Comment 45 Simon Fraser (smfr) 2018-11-15 12:34:26 PST
(In reply to Frédéric Wang (:fredw) from comment #44)
> Comment on attachment 354962 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354962&action=review
> 
> OK thanks!
> 
> > Source/WebCore/ChangeLog:24
> > +        RenderLayerCompositor::updateBackingAndHierarchy will later instantiate
> 
> Also setNeedsCompositingConfigurationUpdate then.

Right, will fix the changelog.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:-721
> > -    // Requires layout.
> 
> Is this change intentional?

Yes; drive-by comment removal. The function already asserts that layout is not dirty.
Comment 46 Simon Fraser (smfr) 2018-11-15 21:03:15 PST
https://trac.webkit.org/changeset/238266/webkit