Able to reproduce the issue on Mac 10.11.6 using 54.0.2816.0.

 Issue 635459  has been merged into this issue.

Did some initial investigation. This particular dialog is one of the few cases where it is not removed from the DOM when it is closed, https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people_page/people_page.html?l=298, where no dom-if exists. It seems that native <dialog> retains focus even after it is closed via close(), which could be a bug.

Next step: Verify this suspicion by determining whether the same bug occurs in other MD Settings, dialogs that remain in the DOM after they are closed. If so, come up with a minimal repro case.

A work-around that would probably work is to remove the dialog from the DOM when it is not displayed.

 Issue 648941  has been merged into this issue.

+dbeam, esprehn

Could this be a blink bug? It happens only when the <dialog> is closed, but it is not removed from the DOM. See attached screencast on how scrolling starts to work again if the window if resized.

I have not been able to reproduce in outside of settings yet, see my attempt at https://jsfiddle.net/8z4vr6we/1/. Perhaps it only happens when Shadow DOM is involved, I will try that next.

Deleted: scrolling_locked.mp4 1.4 MB

	scrolling_locked.mp4 1.4 MB View Download

Managed to create a minimal repro at https://jsfiddle.net/Lzq825k1/.

Repro steps.
1) Make window short enough for a scrollbar to appear.
2) Scroll to the botto and open the dialog.
3) Close dialog using the 'esc' key.
4) Try to scroll, it does not work.
5) Resize the window (even a 1px resize will do).
6) Try to scroll again, it works.

pretty sure this is a blink bug

I did a little bit of sleuthing, but not much came out of it.

I assume this has to do with inert-ness.  Might be wrong, but that's where i started my search.

I sprinkled some extra insertSubtreesChanged() calls in HTMLDialogElement.cpp (or just did in more cases, i.e. commented out some ifs), didn't help.

I tracked down callers of LayoutObject::isInert() and Node::isInert() and found LayoutObject::visibleToHitTesting() which is used by PaintLayerScrollableArea::updateScrollableAreaSet() to determine whether to add a "scrollable area".

PaintLayerScrollableArea::updateScrollableAreaSet() has an early return

  if (HTMLFrameOwnerElement* owner = frame->deprecatedLocalOwner())
      isVisibleToHitTest &= owner->layoutObject() && owner->layoutObject()->visibleToHitTesting();

  bool didScrollOverflow = m_scrollsOverflow;
  m_scrollsOverflow = hasOverflow && isVisibleToHitTest;
  if (didScrollOverflow == scrollsOverflow())
      return;

It seems like this is likely to correctly change when inertness changes, but maybe it doesn't?

The implementation of HTMLDialogElement::closeDialog(), the activeModalDialog-related code, and most "top layer" code all looked pretty air tight...

Just posting my findings, eventually got stumped and didn't want to keep looking; I have no idea whether my looking into PaintLayerScrollableArea is the correct path or not.

skobes@ Got ideas? :)

Works with --disable-threaded-scrolling so definitely cc layer related.

@skobes: Any updates here? Is there an ETA for a fix?

I'll try to fix this next week.

Thanks. One more thing to note. I am planning to use <dialog> for implementing action menus for Chrome's MD settings page (like a menu that comes up when you click on a "..." icon, see attachment), so this fix will be of great help.

Deleted: dialog_for_popup.png 42.0 KB

	dialog_for_popup.png 42.0 KB View Download

Comment #12 was pretty close.

First note that the use of html { overflow: hidden; } body { overflow: auto; } makes the body a separate scroller from the document.  The bug does not repro for document scrolling.

PaintLayerScrollableArea::updateScrollableAreaSet updates the set of active scrollable areas in FrameView::m_scrollableAreas.  If the scroller is not visible to hit testing (for example, due to a modal dialog) it is removed from the set.

So updateScrollableAreaSet depends on the dialog state, but it only runs when the scroller is laid out.  That's brittle.

When the dialog is shown, it runs a normal layout on the document.  If the scroller and all its ancestors have percentage heights, they're marked for relayout during a normal layout, even if their height hasn't actually changed (see LayoutBlock::dirtyForLayoutFromPercentageHeightDescendants).

