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] whatwg/html#9776 (comment) Bug: 1416284,1513282 Change-Id: Ic064ecf1377bb8abfc812654c85016e6d1cbbdaf
web-platform-tests · Jan 12, 2024 · 125bcf3 · 125bcf3
1 parent 9344126
commit 125bcf3
Show file tree

Hide file tree

Showing 4 changed files with 195 additions and 119 deletions.
diff --git a/html/semantics/popovers/popover-light-dismiss-hint.tentative.html 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
@@ -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
@@ -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>