Move the pop-up "show" event earlier and make it cancelable

Per [1], there's a desire from Mozilla to move the show event
earlier in the process, and make it cancelable. I see no issues
with doing that, so pending a discussion (at [1]), I'm going to
make this change.

[1] openui/open-ui#579

Bug: 1307772
Change-Id: I87fcec84146f99434bb88d2af12941a2c9156586
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3826329
Reviewed-by: Joey Arhar <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1035213}
NOKEYCHECK=True
GitOrigin-RevId: 2343d80475c01f868b592640435e1c8dfd03498d

blink/renderer/core/dom/element.cc

@@ -86,6 +86,7 @@
 #include "third_party/blink/renderer/core/dom/element_rare_data.h"
 #include "third_party/blink/renderer/core/dom/element_traversal.h"
 #include "third_party/blink/renderer/core/dom/events/event_dispatch_forbidden_scope.h"
+#include "third_party/blink/renderer/core/dom/events/event_dispatch_result.h"
 #include "third_party/blink/renderer/core/dom/events/event_dispatcher.h"
 #include "third_party/blink/renderer/core/dom/events/event_path.h"
 #include "third_party/blink/renderer/core/dom/first_letter_pseudo_element.h"
@@ -2512,6 +2513,17 @@ void Element::showPopUp(ExceptionState& exception_state) {
         "Invalid on already-showing or disconnected popup elements");
   }
 
+  // Fire the show event (bubbles, cancelable).
+  Event* event = Event::CreateCancelableBubble(event_type_names::kShow);
+  event->SetTarget(this);
+  if (DispatchEvent(*event) != DispatchEventResult::kNotCanceled)
+    return;
+
+  // The 'show' event handler could have changed this pop-up, e.g. by changing
+  // its type, removing it from the document, or calling showPopUp().
+  if (!HasValidPopupAttribute() || !isConnected() || popupOpen())
+    return;
+
   bool should_restore_focus = false;
   auto& document = GetDocument();
   if (PopupType() == PopupValueType::kAuto ||
@@ -2561,17 +2573,6 @@ void Element::showPopUp(ExceptionState& exception_state) {
   DCHECK(!document.AllOpenPopUps().Contains(this));
   document.AllOpenPopUps().insert(this);
 
-  // Fire the show event (bubbles, not cancelable).
-  Event* event = Event::CreateBubble(event_type_names::kShow);
-  event->SetTarget(this);
-  auto result = DispatchEvent(*event);
-  DCHECK_EQ(result, DispatchEventResult::kNotCanceled);
-
-  // The 'show' event handler could have changed this pop-up, e.g. by changing
-  // its type, removing it from the document, or calling showPopUp().
-  if (!HasValidPopupAttribute() || !isConnected() || popupOpen())
-    return;
-
   GetPopupData()->setAnimationFinishedListener(nullptr);
   GetPopupData()->setPreviouslyFocusedElement(
       should_restore_focus ? document.FocusedElement() : nullptr);

blink/web_tests/external/wpt/html/semantics/popups/popup-events.tentative.html

@@ -10,31 +10,54 @@
 <div popup>Popup</div>
 
 <script>
-promise_test(async t => {
-  const popup = document.querySelector('[popup]');
-  let showCount = 0;
-  let hideCount = 0;
-  await new Promise(resolve => window.addEventListener('load',() => resolve()));
-  assert_false(popup.matches(':top-layer'));
-  document.addEventListener('show',() => ++showCount);
-  document.addEventListener('hide',() => ++hideCount);
-  assert_equals(0,showCount);
-  assert_equals(0,hideCount);
-  popup.showPopUp();
-  assert_true(popup.matches(':top-layer'));
-  assert_equals(1,showCount);
-  assert_equals(0,hideCount);
-  await waitForRender();
-  assert_true(popup.matches(':top-layer'));
-  popup.hidePopUp();
-  assert_false(popup.matches(':top-layer'));
-  assert_equals(1,showCount);
-  assert_equals(1,hideCount);
-  await waitForRender();
-  // No additional events after animation frame
-  assert_false(popup.matches(':top-layer'));
-  assert_equals(1,showCount);
-  assert_equals(1,hideCount);
-}, 'Show and hide events get properly dispatched for popups');
+window.onload = () => {
+  promise_test(async t => {
+    const popup = document.querySelector('[popup]');
+    let showCount = 0;
+    let hideCount = 0;
+    assert_false(popup.matches(':top-layer'));
+    const controller = new AbortController();
+    const signal = controller.signal;
+    t.add_cleanup(() => controller.abort());
+    document.addEventListener('show',() => ++showCount, {signal});
+    document.addEventListener('hide',() => ++hideCount, {signal});
+    assert_equals(0,showCount);
+    assert_equals(0,hideCount);
+    popup.showPopUp();
+    assert_true(popup.matches(':top-layer'));
+    assert_equals(1,showCount);
+    assert_equals(0,hideCount);
+    await waitForRender();
+    assert_true(popup.matches(':top-layer'));
+    popup.hidePopUp();
+    assert_false(popup.matches(':top-layer'));
+    assert_equals(1,showCount);
+    assert_equals(1,hideCount);
+    await waitForRender();
+    // No additional events after animation frame
+    assert_false(popup.matches(':top-layer'));
+    assert_equals(1,showCount);
+    assert_equals(1,hideCount);
+  }, 'Show and hide events get properly dispatched for popups');
 
+  promise_test(async t => {
+    const popUp = document.querySelector('[popup]');
+    const controller = new AbortController();
+    const signal = controller.signal;
+    t.add_cleanup(() => controller.abort());
+    let cancel = true;
+    popUp.addEventListener('show',(e) => {
+      if (cancel)
+        e.preventDefault();
+    }, {signal});
+    assert_false(popUp.matches(':top-layer'));
+    popUp.showPopUp();
+    assert_false(popUp.matches(':top-layer'),'The "show" event should be cancelable');
+    cancel = false;
+    popUp.showPopUp();
+    assert_true(popUp.matches(':top-layer'));
+    popUp.hidePopUp();
+    assert_false(popUp.matches(':top-layer'));
+  }, 'Show event is cancelable');
+};
 </script>