Remove scroll-to-light-dismiss behavior from popups

Per [1], we have resolved that scrolling should *not* light dismiss popups. This CL implements that change. [1] openui/open-ui#240 (comment) Bug: 1307772 Change-Id: I8749574733892bc33c1b19af954badb367d24139 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3564406 Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#988054}
chromium · Apr 1, 2022 · 6c41e8b · 6c41e8b
1 parent 189fbf1
commit 6c41e8b
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 67 deletions.
diff --git a/third_party/blink/renderer/core/dom/element.cc b/third_party/blink/renderer/core/dom/element.cc
@@ -2517,12 +2517,9 @@ void Element::HandlePopupLightDismiss(const Event& event) {
   auto& document = target_node->GetDocument();
   DCHECK(document.PopupShowing());
   const AtomicString& event_type = event.type();
-  if (event_type == event_type_names::kMousedown ||
-      event_type == event_type_names::kScroll) {
-    // - For scroll, hide everything up to the scrolled element, to allow
-    //   scrolling within a popup.
-    // - For mousedown, hide everything up to the clicked element. We do
-    //   this on mousedown, rather than mouseup/click, for two reasons:
+  if (event_type == event_type_names::kMousedown) {
+    // - Hide everything up to the clicked element. We do this on mousedown,
+    //   rather than mouseup/click, for two reasons:
     //    1. This mirrors typical platform popups, which dismiss on mousedown.
     //    2. This allows a mouse-drag that starts on a popup and finishes off
     //       the popup, without light-dismissing the popup.

diff --git a/...ntics/interactive-elements/the-popup-element/popup-light-dismiss-on-scroll.tentative.html b/...ntics/interactive-elements/the-popup-element/popup-light-dismiss-on-scroll.tentative.html
diff --git a/...web_tests/external/wpt/html/semantics/popups/popup-light-dismiss-on-scroll.tentative.html b/...web_tests/external/wpt/html/semantics/popups/popup-light-dismiss-on-scroll.tentative.html
@@ -1,9 +1,10 @@
 <!DOCTYPE html>
 <html lang="en">
 <meta charset="utf-8" />
-<title>Popup light dismiss on scroll</title>
+<title>Popup should *not* light dismiss on scroll</title>
 <link rel="author" href="mailto:masonf@chromium.org">
 <link rel=help href="https://open-ui.org/components/popup.research.explainer">
+<link rel=help href="https://github.com/openui/open-ui/issues/240">
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 
@@ -15,8 +16,12 @@
   quis. Tortor dignissim convallis aenean et. Eu mi bibendum neque egestas congue quisque
 </div>
 
-<div popup=popup id=popup1>This is popup 1<div id=anchor></div></div>
-<div popup=popup id=popup2 anchor=anchor>This is popup 2</div>
+<div popup=popup id=popup1>
+  This is popup 1
+  <div popup=popup id=popup2 anchor=anchor>
+    This is popup 2
+  </div>
+</div>
 <button onclick='popup1.showPopup();popup2.showPopup();'>Open popups</button>
 
 <style>
@@ -31,17 +36,28 @@
 </style>
 
 <script>
+  const popups = document.querySelectorAll('[popup]');
+  function assertAll(showing) {
+    for(let popup of popups) {
+      assert_equals(popup.matches(':popup-open'),showing);
+    }
+  }
   async_test(t => {
-    popup1.addEventListener('hide',e => {
-      assert_false(popup2.matches(':popup-open'));
-      t.done();
-    })
-    assert_false(popup1.matches(':popup-open'));
-    assert_false(popup2.matches(':popup-open'));
-    popup1.showPopup();
-    popup2.showPopup();
-    assert_true(popup1.matches(':popup-open'));
-    assert_true(popup2.matches(':popup-open'));
+    for(let popup of popups) {
+      popup.addEventListener('hide',e => {
+        assert_unreached('Scrolling should not light-dismiss a popup');
+      });
+    }
+    assertAll(/*showing*/false);
+    popups[0].showPopup();
+    popups[1].showPopup();
+    assertAll(/*showing*/true);
     scroller.scrollTo(0, 100);
-  },'Scrolling light-dismisses all popups');
+    requestAnimationFrame(() => {
+      requestAnimationFrame(() => {
+        assertAll(/*showing*/true);
+        t.done();
+      });
+    });
+  },'Scrolling should not light-dismiss popups');
 </script>