Problem with delayed scroll updates

[Christian Biesinger]

Hi Steve/Stefan/layout-dev!

Consider this testcase:
http://plexode.com/eval3/#s=aekVQXANJVQMbAx1yAXgePQN5GwFHTUZZHJofPU+mAQGVl5kDn6EbT1BPRhwBeg5URk1Hna9ZuVVCU1W2hJ0UUVkBhwFDTVa1pKipqpaYmnydQlZVULaCnRMRyqMDpU/V1Tns7TnUqR0Qcujx8+jy9ANeAA==

The blue border does not include the scrollbar, because the height of
this container does not get updated once we relayout the overflow:auto
layout object.

(It fixes itself if you trigger any layout for the box, e.g. if you
change the width:20px to 30px in devtools)

This is because of a combination of two things:
- For any descendants of a flexbox, we delay the overflow:auto
relayout until later:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/layout/LayoutBlock.cpp&l=830

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp&rcl=1456332062&l=285

- Once we *do* the delayed layouts, we only layout the o:auto object *itself*:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp&rcl=1456332062&l=688

That code is broken because, as the testcase illustrates, when we
add/remove scrollbars (=the only case where this code runs), we
usually have to relayout the ancestor chain.

As far as I can tell, this scroll update delaying is inherently broken
because of this issue, ever since it was checked in. However, it is
also an important performance optimization (as per szager's
investigations).

So, uh... any suggestions for how to fix this? I haven't thought of a
good solution yet.

-Christian

[Steve Kobes]

What is the significance of the layout being "delayed"?  Isn't the problem just that we fail to mark the ancestor chain for relayout?

[Christian Biesinger]

The short answer is yes. The long answer is, when we don't delay
scroll updates, we finish the scrolling updates *during* the parent's
layout and take the final height (incl. scrollbars) into account as
normal. When we do delay it, we are "done" with the layout of the
parent. I don't think it's enough to *mark* the ancestor chain for
layout, we actually have to do the layout. And then we also have to
layout *this again in/around finishDelayUpdateScrollInfo, but I have
no idea how that would work.

-Christian

[Christian Biesinger]

I suppose it would be something like:

if (finishDelayUpdateScrollInfo()) /* we'll have to pass back a bool
indicating whether we did any markings */
layoutFlexItems(false, layoutScope);

and change updateAfterLayout to only mark, and to also mark ancestors

hm... maybe that'll work.

[Stefan Zager]

When I made my attempt to to fix this, I reasoned this way:

As soon as you know that you're going to need a horizontal scrollbar for the innermost div, it should be OK to immediately add the scrollbar height to the block's logical height -- without running another layout -- so that the flex box calculation will take it into account.

When you do the relayout at the end of flexing, it shouldn't matter if the overflow width of the inner div changes for some reason; it *already* has a scrollbar, and its client rect need not change.

Honestly, I'm still unclear on why that strategy did not work, and I haven't taken the time to dig into the bugs that necessitated removing it.  Hopefully, I'll have some time to dig into it once the IntersectionObserver stuff cools off (hopefully next week).

Stefan

[Christian Biesinger]

Maybe if I undo my misguided change to improve your change
(https://codereview.chromium.org/1504923010/), add back your change
for heights as suggested (with an added updateLogicalHeight), we can
mostly make that approach work (though not for widths). I would be
worried about the widths because you have added scrollbars but didn't
update anything else; you run the risk of a negative
contentLogicalWidth(). But you can't just add the scrollbar to the
width because typically that's wrong. But if you add a flooring of
contentWidth at zero, and now that
https://codereview.chromium.org/1670643002/ landed, this may have
worked.

But mostly that approach really worries me because you have the layout
tree in an inconsistent state. Am I overly worried about it?

-Christian

[Christian Biesinger]

I uploaded a patch implementing this to
https://codereview.chromium.org/1734203002

Thoughts?

-Christian

On Wed, Feb 24, 2016 at 7:13 PM, Christian Biesinger