Fix unscrollable scrollers after closing a <dialog>.

Fix unscrollable scrollers after closing a <dialog>.

Compositing state was stale because removing the dialog from the top layer
triggered simplified layout, which did not invoke PLSA::updateScrollableAreaSet.

See bug for detailed analysis.

BUG=633520

[skobes, 2016-10-15 04:57:49]

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

[commit-bot: I haz the power, 2016-10-15 04:57:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2016-10-15 04:58:30]

Description was changed from

==========
Fix unscrollable scrollers after closing a <dialog>.

Compositing state was stale because removing the dialog from the top layer
triggered simplified layout, which did not invoke PLSA::updateScrollableAreaSet.

BUG=633520
==========

to

==========
Fix unscrollable scrollers after closing a <dialog>.

Compositing state was stale because removing the dialog from the top layer
triggered simplified layout, which did not invoke PLSA::updateScrollableAreaSet.

See bug for detailed analysis.

BUG=633520
==========

[skobes, 2016-10-15 05:17:06]

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

[commit-bot: I haz the power, 2016-10-15 06:36:35]

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

[commit-bot: I haz the power, 2016-10-15 06:36:36]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-10-15 08:38:45]

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

[esprehn, 2016-10-15 08:38:46]

Dialog is just removing a child and putting it as a child of the view. This
would manifest for any situation where we do that, so shouldn't LayoutView be
handling this whenever a child is removed?

https://codereview.chromium.org/2416423003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2416423003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:126:
document().layoutView()->setNeedsLayout(
This should be in the top layer management code, not inside dialog. Anything
inside too layer will manifest similar bugs since they become kids of the
LayoutView. It's also very strange for the DOM to explicitly  mark layout dirty,
that generally means something is broken at a deeper level in the system.

[skobes, 2016-10-15 18:48:26]

On 2016/10/15 08:38:46, esprehn wrote:
> Dialog is just removing a child and putting it as a child of the view. This
> would manifest for any situation where we do that, so shouldn't LayoutView be
> handling this whenever a child is removed?

It's not needed for all child removals, it's only needed when it's removed from
m_topLayerElements.  I can do it in Document::removeFromTopLayer if you think
that's better.

>
https://codereview.chromium.org/2416423003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):
> 
>
https://codereview.chromium.org/2416423003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:126:
> document().layoutView()->setNeedsLayout(
> This should be in the top layer management code, not inside dialog. Anything
> inside too layer will manifest similar bugs since they become kids of the
> LayoutView. It's also very strange for the DOM to explicitly  mark layout
dirty,
> that generally means something is broken at a deeper level in the system.

It's strange because we have this non-local dependency where the scroller's
layout checks LayoutObject::visibleToHitTesting -> Node::isInert ->
Document::activeModalDialog -> m_topLayerElements.  This means changes to
m_topLayerElements specifically require more layout invalidation than just
adding/removing children of the LayoutView.

[skobes, 2016-10-15 19:05:12]

Patchset #2 (id:20001) has been deleted

[skobes, 2016-10-15 19:05:51]

I moved the invalidation into Element::setIsInTopLayer, and gave it a new
LayoutInvalidationReason.

[szager1, 2016-10-17 19:16:30]

https://codereview.chromium.org/2416423003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2416423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3397:
document().layoutView()->setNeedsLayout(
Can we condition this on (layoutObject() && layoutObject()->hasOverflowClip())?

[esprehn, 2016-10-17 19:19:58]

https://codereview.chromium.org/2416423003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2416423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3397:
document().layoutView()->setNeedsLayout(
This belongs in Document::removeFromTopLayer

[skobes, 2016-10-17 19:33:23]

Patchset #3 (id:60001) has been deleted

[skobes, 2016-10-17 19:45:30]

https://codereview.chromium.org/2416423003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2416423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3397:
document().layoutView()->setNeedsLayout(
On 2016/10/17 19:19:58, esprehn wrote:
> This belongs in Document::removeFromTopLayer

Done.

https://codereview.chromium.org/2416423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3397:
document().layoutView()->setNeedsLayout(
On 2016/10/17 19:16:30, szager1 wrote:
> Can we condition this on (layoutObject() &&
layoutObject()->hasOverflowClip())?

We can't - the dialog itself is not a scroller.

[skobes, 2016-10-17 19:47:23]

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

[commit-bot: I haz the power, 2016-10-17 19:48:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2016-10-17 20:30:09]

lgtm

https://codereview.chromium.org/2416423003/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/dialog/scrollable-after-close.html (right):

https://codereview.chromium.org/2416423003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/dialog/scrollable-after-close.html:3: <head>
leave out html, head and body

[esprehn, 2016-10-17 20:31:02]

btw does this make us visit every child of the document doing a layout? Where
does this early out?

[esprehn, 2016-10-17 20:31:12]

On 2016/10/17 at 20:31:02, esprehn wrote:
> btw does this make us visit every child of the document doing a layout? Where
does this early out?

err, every *descendant*

[skobes, 2016-10-17 20:37:46]

On 2016/10/17 20:31:12, esprehn wrote:
> On 2016/10/17 at 20:31:02, esprehn wrote:
> > btw does this make us visit every child of the document doing a layout?
Where
> does this early out?
> 
> err, every *descendant*

No, in general if a child isn't marked for layout, it won't get laid out.  It
only matters for things like percentage-height descendants that get marked as
needing layout by LayoutBlockFlow::layoutBlock.

[skobes, 2016-10-17 20:43:13]

The CQ bit was checked by skobes@chromium.org

[skobes, 2016-10-17 20:43:14]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2416423003/#ps100001
(title: "address review comments")

[commit-bot: I haz the power, 2016-10-17 20:43:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2016-10-17 22:19:56]

The CQ bit was unchecked by skobes@chromium.org

[skobes, 2016-10-17 22:36:24]

The CQ bit was checked by skobes@chromium.org

[skobes, 2016-10-17 22:36:24]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2416423003/#ps120001
(title: "add null check")

[commit-bot: I haz the power, 2016-10-17 22:37:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 01:38:00]

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

[commit-bot: I haz the power, 2016-10-18 01:38:01]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[skobes, 2016-10-21 05:19:00]

Based on http://crbug.com/633520#c22, I'm abandoning this patch in favor of
http://crrev.com/2437303002.