When the dialog is dismissed, it runs a simplified layout (LayoutBlock::simplifiedLayout) on the document.  This doesn't descend to the scroller, so we never call updateScrollableAreaSet.

FrameView::m_scrollableAreas is used to set up compositing state - the scroller's layers on high DPI, or the non_fast_scrollable_region on low DPI.  Either way, if it's missing our scroller, the compositor thread won't know about it.  So, the compositor thread tries to scroll the (un-scrollable) document.

The Blink codepaths for handling scroll input don't rely on FrameView::m_scrollableAreas, which is why it works with threaded scrolling disabled.

My solution in https://codereview.chromium.org/2416423003/ is for HTMLDialogElement::closeDialog to mark the document as needing a normal layout and not a simplified layout.  This matches what happens during showModal.

 Issue 655017  has been merged into this issue.

hey skobes@: I'm trying your patch (https://codereview.chromium.org/2416423003/) now and it fixes dpapad@'s minimal repro case (https://jsfiddle.net/Lzq825k1/) but not the actual Material Design settings page.

the repro steps there are:
0. go to chrome://md-settings/searchEngines
1. click the ... menu
2. close the <dialog> shown to implement this action menu (i.e. Esc, click on scrim, press tab)

what do I expect?
all ways that close the <dialog> result in scrolling being restored

what happens instead?
many ways (i.e. Esc, clicking on scrim) don't seem to re-enable scrolling :(

Hmm, you're right.  There is another failure mode, where neither showModal nor closeDialog makes the scroller go through layout, but something else does while the dialog is open.  So it's not sufficient for closeDialog to imitate the layout invalidations of showModal.

We could walk the DOM and mark every scroller for layout, but that's rather extreme / inefficient.

We could have a mechanism where PaintLayerScrollableArea subscribes itself to changes in Document::m_topLayerElements, and reacts by running updateScrollableAreaSet.

Or perhaps we can rethink the dependency of FrameView::m_scrollableAreas on LayoutObject::visibleToHitTesting.  If a scrollable area is covered by a dialog, scroll events won't even reach it.  So why do we need to explicitly mark it "unscrollable"?

I'll poke at it some more this week.

 Issue 656893  has been merged into this issue.

New patch: https://codereview.chromium.org/2437303002/

This fixes the md-settings page.

Verified that this fixes the issues affecting MD settings. Thanks!

Just one question, by reading the CL description, it sounds that an assumption is made that ::backdrop is always there, but I don't think it is if show() is called instead of showModal(). Perhaps worth verifying that it does not break the show() case.

I think show() is fine since it doesn't touch Document::m_topLayerElements.  If you show() a dialog, you can still scroll things on the page while it is shown.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3f34d9380173cbb2b36d76a859a477472e80ae61

commit 3f34d9380173cbb2b36d76a859a477472e80ae61
Author: skobes <skobes@chromium.org>
Date: Sat Oct 22 05:02:33 2016

Fix unscrollable scrollers after closing a <dialog>.

Inertness of a scroller can change without causing it to be laid out, if a
dialog is added or removed from Document::m_topLayerElements.  Checking
inertness in PLSA::updateScrollableAreaSet led to stale compositing state.

This patch updates PLSA to ignore inertness.  This means scrollers will remain
in FrameView::m_scrollableAreas even when covered by a dialog.  That should be
ok, since the ::backdrop element prevents them from actually receiving input.

BUG= 633520 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://chromiumcodereview.appspot.com/2437303002
Cr-Commit-Position: refs/heads/master@{#426975}

[add] https://crrev.com/3f34d9380173cbb2b36d76a859a477472e80ae61/third_party/WebKit/LayoutTests/dialog/scrollable-after-close-expected.txt
[add] https://crrev.com/3f34d9380173cbb2b36d76a859a477472e80ae61/third_party/WebKit/LayoutTests/dialog/scrollable-after-close.html
[modify] https://crrev.com/3f34d9380173cbb2b36d76a859a477472e80ae61/third_party/WebKit/Source/core/layout/LayoutObject.h
[modify] https://crrev.com/3f34d9380173cbb2b36d76a859a477472e80ae61/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
[modify] https://crrev.com/3f34d9380173cbb2b36d76a859a477472e80ae61/third_party/WebKit/Source/core/style/ComputedStyle.h

This should now be fixed in canary (56.0.2899.0).