Change the popup invalid value default to "manual"

Per the resolution [1], invalid values should now be treated as if they were `popup=manual`. This lines up with the related change [2] to the UA stylesheet for pop-up. I also took the opportunity to expand the "basic" test to include adding the `popup` attribute on all (visible) element types. [1] openui/open-ui#533 (comment) [2] https://chromium-review.googlesource.com/c/chromium/src/+/3840811 Bug: 1307772 Change-Id: I6d5bf956e344f9256ab9cc1bf3d30655c8dee5bf
web-platform-tests · Aug 25, 2022 · cd8dfbd · cd8dfbd · community-tc-integration · Aug 25, 2022
1 parent 36029f9
commit cd8dfbd
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 46 deletions.
diff --git a/html/semantics/popups/popup-appearance-ref.tentative.html b/html/semantics/popups/popup-appearance-ref.tentative.html
@@ -5,15 +5,16 @@
 
 <style>
 .fake-pop-up {top: 100px; bottom: auto;}
-#blank {left: -300px;}
-#auto {left: -100px;}
-#hint {left: 100px;}
-#manual {left: 300px;}
+#blank {left: -400px;}
+#auto {left: -200px;}
+#hint {left: 0;}
+#manual {left: 200px;}
+#invalid {left: 400px;}
 </style>
 
-<p>There should be four pop-ups with similar appearance, and
-   the word "Unknown" should not be visible on the page.</p>
+<p>There should be five pop-ups with similar appearance.</p>
 <div class="fake-pop-up" id=blank>Blank</div>
 <div class="fake-pop-up" id=auto>Auto</div>
-<div class="fake-pop-up" id=hint><span>Hint</span></div>
+<div class="fake-pop-up" id=hint>Hint</div>
 <div class="fake-pop-up" id=manual>Manual</div>
+<div class="fake-pop-up" id=invalid>Invalid</div>
diff --git a/html/semantics/popups/popup-appearance.tentative.html b/html/semantics/popups/popup-appearance.tentative.html
@@ -7,27 +7,21 @@
 
 <style>
 [popup] {top: 100px; bottom: auto;}
-[popup=""] {left: -300px}
-[popup=auto] {left: -100px; }
-[popup=hint] {left: 100px; }
-[popup=manual] {left: 300px; }
+[popup=""] {left: -400px}
+[popup=auto] {left: -200px; }
+[popup=hint] {left: 0; }
+[popup=manual] {left: 200px; }
+[popup=invalid] {left: 400px; }
 </style>
 
-<p>There should be four pop-ups with similar appearance, and
-   the word "Unknown" should not be visible on the page.</p>
+<p>There should be five pop-ups with similar appearance.</p>
 <div popup>Blank
   <div popup=auto>Auto</div>
 </div>
 <div popup=hint>Hint</div>
 <div popup=manual>Manual</div>
-<!-- This ensures unsupported popup values are hidden -->
-<div popup=unknown>Unknown</div>
+<!-- This ensures unsupported popup values are treated as popup=manual -->
+<div popup=invalid>Invalid</div>
 <script>
-  document.querySelectorAll('[popup]').forEach(p => {
-    try {
-      p.showPopUp();
-    } catch {
-      // The unknown popup should throw an error on .showPopUp().
-    }
-  });
+  document.querySelectorAll('[popup]').forEach(p => p.showPopUp());
 </script>
diff --git a/html/semantics/popups/popup-attribute-basic.tentative.html b/html/semantics/popups/popup-attribute-basic.tentative.html
@@ -5,19 +5,25 @@
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="resources/popup-utils.js"></script>
+<script src="../../resources/common.js"></script>
 
 <div id=popups>
   <div popup id=boolean>Pop up</div>
   <div popup="">Pop up</div>
   <div popup=auto>Pop up</div>
   <div popup=hint>Pop up</div>
   <div popup=manual>Pop up</div>
+  <article popup>Different element type</article>
+  <header popup>Different element type</header>
+  <nav popup>Different element type</nav>
+  <input type=text popup value="Different element type">
+  <div popup=true>Invalid popup value - defaults to popup=manual</div>
+  <div popup=popup>Invalid popup value - defaults to popup=manual</div>
+  <div popup=invalid>Invalid popup value - defaults to popup=manual</div>
 </div>
 
 <div id=nonpopups>
   <div>Not a pop-up</div>
