Bug 158342

<rdar://problem/26652811>

Had to disable buttery-smooth Bootstrap modal scrolling in iOS because of this: https://github.com/twbs/bootstrap/commit/1ca6c9d7f1d15017c549dd1375ed98bf64404b33

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).

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 :( )

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

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...

Created attachment 353483 [details] Repro case (position: fixed)

Created attachment 353484 [details] Test case (position: absolute)

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.

(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.

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.

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.

(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

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 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

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 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

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

Created attachment 353880 [details] Patch

@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 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

Committed r237849: <https://trac.webkit.org/changeset/237849>

This causes the MotionMark test to not paint anything, and therefore report erroneously high results (https://browserbench.org/MotionMark1.1/).

Rolled out.

Created attachment 354616 [details] Patch

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).

*** Bug 150974 has been marked as a duplicate of this bug. ***

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 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.

(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.

(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 ;-)

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.

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.

(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].

Created attachment 354790 [details] Patch

(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 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 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 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.

(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.

(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 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.

Created attachment 354962 [details] Patch

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?

(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.