From 125bcf3398213ed8cd7134e70b5238d45bc69422 Mon Sep 17 00:00:00 2001
From: Mason Freed <masonf@chromium.org>
Date: Thu, 11 Jan 2024 18:21:14 -0800
Subject: [PATCH] Update popover=hint behavior to allow a stack of hints

The previous implementation only allowed one popover=hint to be open
at a time. Per conversation at [1], developers feel that it should be
possible to nest popover=hint popovers. This CL implements that
capability.

There are now two popover stacks in Document: PopoverAutoStack and
PopoverHintStack. Since it is possible to nest hints within autos,
the PopoverAutoStack can contain hints. However, there
are a few constraints:
 - The PopoverHintStack only ever contains hints.
 - Once the PopoverAutoStack contains a hint, all subsequent popovers
   in the stack must also be hints.
 - A popover=hint can never be the ancestor of a popover=auto.

The light dismiss behavior is roughly the same as before, with a
slight tweak that simplifies behavior: closing anything in the
PopoverAutoStack will always close everything in the PopoverHintStack.
That was not the case before, but it was a bit of a weird corner case.

Note that I found a crasher (happens in stable, with just auto
popovers) that I added a test for here. The bug for that is
crbug.com/1513282. I'll fix that in a followup.

[1] https://github.com/whatwg/html/issues/9776#issuecomment-1785814179

Bug: 1416284,1513282
Change-Id: Ic064ecf1377bb8abfc812654c85016e6d1cbbdaf
---
 .../popover-light-dismiss-hint.tentative.html | 107 +++++++++++++
 .../popovers/popover-light-dismiss.html       |  46 ------
 .../popovers/popover-open-in-event-crash.html |  20 +++
 .../popover-types-with-hints.tentative.html   | 141 +++++++++---------
 4 files changed, 195 insertions(+), 119 deletions(-)
 create mode 100644 html/semantics/popovers/popover-light-dismiss-hint.tentative.html
 create mode 100644 html/semantics/popovers/popover-open-in-event-crash.html
