Fully (?) fix overflow: auto with delayed scroll updates

Fully (?) fix overflow: auto with delayed scroll updates

The scroll delaying code, used by Flexbox, never lays out the
parent after the child relayout. This is a problem especially
because the layout can add/remove a horizontal scrollbar,
which changes the height of the element. A vertical scrollbar
has this problem less often, though it sometimes does, especially
in case of flex items. As far as I can tell, this never worked
correctly since the scroll delaying code existed.

This patch changes it so that when we eventually do the scroll info
updates, we mark the ancestor chain for layout (up to the outer flexbox)
and then call layoutFlexItems again, to do the correct layout.

See also the discussion thread at
https://groups.google.com/a/chromium.org/d/topic/layout-dev/e7Wzvxu7qiA/discussion

BUG=579401, 584363
Committed: https://crrev.com/775378f6ac8293e20a393c5203f5431a4a679339
Cr-Commit-Position: refs/heads/master@{#377826}

[cbiesinger, 2016-02-25 20:46:26]

https://codereview.chromium.org/1734203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/css3/flexbox/overflow-auto-resizes-correctly.html
(right):

https://codereview.chromium.org/1734203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/css3/flexbox/overflow-auto-resizes-correctly.html:22:
max-height: 200px;
I made this change because I am no longer sure that the testcase was correct. I
don't think a reasonable flexbox implementation can produce the expected result,
because to get that result you need to apply the stretched height from the
flexbox layout first, then lay out the object and notice the overflow.

At any rate, the changed CSS still tests what the original was meant to test,
afaict.

IE 11 and FF (yesterday's nightly) did not match us here.

[cbiesinger, 2016-02-25 20:46:39]

The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-25 20:49:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1734203002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1734203002/1

[cbiesinger, 2016-02-25 20:57:44]

Description was changed from

==========
Fully (?) fix scrolling

BUG=579401,589451,584363
==========

to

==========
Fully (?) fix overflow: auto with delayed scroll updates

BUG=579401,589451,584363
==========

[cbiesinger, 2016-02-25 20:57:44]

cbiesinger@chromium.org changed reviewers:
+ leviw@chromium.org, skobes@chromium.org, szager@google.com

[cbiesinger, 2016-02-25 21:07:36]

Description was changed from

==========
Fully (?) fix overflow: auto with delayed scroll updates

BUG=579401,589451,584363
==========

to

==========
Fully (?) fix overflow: auto with delayed scroll updates

BUG=579401,584363
==========

[szager1, 2016-02-25 21:08:08]

szager@chromium.org changed reviewers:
+ szager@chromium.org

[szager1, 2016-02-25 21:08:09]

Wow, this is WAY better than my fix.

https://codereview.chromium.org/1734203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1734203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:704:
didMarkForLayout = true;
Should this go inside the "if (delayedLayoutScope)" conditional?  If the second
layout happens immediately, then it's not necessary to re-flex later.

I don't think that could ever happen with this version of the code (i.e., if
flexing is happening, then delayedLayoutScope will always be non-null), but it
seems like something that could get broken in the future.

[cbiesinger, 2016-02-25 21:27:59]

The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run

[cbiesinger, 2016-02-25 21:29:38]

Good point. Made that change and renamed that variable to
didMarkForDelayedLayout.

Also, this patch does fix release blocker
https://bugs.chromium.org/p/chromium/issues/detail?id=579401

[leviw_travelin_and_unemployed, 2016-02-25 21:31:29]

Nice work!

https://codereview.chromium.org/1734203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1734203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:842: bool didMarking =
false;
This could have a better name.

[cbiesinger, 2016-02-25 21:33:13]

https://codereview.chromium.org/1734203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1734203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:842: bool didMarking =
false;
On 2016/02/25 21:31:28, leviw wrote:
> This could have a better name.

Done.

[skobes, 2016-02-25 21:33:33]

Can you put some more detail in the change description?

[szager1, 2016-02-25 21:38:39]

lgtm after addressing skobes and leviw comments.

[cbiesinger, 2016-02-25 21:45:22]

Description was changed from

==========
Fully (?) fix overflow: auto with delayed scroll updates

BUG=579401,584363
==========

to

==========
Fully (?) fix overflow: auto with delayed scroll updates

The scroll delaying code, used by Flexbox, never lays out the
parent after the child relayout. This is a problem especially
because the layout can add/remove a horizontal scrollbar, 
which changes the height of the element. A vertical scrollbar
has this problem less often, though it sometimes does, especially
in case of flex items. As far as I can tell, this never worked
correctly since the scroll delaying code existed.

This patch changes it so that when we eventually do the scroll info
updates, we mark the ancestor chain for layout (up to the outer flexbox)
and then call layoutFlexItems again, to do the correct layout.

See also the discussion thread at
https://groups.google.com/a/chromium.org/d/topic/layout-dev/e7Wzvxu7qiA/discu...

BUG=579401,584363
==========

[cbiesinger, 2016-02-25 21:45:54]

Improved the description. skobes/leviw any further comments?

[cbiesinger, 2016-02-25 21:46:03]

The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run

[cbiesinger, 2016-02-25 21:46:13]

Description was changed from

==========
Fully (?) fix overflow: auto with delayed scroll updates

The scroll delaying code, used by Flexbox, never lays out the
parent after the child relayout. This is a problem especially
because the layout can add/remove a horizontal scrollbar, 
which changes the height of the element. A vertical scrollbar
has this problem less often, though it sometimes does, especially
in case of flex items. As far as I can tell, this never worked
correctly since the scroll delaying code existed.

This patch changes it so that when we eventually do the scroll info
updates, we mark the ancestor chain for layout (up to the outer flexbox)
and then call layoutFlexItems again, to do the correct layout.

See also the discussion thread at
https://groups.google.com/a/chromium.org/d/topic/layout-dev/e7Wzvxu7qiA/discu...

BUG=579401,584363
==========

to

==========
Fully (?) fix overflow: auto with delayed scroll updates

The scroll delaying code, used by Flexbox, never lays out the
parent after the child relayout. This is a problem especially
because the layout can add/remove a horizontal scrollbar, 
which changes the height of the element. A vertical scrollbar
has this problem less often, though it sometimes does, especially
in case of flex items. As far as I can tell, this never worked
correctly since the scroll delaying code existed.

This patch changes it so that when we eventually do the scroll info
updates, we mark the ancestor chain for layout (up to the outer flexbox)
and then call layoutFlexItems again, to do the correct layout.

See also the discussion thread at
https://groups.google.com/a/chromium.org/d/topic/layout-dev/e7Wzvxu7qiA/discu...

BUG=579401,584363
==========

[leviw_travelin_and_unemployed, 2016-02-25 21:49:51]

LGTM! :D

https://codereview.chromium.org/1734203002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp
(right):

https://codereview.chromium.org/1734203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp:535:
LayoutBlock::finishDelayUpdateScrollInfo(0);
nullptr

https://codereview.chromium.org/1734203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp:786:
LayoutBlock::finishDelayUpdateScrollInfo(0);
ditto

https://codereview.chromium.org/1734203002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):

https://codereview.chromium.org/1734203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:256: bool
updateAfterLayout(SubtreeLayoutScope* = 0);
nullptr

[commit-bot: I haz the power, 2016-02-25 21:51:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1734203002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1734203002/40001

[cbiesinger, 2016-02-25 21:53:45]

The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-25 22:00:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1734203002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1734203002/60001

[skobes, 2016-02-25 22:09:31]

lgtm % nit

https://codereview.chromium.org/1734203002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):

https://codereview.chromium.org/1734203002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:256: bool
updateAfterLayout(SubtreeLayoutScope* = nullptr);
Can you add a comment here explaining the return value?

[cbiesinger, 2016-02-25 22:18:32]

The CQ bit was unchecked by cbiesinger@chromium.org

[cbiesinger, 2016-02-25 22:23:29]

cbiesinger@chromium.org changed reviewers:
- szager@google.com

[cbiesinger, 2016-02-25 22:28:49]

The CQ bit was checked by cbiesinger@chromium.org

[cbiesinger, 2016-02-25 22:28:49]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org,
szager@chromium.org, skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1734203002/#ps80001
(title: "added comment")

[commit-bot: I haz the power, 2016-02-25 22:33:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1734203002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1734203002/80001

[commit-bot: I haz the power, 2016-02-26 02:42:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-26 02:42:43]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[cbiesinger1, 2016-02-26 03:28:12]

The CQ bit was checked by cbiesinger@google.com

[commit-bot: I haz the power, 2016-02-26 03:30:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1734203002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1734203002/80001

[commit-bot: I haz the power, 2016-02-26 06:06:08]

Description was changed from

==========
Fully (?) fix overflow: auto with delayed scroll updates

The scroll delaying code, used by Flexbox, never lays out the
parent after the child relayout. This is a problem especially
because the layout can add/remove a horizontal scrollbar, 
which changes the height of the element. A vertical scrollbar
has this problem less often, though it sometimes does, especially
in case of flex items. As far as I can tell, this never worked
correctly since the scroll delaying code existed.

This patch changes it so that when we eventually do the scroll info
updates, we mark the ancestor chain for layout (up to the outer flexbox)
and then call layoutFlexItems again, to do the correct layout.

See also the discussion thread at
https://groups.google.com/a/chromium.org/d/topic/layout-dev/e7Wzvxu7qiA/discu...

BUG=579401,584363
==========

to

==========
Fully (?) fix overflow: auto with delayed scroll updates

The scroll delaying code, used by Flexbox, never lays out the
parent after the child relayout. This is a problem especially
because the layout can add/remove a horizontal scrollbar, 
which changes the height of the element. A vertical scrollbar
has this problem less often, though it sometimes does, especially
in case of flex items. As far as I can tell, this never worked
correctly since the scroll delaying code existed.

This patch changes it so that when we eventually do the scroll info
updates, we mark the ancestor chain for layout (up to the outer flexbox)
and then call layoutFlexItems again, to do the correct layout.

See also the discussion thread at
https://groups.google.com/a/chromium.org/d/topic/layout-dev/e7Wzvxu7qiA/discu...

BUG=579401,584363
==========

[commit-bot: I haz the power, 2016-02-26 06:06:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-26 06:07:04]

Description was changed from

==========
Fully (?) fix overflow: auto with delayed scroll updates

The scroll delaying code, used by Flexbox, never lays out the
parent after the child relayout. This is a problem especially
because the layout can add/remove a horizontal scrollbar, 
which changes the height of the element. A vertical scrollbar
has this problem less often, though it sometimes does, especially
in case of flex items. As far as I can tell, this never worked
correctly since the scroll delaying code existed.

This patch changes it so that when we eventually do the scroll info
updates, we mark the ancestor chain for layout (up to the outer flexbox)
and then call layoutFlexItems again, to do the correct layout.

See also the discussion thread at
https://groups.google.com/a/chromium.org/d/topic/layout-dev/e7Wzvxu7qiA/discu...

BUG=579401,584363
==========

to

==========
Fully (?) fix overflow: auto with delayed scroll updates

The scroll delaying code, used by Flexbox, never lays out the
parent after the child relayout. This is a problem especially
because the layout can add/remove a horizontal scrollbar,
which changes the height of the element. A vertical scrollbar
has this problem less often, though it sometimes does, especially
in case of flex items. As far as I can tell, this never worked
correctly since the scroll delaying code existed.

This patch changes it so that when we eventually do the scroll info
updates, we mark the ancestor chain for layout (up to the outer flexbox)
and then call layoutFlexItems again, to do the correct layout.

See also the discussion thread at
https://groups.google.com/a/chromium.org/d/topic/layout-dev/e7Wzvxu7qiA/discu...

BUG=579401,584363

Committed: https://crrev.com/775378f6ac8293e20a393c5203f5431a4a679339
Cr-Commit-Position: refs/heads/master@{#377826}
==========

[commit-bot: I haz the power, 2016-02-26 06:07:06]

Patchset 5 (id:??) landed as
https://crrev.com/775378f6ac8293e20a393c5203f5431a4a679339
Cr-Commit-Position: refs/heads/master@{#377826}

[lfg, 2016-02-29 23:45:05]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1746353002/ by lfg@chromium.org.

The reason for reverting is: Crashes in blink::LayoutObject::container. See
https://crbug.com/525316..