-  <div popup=popup>Not a pop-up</div>
-  <div popup=invalid>Not a pop-up</div>
 </div>
 
 <div popup class=animated>Animated pop-up</div>
@@ -32,24 +38,25 @@
 </style>
 
 <script>
-  function popUpVisible(popUp, isPopUp) {
+  function assertPopUpVisibility(popUp, isPopUp, expectedVisibility, message) {
     const isVisible = isElementVisible(popUp);
+    assert_equals(isVisible, expectedVisibility,`${message}: Expected this element to be ${expectedVisibility ? "visible" : "not visible"}`);
+    // Check other things related to being visible or not:
     if (isVisible) {
       assert_not_equals(window.getComputedStyle(popUp).display,'none');
-      assert_equals(popUp.matches(':open'),isPopUp);
+      assert_equals(popUp.matches(':open'),isPopUp,`${message}: Visible pop-ups should match :open`);
     } else {
-      assert_equals(window.getComputedStyle(popUp).display,'none');
-      assert_false(popUp.matches(':open'));
+      assert_equals(window.getComputedStyle(popUp).display,'none',`${message}: Non-showing pop-ups should have display:none`);
+      assert_false(popUp.matches(':open'),`${message}: Non-showing pop-ups should *not* match :open`);
     }
-    return isVisible;
   }
   function assertIsFunctionalPopUp(popUp) {
-    assert_false(popUpVisible(popUp, /*isPopUp*/true));
+    assertPopUpVisibility(popUp, /*isPopUp*/true, /*expectedVisibility*/false, 'A pop-up should start out hidden');
     popUp.showPopUp();
-    assert_true(popUpVisible(popUp, /*isPopUp*/true));
+    assertPopUpVisibility(popUp, /*isPopUp*/true, /*expectedVisibility*/true, 'After showPopUp(), a pop-up should be visible');
     assert_throws_dom("InvalidStateError",() => popUp.showPopUp(),'Calling showPopUp on a showing pop-up should throw InvalidStateError');
     popUp.hidePopUp();
-    assert_false(popUpVisible(popUp, /*isPopUp*/true));
+    assertPopUpVisibility(popUp, /*isPopUp*/true, /*expectedVisibility*/false, 'After hidePopUp(), a pop-up should be hidden');
     assert_throws_dom("InvalidStateError",() => popUp.hidePopUp(),'Calling hidePopUp on a hidden pop-up should throw InvalidStateError');
     const parent = popUp.parentElement;
     popUp.remove();
@@ -60,23 +67,36 @@
     // If the non-pop-up element nonetheless has a 'popup' attribute, it should
     // be invisible. Otherwise, it should be visible.
     const expectVisible = !nonPopUp.hasAttribute('popup');
-    assert_equals(popUpVisible(nonPopUp, /*isPopUp*/false),expectVisible);
+    assertPopUpVisibility(nonPopUp, /*isPopUp*/false, expectVisible, 'A non-pop-up should start out visible');
     assert_throws_dom("NotSupportedError",() => nonPopUp.showPopUp(),'Calling showPopUp on a non-pop-up should throw NotSupportedError');
-    assert_equals(popUpVisible(nonPopUp, /*isPopUp*/false),expectVisible);
+    assertPopUpVisibility(nonPopUp, /*isPopUp*/false, expectVisible, 'Calling showPopUp on a non-pop-up should leave it visible');
     assert_throws_dom("NotSupportedError",() => nonPopUp.hidePopUp(),'Calling hidePopUp on a non-pop-up should throw NotSupportedError');
-    assert_equals(popUpVisible(nonPopUp, /*isPopUp*/false),expectVisible);
+    assertPopUpVisibility(nonPopUp, /*isPopUp*/false, expectVisible, 'Calling hidePopUp on a non-pop-up should leave it visible');
   }
 
+  // Start with the provided examples:
   Array.from(document.getElementById('popups').children).forEach(popUp => {
     test((t) => {
       assertIsFunctionalPopUp(popUp);
-    }, `The .showPopUp() and .hidePopUp() work on a pop-up, for ${popUp.outerHTML}.`);
+    }, `The element ${popUp.outerHTML} should behave as a pop-up.`);
   });
