Only reflect valid popup values

This was resolved in [1] - only valid values for the `popup` attribute should be reflected, so that feature detection can be used on the values. [1] openui/open-ui#491 (comment) Bug: 1307772 Change-Id: I932a28bd48e336cf01253a971804dc00dcd099fa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3639122 Auto-Submit: Mason Freed <[email protected]> Reviewed-by: Joey Arhar <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1001641} NOKEYCHECK=True GitOrigin-RevId: 21e6efd79ab1fecf1b95c3fb35a8133d801c1396
mjfroman · May 10, 2022 · a420056 · a420056
1 parent d724a13
commit a420056
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 6 deletions.
diff --git a/blink/renderer/core/dom/element.idl b/blink/renderer/core/dom/element.idl
@@ -149,7 +149,7 @@ dictionary IsVisibleOptions {
     // The Popup API
     [MeasureAs=ElementShowPopup,RuntimeEnabled=HTMLPopupAttribute,RaisesException] void showPopup();
     [MeasureAs=ElementHidePopup,RuntimeEnabled=HTMLPopupAttribute,RaisesException] void hidePopup();
-    [Unscopable,CEReactions,RuntimeEnabled=HTMLPopupAttribute,Reflect] attribute DOMString popup;
+    [Unscopable,CEReactions,RuntimeEnabled=HTMLPopupAttribute,Reflect,ReflectOnly=("popup","hint","async"),ReflectMissing="",ReflectInvalid=""] attribute DOMString popup;
     [CEReactions,RuntimeEnabled=HTMLPopupAttribute,Reflect] attribute boolean defaultOpen;
 
     // Experimental accessibility API

diff --git a/blink/renderer/core/html/keywords.json5 b/blink/renderer/core/html/keywords.json5
@@ -73,6 +73,12 @@
     "eager",
     "auto",
 
+    // popup attribute (experimental)
+    // https://github.com/openui/open-ui/blob/main/research/src/pages/popup/popup.research.explainer.mdx
+    "popup",
+    "hint",
+    "async",
+
     // referrerpolicy attribute
     // https://w3c.github.io/webappsec-referrer-policy/#referrer-policies
     "no-referrer",

diff --git a/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html b/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html
@@ -80,19 +80,27 @@
     popup.setAttribute('popup','hint');
     assert_equals(popup.popup,'hint');
     popup.setAttribute('popup','invalid');
-    assert_equals(popup.popup,'invalid');
+    assert_equals(popup.popup,'','Invalid values should reflect as empty string');
     popup.setAttribute('popup','HiNt');
-    assert_equals(popup.popup,'HiNt');
+    assert_equals(popup.popup,'hint','Case is normalized');
     popup.removeAttribute('popup');
-    assert_equals(popup.popup,'');
+    assert_equals(popup.popup,'','No value should reflect as empty string');
     popup.popup='hint';
     assert_equals(popup.getAttribute('popup'),'hint');
     popup.popup='popup';
     assert_equals(popup.getAttribute('popup'),'popup');
     popup.popup='invalid';
-    assert_equals(popup.getAttribute('popup'),'invalid');
+    assert_equals(popup.getAttribute('popup'),'invalid','IDL setter allows any value');
+    assert_equals(popup.popup,'','but IDL getter does not re-reflect invalid values');
     popup.popup='';
-    assert_equals(popup.getAttribute('popup'),'');
+    assert_equals(popup.getAttribute('popup'),'','Empty should map to empty');
+    assert_equals(popup.popup,'','Empty should map to empty in IDL');
+    popup.popup=null;
+    assert_equals(popup.getAttribute('popup'),'null','Attribute for "null" should still get set');
+    assert_equals(popup.popup,'','Null should map to empty in IDL');
+    popup.popup=undefined;
+    assert_equals(popup.getAttribute('popup'),'undefined','Attribute for "undefined" should still get set');
+    assert_equals(popup.popup,'','undefined should map to empty in IDL');
   },'IDL attribute reflection');
 
   test(() => {
@@ -110,6 +118,8 @@
     assertIsFunctionalPopup(popup);
     popup.popup = 'PoPuP';
     assertIsFunctionalPopup(popup);
+    popup.popup = 'invalid';
+    assertNotAPopup(popup);
   },'Popup attribute value should be case insensitive');
 
   test(() => {