diff --git a/html/semantics/popovers/popover-light-dismiss-hint.tentative.html b/html/semantics/popovers/popover-light-dismiss-hint.tentative.html
new file mode 100644
index 00000000000000..f07363115fba68
--- /dev/null
+++ b/html/semantics/popovers/popover-light-dismiss-hint.tentative.html
@@ -0,0 +1,107 @@
+<!DOCTYPE html>
+<meta charset="utf-8" />
+<title>Popover light dismiss behavior for hints</title>
+<meta name="timeout" content="long">
+<link rel="author" href="mailto:masonf@chromium.org">
+<link rel=help href="https://open-ui.org/components/popover-hint.research.explainer">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/testdriver.js"></script>
+<script src="/resources/testdriver-actions.js"></script>
+<script src="/resources/testdriver-vendor.js"></script>
+<script src="resources/popover-utils.js"></script>
+
+<div id=outside></div>
+<div popover id=auto1>auto 1
+  <div popover id=auto2>auto 2
+    <div popover=hint id=innerhint1>inner hint 1
+      <div popover=hint id=innerhint2>inner hint 2
+        <div popover id=invalidauto1>Improperly nested auto 1</div>
+      </div>
+    </div>
+  </div>
+</div>
+<div popover=hint id=hint1>hint 1
+  <div popover=hint id=hint2>hint 2
+    <div popover id=invalidauto2>Improperly nested auto 2</div>
+  </div>
+</div>
+<div popover=manual id=manual1>Manual</div>
+
+<style>
+  [popover] {right:auto;bottom:auto;}
+  #auto1        {left:100px; top:100px;}
+  #auto2        {left:100px; top:200px;}
+  #innerhint1   {left:100px; top:300px;}
+  #innerhint2   {left:100px; top:400px;}
+  #invalidauto1 {left:100px; top:500px;}
+  #hint1        {left:200px; top:100px;}
+  #hint2        {left:200px; top:200px;}
+  #invalidauto1 {left:200px; top:400px;}
+  #manual1      {left:300px; top:100px;}
+  #outside {width:25px;height:25px}
+</style>
+
+<script>
+  const popovers = [
+    document.querySelector('#auto1'),
+    document.querySelector('#auto2'),
+    document.querySelector('#innerhint1'),
+    document.querySelector('#innerhint2'),
+    document.querySelector('#hint1'),
+    document.querySelector('#hint2'),
+    document.querySelector('#manual1'),
+  ];
+  function assertState(expectedState,description) {
+    description = description || 'Error';
+    const n = popovers.length;
+    assert_equals(expectedState.length,n,'Invalid');
+    for(let i=0;i<n;++i) {
+      assert_equals(popovers[i].matches(':popover-open'),expectedState[i],`${description}, index ${i} (${popovers[i].id})`);
+    }
+  }
+  function openall(t) {
+    // All popovers can be open at once, if shown in order:
+    popovers.forEach((p) => p.hidePopover());
+    popovers.forEach((p) => p.showPopover());
+    assertState(Array(popovers.length).fill(true),'All popovers should be able to be open at once');
+    t.add_cleanup(() => popovers.forEach((p) => p.hidePopover()));
+  }
+  function nvals(n,val) {
+    return new Array(n).fill(val);
+  }
+  for(let i=0;i<(popovers.length-1);++i) {
+    promise_test(async (t) => {
+      openall(t);
+      await clickOn(popovers[i]);
+      let expectedState = [...nvals(i+1,true),...nvals(popovers.length-i-2,false),true];
+      assertState(expectedState);
+    },`Mixed auto/hint light dismiss behavior, click on ${popovers[i].id}`);
+  }
+
+  promise_test(async (t) => {
+    openall(t);
+    await clickOn(outside);
+    assertState([false,false,false,false,false,false,true]);
+  },'Clicking outside closes all');
+
+  promise_test(async (t) => {
+    openall(t);
+    invalidauto1.showPopover();
+    assertState([true,true,false,false,false,false,true],'auto inside hint ignores the hints and gets nested within auto2');
+    assert_true(invalidauto1.matches(':popover-open'),'the inner nested auto should be open');
+    invalidauto1.hidePopover();
+    assertState([true,true,false,false,false,false,true]);
+    assert_false(invalidauto1.matches(':popover-open'));
+  },'Auto cannot be nested inside hint (invalidauto1)');
+
+  promise_test(async (t) => {
+    openall(t);
+    invalidauto2.showPopover();
+    assertState([false,false,false,false,false,false,true],'auto inside hint works as an independent (non-nested) auto');
+    assert_true(invalidauto2.matches(':popover-open'),'the inner nested auto should be open');
+    invalidauto2.hidePopover();
+    assertState([false,false,false,false,false,false,true]);
+    assert_false(invalidauto2.matches(':popover-open'));
+  },'Auto cannot be nested inside hint (invalidauto2)');
+</script>
diff --git a/html/semantics/popovers/popover-light-dismiss.html b/html/semantics/popovers/popover-light-dismiss.html
index 4a57276bce3cbb..0c51693aa120a7 100644
--- a/html/semantics/popovers/popover-light-dismiss.html
+++ b/html/semantics/popovers/popover-light-dismiss.html
@@ -449,52 +449,6 @@
 },'Ensure circular/convoluted ancestral relationships are functional, with a direct showPopover()');
 </script>
 
-<div popover id=p10>Popover</div>
-<div popover=hint id=p11>Hint</div>
-<div popover=manual id=p12>Manual</div>
-<style>
-  #p10 {top:100px;}
-  #p11 {top:200px;}
-  #p12 {top:300px;}
-</style>
-<script>
-if (popoverHintSupported()) {
-  promise_test(async () => {
-    const auto = document.querySelector('#p10');
-    const hint = document.querySelector('#p11');
-    const manual = document.querySelector('#p12');
-    // All three can be open at once, if shown in this order:
-    auto.showPopover();
-    hint.showPopover();
-    manual.showPopover();
-    assert_true(auto.matches(':popover-open'));
-    assert_true(hint.matches(':popover-open'));
-    assert_true(manual.matches(':popover-open'));
-    // Clicking the hint will close the auto, but not the manual.
-    await clickOn(hint);
-    assert_false(auto.matches(':popover-open'),'auto should be hidden');
-    assert_true(hint.matches(':popover-open'),'hint should stay open');
-    assert_true(manual.matches(':popover-open'),'manual does not light dismiss');
-    // Clicking outside should close the hint, but not the manual:
-    await clickOn(outside);
-    assert_false(auto.matches(':popover-open'));
-    assert_false(hint.matches(':popover-open'),'hint should close');
-    assert_true(manual.matches(':popover-open'),'manual does not light dismiss');
-    manual.hidePopover();
-    assert_false(manual.matches(':popover-open'));
-    auto.showPopover();
-    hint.showPopover();
-    assert_true(auto.matches(':popover-open'));
-    assert_true(hint.matches(':popover-open'));
-    // Clicking on the auto should close the hint:
-    await clickOn(auto);
-    assert_true(auto.matches(':popover-open'),'auto should stay open');
-    assert_false(hint.matches(':popover-open'),'hint should light dismiss');
-    auto.hidePopover();
-    assert_false(auto.matches(':popover-open'));
-  },'Light dismiss of mixed popover types including hints');
-}
-</script>
 <div popover id=p13>Popover 1
   <div popover id=p14>Popover 2
     <div popover id=p15>Popover 3</div>
