Fix unscrollable scrollers after closing a <dialog>. (issue 2437303002 by skobes@chromium.org)

1 view
Skip to first unread message

sko...@chromium.org

unread,
Oct 21, 2016, 1:18:24 AM10/21/16
to sza...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, blink-rev...@chromium.org, esp...@chromium.org
Reviewers: szager1
CL: https://codereview.chromium.org/2437303002/

Description:
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

Affected files (+63, -6 lines):
A third_party/WebKit/LayoutTests/dialog/scrollable-after-close.html
A third_party/WebKit/LayoutTests/dialog/scrollable-after-close-expected.txt
M third_party/WebKit/Source/core/layout/LayoutObject.h
M third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp


Index: third_party/WebKit/LayoutTests/dialog/scrollable-after-close-expected.txt
diff --git a/third_party/WebKit/LayoutTests/dialog/scrollable-after-close-expected.txt b/third_party/WebKit/LayoutTests/dialog/scrollable-after-close-expected.txt
new file mode 100644
index 0000000000000000000000000000000000000000..be0b87d9899e96f5409efbcfbb0c28043593e8a3
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/dialog/scrollable-after-close-expected.txt
@@ -0,0 +1,10 @@
+Tests that FrameView::m_scrollableAreas is updated after closing a modal dialog when the scroller and its ancestors have percentage heights. (see crbug.com/633520)
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS internals.numberOfScrollableAreas(document) is 1
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Index: third_party/WebKit/LayoutTests/dialog/scrollable-after-close.html
diff --git a/third_party/WebKit/LayoutTests/dialog/scrollable-after-close.html b/third_party/WebKit/LayoutTests/dialog/scrollable-after-close.html
new file mode 100644
index 0000000000000000000000000000000000000000..567fba734960fbdd2527ed665e6b228441f7af0a
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/dialog/scrollable-after-close.html
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<script src="../resources/js-test.js"></script>
+<style>
+html, body { height: 100%; margin: 0; overflow: hidden; }
+#scroller {
+ overflow: scroll;
+ position: relative;
+ z-index: 0;
+ width: 300px;
+ height: 50%;
+ left: 20px;
+ top: 20px;
+ border: 1px solid black;
+}
+</style>
+<dialog></dialog>
+<div id="scroller">
+ <div style="height: 700px; width: 700px"></div>
+</div>
+<script>
+
+description("Tests that FrameView::m_scrollableAreas is updated after " +
+ "closing a modal dialog when the scroller and its ancestors " +
+ "have percentage heights. (see crbug.com/633520)");
+
+var dialog = document.querySelector("dialog");
+dialog.showModal();
+dialog.close();
+
+// Force layout.
+document.body.offsetWidth;
+
+shouldBe("internals.numberOfScrollableAreas(document)", "1");
+
+</script>
Index: third_party/WebKit/Source/core/layout/LayoutObject.h
diff --git a/third_party/WebKit/Source/core/layout/LayoutObject.h b/third_party/WebKit/Source/core/layout/LayoutObject.h
index c4cf35150e68a740cc9b2c61775d1e1e8db32475..f9f9b786a18b72edeb689dbc528b80b28c8b6be6 100644
--- a/third_party/WebKit/Source/core/layout/LayoutObject.h
+++ b/third_party/WebKit/Source/core/layout/LayoutObject.h
@@ -1505,9 +1505,17 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
!isInert();
}

- bool visibleToHitTesting() const {
+ enum VisibleToHitTestingBehavior {
+ VisibleToHitTestingDefault,
+ VisibleToHitTestingStyleRulesOnly
+ };
+ // Warning: VisibleToHitTestingDefault checks inertness, which can change
+ // without causing relayout of this LayoutObject.
+ bool visibleToHitTesting(
+ VisibleToHitTestingBehavior behavior = VisibleToHitTestingDefault) const {
return style()->visibility() == EVisibility::Visible &&
- style()->pointerEvents() != PE_NONE && !isInert();
+ style()->pointerEvents() != PE_NONE &&
+ (!isInert() || behavior == VisibleToHitTestingStyleRulesOnly);
}

// Map points and quads through elements, potentially via 3d transforms. You
Index: third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
index 57e8539ce33fbf0c3f98b9cbb7a7ff2308174a49..10f4452f94d059c8501c45d3c68cf3a7f29ddad5 100644
--- a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
@@ -1620,11 +1620,15 @@ void PaintLayerScrollableArea::updateScrollableAreaSet(bool hasOverflow) {
if (!frameView)
return;

+ LayoutObject::VisibleToHitTestingBehavior behavior =
+ LayoutObject::VisibleToHitTestingStyleRulesOnly;
+
// FIXME: Does this need to be fixed later for OOPI?
- bool isVisibleToHitTest = box().visibleToHitTesting();
- if (HTMLFrameOwnerElement* owner = frame->deprecatedLocalOwner())
- isVisibleToHitTest &=
- owner->layoutObject() && owner->layoutObject()->visibleToHitTesting();
+ bool isVisibleToHitTest = box().visibleToHitTesting(behavior);
+ if (HTMLFrameOwnerElement* owner = frame->deprecatedLocalOwner()) {
+ isVisibleToHitTest &= owner->layoutObject() &&
+ owner->layoutObject()->visibleToHitTesting(behavior);
+ }

bool didScrollOverflow = m_scrollsOverflow;



sza...@chromium.org

unread,
Oct 21, 2016, 8:14:11 PM10/21/16
to sko...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, blink-rev...@chromium.org, esp...@chromium.org
lgtm, although I think it would be cleaner to do this:

ComputedStyle.cpp:

bool visibleToHitTesting() const {
return visibility() == EVisibility::Visible && pointerEvents() != PE_NONE;
}

LayoutObject.h:

bool visibleToHitTesting const {
return style()->visibleToHitTesting && !isInert();
}

PaintLayerScrollableArea.cpp:

bool isVisibleToHitTest = box().style()->visibleToHitTesting();

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

sko...@chromium.org

unread,
Oct 21, 2016, 9:52:09 PM10/21/16
to sza...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, blink-rev...@chromium.org, esp...@chromium.org
On 2016/10/22 00:14:10, szager1 wrote:
> lgtm, although I think it would be cleaner to do this:
>
> ComputedStyle.cpp:
>
> bool visibleToHitTesting() const {
> return visibility() == EVisibility::Visible && pointerEvents() != PE_NONE;
> }
>
> LayoutObject.h:
>
> bool visibleToHitTesting const {
> return style()->visibleToHitTesting && !isInert();
> }
>
> PaintLayerScrollableArea.cpp:
>
> bool isVisibleToHitTest = box().style()->visibleToHitTesting();

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,
Oct 21, 2016, 9:52:46 PM10/21/16
to sko...@chromium.org, sza...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, blink-rev...@chromium.org, esp...@chromium.org

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,
Oct 22, 2016, 1:02:59 AM10/22/16
to sko...@chromium.org, sza...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, blink-rev...@chromium.org, esp...@chromium.org
Committed patchset #2 (id:60001)

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

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,
Oct 22, 2016, 1:04:56 AM10/22/16
to sko...@chromium.org, sza...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, blink-rev...@chromium.org, esp...@chromium.org
Patchset 2 (id:??) landed as
https://crrev.com/3f34d9380173cbb2b36d76a859a477472e80ae61
Cr-Commit-Position: refs/heads/master@{#426975}

https://codereview.chromium.org/2437303002/
Reply all
Reply to author
Forward
0 new messages