-
   Array.from(document.getElementById('nonpopups').children).forEach(nonPopUp => {
     test((t) => {
       assertNotAPopUp(nonPopUp);
-    }, `The .showPopUp() and .hidePopUp() do NOT work on elements without a 'popup' attribute, ${nonPopUp.outerHTML}.`);
+    }, `The element ${nonPopUp.outerHTML} should *not* behave as a pop-up.`);
+  });
+
+  // Then loop through all HTML5 elements:
+  let invisibleElements = ['audio','base','br','datalist','dialog','embed','head','link','meta','noscript','param','rp','script','style','template','title','wbr'];
+  const elements = HTML5_ELEMENTS.filter(el => !invisibleElements.includes(el));
+  elements.forEach(tag => {
+    test((t) => {
+      const element = document.createElement(tag);
+      element.setAttribute('popup','auto');
+      document.body.appendChild(element);
+      t.add_cleanup(() => element.remove());
+      assertIsFunctionalPopUp(element);
+    }, `A <${tag}> element should behave as a pop-up.`);
   });
 
   function createPopUp(t) {
@@ -102,7 +122,7 @@
     assert_equals(popUp.popUp,'hint','Case is normalized in IDL');
     assert_equals(popUp.getAttribute('popup'),'hInT','Value set from IDL is propagated exactly to the content attribute');
     popUp.setAttribute('popup','invalid');
-    assert_equals(popUp.popUp,null,'Invalid values should reflect as null');
+    assert_equals(popUp.popUp,'manual','Invalid values should reflect as "manual"');
     popUp.removeAttribute('popup');
     assert_equals(popUp.popUp,null,'No value should reflect as null');
     popUp.popUp='hint';
@@ -117,7 +137,7 @@
     assert_equals(popUp.popUp,'auto');
     popUp.popUp='invalid';
     assert_equals(popUp.getAttribute('popup'),'invalid','IDL setter allows any value');
-    assert_equals(popUp.popUp,null,'but IDL getter does not re-reflect invalid values');
+    assert_equals(popUp.popUp,'manual','but IDL getter reflects "manual"');
     popUp.popUp='';
     assert_equals(popUp.getAttribute('popup'),'','IDL setter propagates exactly');
     assert_equals(popUp.popUp,'auto','Empty should map to auto in IDL');
@@ -146,8 +166,8 @@
     assertIsFunctionalPopUp(popUp);
     popUp.popUp = 'aUtO';
     assertIsFunctionalPopUp(popUp);
-    popUp.popUp = 'invalid';
-    assertNotAPopUp(popUp);
+    popUp.popUp = 'invalid'; // treated as "manual"
+    assertIsFunctionalPopUp(popUp);
   },'Popup attribute value should be case insensitive');
 
   test((t) => {
@@ -156,11 +176,11 @@
     popUp.setAttribute('popup','hint'); // Change pop-up type
     assertIsFunctionalPopUp(popUp);
     popUp.setAttribute('popup','invalid'); // Change pop-up type to something invalid
-    assertNotAPopUp(popUp);
+    assertIsFunctionalPopUp(popUp);
     popUp.popUp = 'hint'; // Change pop-up type via IDL
     assertIsFunctionalPopUp(popUp);
-    popUp.popUp = 'invalid'; // Make invalid via IDL
-    assertNotAPopUp(popUp);
+    popUp.popUp = 'invalid'; // Make invalid via IDL (treated as "manual")
+    assertIsFunctionalPopUp(popUp);
   },'Changing attribute values for pop-up should work');
 
   test((t) => {
@@ -176,7 +196,13 @@
     popUp.showPopUp();
     assert_true(popUp.matches(':open'));
     popUp.setAttribute('popup','invalid');
-    assert_false(popUp.matches(':open'));
+    assert_true(popUp.matches(':open'),'From "manual" to "invalid" (which is interpreted as "manual") should not close the pop-up');
+    popUp.setAttribute('popup','auto');
+    assert_false(popUp.matches(':open'),'From "invalid" ("manual") to "auto" should hide the pop-up');
+    popUp.showPopUp();
+    assert_true(popUp.matches(':open'));
+    popUp.setAttribute('popup','invalid');
+    assert_false(popUp.matches(':open'),'From "auto" to "invalid" (which is interpreted as "manual") should close the pop-up');
   },'Changing attribute values should close open pop-ups');
 
   function modalPseudoSupported() {