diff --git a/html/semantics/popovers/popover-open-in-event-crash.html b/html/semantics/popovers/popover-open-in-event-crash.html
new file mode 100644
index 00000000000000..0a862388d516a3
--- /dev/null
+++ b/html/semantics/popovers/popover-open-in-event-crash.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<meta charset="utf-8" />
+<link rel="author" href="mailto:masonf@chromium.org">
+<link rel=help href="https://html.spec.whatwg.org/multipage/popover.html#attr-popover">
+
+<div popover id=p1>Popover 1
+  <div popover id=p2>Popover 2</div>
+</div>
+<script>
+  const p1 = document.querySelector('#p1');
+  const p2 = document.querySelector('#p2');
+  p1.addEventListener('beforetoggle',e => {
+    if (e.newState === "closed") {
+      p2.showPopover();
+    }
+  })
+  p1.showPopover();
+  p1.hidePopover();
+  // This test passes if it does not crash
+</script>
diff --git a/html/semantics/popovers/popover-types-with-hints.tentative.html b/html/semantics/popovers/popover-types-with-hints.tentative.html
index 7a73efb50fc8e5..07f0e26fce71b9 100644
--- a/html/semantics/popovers/popover-types-with-hints.tentative.html
+++ b/html/semantics/popovers/popover-types-with-hints.tentative.html
@@ -5,6 +5,22 @@
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 
+<script>
+  function getPopovers() {
+    return Array.from(document.currentScript.parentElement.querySelectorAll('[popover]'));
+  }
+  function assertState(expectedState,description) {
+    description = description || 'Error';
+    const popovers = getPopovers();
+    const n = popovers.length;
+    assert_equals(expectedState.length,n,'Invalid expected state length');
+    for(let i=0;i<n;++i) {
+      const html = '<' + popovers[i].outerHTML.split('<')[1] + '</div>';
+      assert_equals(popovers[i].matches(':popover-open'),expectedState[i],`${description}, index ${i} (${html})`);
+    }
+  }
+</script>
+
 <div>
   <div popover>Popover</div>
   <div popover=hint>Hint</div>
