Bug 158342

Summary:

Overlay with -webkit-overflow-scrolling:touch doesn't become scrollable after added text makes it taller

Product:

WebKit

Reporter:

Chris Rebert <webkit>

Component:

Layout and Rendering

Assignee:

Simon Fraser (smfr) <simon.fraser>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ajuma, ali.ghassemi, ap, ews-watchlist, firdezerdu, fred.wang, joacim.lowgren, rniwa, rwlbuis, samtsai, simon.fraser, tahoon, thomas.kelly, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

Safari 9

Hardware:

iPhone / iPad

OS:

iOS 10

URL:

http://output.jsbin.com/tuxahu/quiet

Bug Depends on:

90342, 191398

Bug Blocks:

159753

Attachments:

Description	Flags
Simple testcase	none
Repro case (position: fixed)	none
Test case (position: absolute)	none
Repro case (translate transform)	none
Patch	none
Archive of layout-test-results from ews102 for mac-sierra	none
Archive of layout-test-results from ews114 for mac-sierra	none
Patch	none
Patch	none
Patch	none
Patch	zalan: review+

Chris Rebert

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

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
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2016-06-06 10:51:09 PDT

<rdar://problem/26652811>

Chris Rebert

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

Simon Fraser (smfr)

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

Ali G

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

Ali G

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

Frédéric Wang (:fredw)

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

Frédéric Wang (:fredw)

Comment 7 2018-10-31 04:05:32 PDT

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

Frédéric Wang (:fredw)

Comment 8 2018-10-31 04:06:05 PDT

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

Frédéric Wang (:fredw)

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

Frédéric Wang (:fredw)

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

tahoon

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

tahoon

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

Frédéric Wang (:fredw)

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

Frédéric Wang (:fredw)

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

EWS Watchlist

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

EWS Watchlist

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

EWS Watchlist

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

EWS Watchlist

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

Frédéric Wang (:fredw)

Comment 19 2018-11-05 09:45:19 PST

Created attachment 353880 [details] Patch

Frédéric Wang (:fredw)

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

Simon Fraser (smfr)

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

Frédéric Wang (:fredw)

Comment 22 2018-11-06 00:20:05 PST

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

Simon Fraser (smfr)

Comment 23 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/).

Simon Fraser (smfr)

Comment 24 2018-11-07 14:25:35 PST

Rolled out.

Simon Fraser (smfr)

Comment 25 2018-11-12 17:53:41 PST

Created attachment 354616 [details] Patch

Simon Fraser (smfr)

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

Simon Fraser (smfr)

Comment 27 2018-11-12 18:15:25 PST

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

Frédéric Wang (:fredw)

Comment 28 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 :-/

Frédéric Wang (:fredw)

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

Simon Fraser (smfr)

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

Frédéric Wang (:fredw)

Comment 31 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 ;-)

Simon Fraser (smfr)

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

Simon Fraser (smfr)

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

Frédéric Wang (:fredw)

Comment 34 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].

Frédéric Wang (:fredw)

Comment 35 2018-11-14 03:09:56 PST

Created attachment 354790 [details] Patch

Frédéric Wang (:fredw)

Comment 36 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].

Simon Fraser (smfr)

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

Frédéric Wang (:fredw)

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

Frédéric Wang (:fredw)

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

Ali Juma

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

Frédéric Wang (:fredw)

Comment 41 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?

Simon Fraser (smfr)

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

Simon Fraser (smfr)

Comment 43 2018-11-15 11:42:45 PST

Created attachment 354962 [details] Patch

Frédéric Wang (:fredw)

Comment 44 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?

Simon Fraser (smfr)

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

Simon Fraser (smfr)

Comment 46 2018-11-15 21:03:15 PST

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

Note You need to log in before you can comment on or make changes to this bug.