@@ -12,55 +28,49 @@
   <div popover=manual>Async</div>
   <script>
   {
-    const auto = document.currentScript.parentElement.querySelector('[popover=""]');
-    const hint = document.currentScript.parentElement.querySelector('[popover=hint]');
-    const manual = document.currentScript.parentElement.querySelectorAll('[popover=manual]')[0];
-    const manual2 = document.currentScript.parentElement.querySelectorAll('[popover=manual]')[1];
-    function assert_state_1(autoOpen,hintOpen,manualOpen,manual2Open) {
-      assert_equals(auto.matches(':popover-open'),autoOpen,'auto open state is incorrect');
-      assert_equals(hint.matches(':popover-open'),hintOpen,'hint open state is incorrect');
-      assert_equals(manual.matches(':popover-open'),manualOpen,'manual open state is incorrect');
-      assert_equals(manual2.matches(':popover-open'),manual2Open,'manual2 open state is incorrect');
-    }
+    const auto = getPopovers()[0];
+    const hint = getPopovers()[1];
+    const manual = getPopovers()[2];
+    const manual2 = getPopovers()[3];
     test(() => {
-      assert_state_1(false,false,false,false);
+      assertState([false,false,false,false]);
       auto.showPopover();
-      assert_state_1(true,false,false,false);
+      assertState([true,false,false,false]);
       hint.showPopover();
-      assert_state_1(true,true,false,false);
+      assertState([true,true,false,false]);
       manual.showPopover();
-      assert_state_1(true,true,true,false);
+      assertState([true,true,true,false]);
       manual2.showPopover();
-      assert_state_1(true,true,true,true);
+      assertState([true,true,true,true]);
       hint.hidePopover();
-      assert_state_1(true,false,true,true);
+      assertState([true,false,true,true]);
       auto.hidePopover();
-      assert_state_1(false,false,true,true);
+      assertState([false,false,true,true]);
       auto.showPopover();
       hint.showPopover();
-      assert_state_1(true,true,true,true);
-      auto.hidePopover(); // Non-nested tooltips can stay open when unrelated popovers are hidden.
-      assert_state_1(false,true,true,true);
+      assertState([true,true,true,true]);
+      auto.hidePopover();
+      assertState([false,false,true,true]);
       hint.hidePopover();
       manual.hidePopover();
-      assert_state_1(false,false,false,true);
+      assertState([false,false,false,true]);
       manual2.hidePopover();
-      assert_state_1(false,false,false,false);
+      assertState([false,false,false,false]);
     },'manuals do not close popovers');
 
     test(() => {
-      assert_state_1(false,false,false,false);
+      assertState([false,false,false,false]);
       hint.showPopover();
       manual.showPopover();
       manual2.showPopover();
-      assert_state_1(false,true,true,true);
+      assertState([false,true,true,true]);
       auto.showPopover();
-      assert_state_1(true,false,true,true);
+      assertState([true,false,true,true]);
       auto.hidePopover();
-      assert_state_1(false,false,true,true);
+      assertState([false,false,true,true]);
       manual.hidePopover();
       manual2.hidePopover();
-      assert_state_1(false,false,false,false);
+      assertState([false,false,false,false]);
     },'autos close hints but not manuals');
   }
   </script>
@@ -76,26 +86,20 @@
   <div popover=hint anchor=anchorid>Hint anchored to popover</div>
   <script>
   {
-    const popover1 = document.currentScript.parentElement.querySelectorAll('[popover=""]')[0];
-    const popover2 = document.currentScript.parentElement.querySelectorAll('[popover=""]')[1];
-    const popover3 = document.currentScript.parentElement.querySelectorAll('[popover=""]')[2];
-    const hint = document.currentScript.parentElement.querySelector('[popover=hint]');
-    function assert_state_2(popover1Open,popover2Open,popover3Open,hintOpen) {
-      assert_equals(popover1.matches(':popover-open'),popover1Open,'popover1 open state is incorrect');
-      assert_equals(popover2.matches(':popover-open'),popover2Open,'popover2 open state is incorrect');
-      assert_equals(popover3.matches(':popover-open'),popover3Open,'popover3 open state is incorrect');
-      assert_equals(hint.matches(':popover-open'),hintOpen,'hint open state is incorrect');
-    }
+    const popover1 = getPopovers()[0];
+    const popover2 = getPopovers()[1];
+    const popover3 = getPopovers()[2];
+    const hint = getPopovers()[3];
     test(() => {
-      assert_state_2(false,false,false,false);
+      assertState([false,false,false,false]);
       popover1.showPopover();
       popover2.showPopover();
       popover3.showPopover();
-      assert_state_2(true,true,true,false);
+      assertState([true,true,true,false]);
       hint.showPopover(); // Because hint is nested in popover2, popover3 should be hidden
-      assert_state_2(true,true,false,true);
+      assertState([true,true,false,true]);
       popover1.hidePopover(); // Should close the hint, which is anchored to popover2
-      assert_state_2(false,false,false,false);
+      assertState([false,false,false,false]);
     },'hint is not closed by pre-existing auto');
   }
   </script>
@@ -107,16 +111,15 @@
   </div>
   <script>
   test(() => {
-    const hint1 = document.currentScript.parentElement.querySelectorAll('[popover=hint]')[0];
-    const hint2 = document.currentScript.parentElement.querySelectorAll('[popover=hint]')[1];
+    const hint1 = getPopovers()[0];
+    const hint2 = getPopovers()[1];
     hint1.showPopover();
-    assert_true(hint1.matches(':popover-open'));
-    assert_false(hint2.matches(':popover-open'));
+    assertState([true,false]);
     hint2.showPopover();
-    assert_false(hint1.matches(':popover-open'));
-    assert_true(hint2.matches(':popover-open'));
-    hint2.hidePopover();
-  },'If a popover=hint is shown, it should hide any other open popover=hint popovers, including ancestral popovers. (You can\'t nest popover=hint)');
+    assertState([true,true]);
+    hint1.hidePopover();
+    assertState([false,false]);
+  },'You can nest hint popovers');
   </script>
 </div>
 
@@ -126,15 +129,14 @@
   </div>
   <script>
   test(() => {
-    const hint = document.currentScript.parentElement.querySelector('[popover=hint]');
-    const auto = document.currentScript.parentElement.querySelector('[popover=""]');
+    const hint = getPopovers()[0];
+    const auto = getPopovers()[1];
     hint.showPopover();
-    assert_true(hint.matches(':popover-open'));
-    assert_false(auto.matches(':popover-open'));
+    assertState([true,false]);
     auto.showPopover();
-    assert_false(hint.matches(':popover-open'));
-    assert_true(auto.matches(':popover-open'));
+    assertState([false,true]);
     auto.hidePopover();
+    assertState([false,false]);
   },'If a popover=auto is shown, it should hide any open popover=hint, including if the popover=hint is an ancestral popover of the popover=auto. (You can\'t nest a popover=auto inside a popover=hint)');
   </script>
 </div>
@@ -146,20 +148,16 @@
   </div>
   <script>
   test(() => {
-    const auto = document.currentScript.parentElement.querySelectorAll('[popover=""]')[0];
-    const auto2 = document.currentScript.parentElement.querySelectorAll('[popover=""]')[1];
-    const hint = document.currentScript.parentElement.querySelector('[popover=hint]');
+    const auto = getPopovers()[0];
+    const auto2 = getPopovers()[1];
+    const hint = getPopovers()[2];
     auto.showPopover();
     auto2.showPopover();
-    assert_true(auto.matches(':popover-open'));
-    assert_true(auto2.matches(':popover-open'));
+    assertState([true,true,false]);
     hint.showPopover(); // This should hide auto2, since it is nested in auto1.
-    assert_true(auto.matches(':popover-open'));
-    assert_false(auto2.matches(':popover-open'));
-    assert_true(hint.matches(':popover-open'));
+    assertState([true,false,true]);
     auto.hidePopover(); // Should hide both auto and hint.
-    assert_false(auto.matches(':popover-open'));
-    assert_false(hint.matches(':popover-open'));
+    assertState([false,false,false]);
   },'If you: a) show a popover=auto (call it D), then b) show a descendent popover=hint of D (call it T), then c) hide D, then T should be hidden. (A popover=hint can be nested inside a popover=auto)');
   </script>
 </div>
@@ -169,16 +167,13 @@
   <div popover=hint>Non-Nested hint</div>
   <script>
   test(() => {
-    const auto = document.currentScript.parentElement.querySelector('[popover=""]');
-    const hint = document.currentScript.parentElement.querySelector('[popover=hint]');
+    const auto = getPopovers()[0]
+    const hint = getPopovers()[1];
     auto.showPopover();
     hint.showPopover();
-    assert_true(auto.matches(':popover-open'));
-    assert_true(hint.matches(':popover-open'));
+    assertState([true,true]);
     auto.hidePopover();
-    assert_false(auto.matches(':popover-open'));
-    assert_true(hint.matches(':popover-open'));
-    hint.hidePopover();
-  },'If you: a) show a popover=auto (call it D), then b) show a non-descendent popover=hint of D (call it T), then c) hide D, then T should be left showing. (Non-nested popover=hint can stay open when unrelated popover=autos are hidden)');
+    assertState([false,false]);
+  },'If you: a) show a popover=auto (call it D), then b) show a non-descendent popover=hint of D (call it T), then c) hide D, then T should be hidden. (Non-nested popover=hint gets hidden when unrelated popover=autos are hidden)');
   </script>
 </div>
