Fix logic around attribute seralization (#26526)

There was a bug in the attribute seralization for stylesheet resources injected by the Fizz runtime. For boolean properties the attribute value was set to an empty string but later immediately set to a string coerced value. This PR fixes that bug and refactors the code paths to be clearer DiffTrain build for [4a1cc2d](4a1cc2d)
facebook · Apr 3, 2023 · 8fb86b0 · 8fb86b0
1 parent 54201b8
commit 8fb86b0
Show file tree

Hide file tree

Showing 7 changed files with 275 additions and 185 deletions.
diff --git a/compiled/facebook-www/REVISION b/compiled/facebook-www/REVISION
@@ -1 +1 @@
-7329ea81c154d40800e30104be40f050e8c2af3e
+4a1cc2ddd035f5c269e82ab6f7686e2e60d3b3ea
diff --git a/compiled/facebook-www/ReactDOMServer-dev.classic.js b/compiled/facebook-www/ReactDOMServer-dev.classic.js
@@ -19,7 +19,7 @@ if (__DEV__) {
 var React = require("react");
 var ReactDOM = require("react-dom");
 
-var ReactVersion = "18.3.0-www-classic-81d2f28c";
+var ReactVersion = "18.3.0-www-classic-83a1ba88";
 
 // This refers to a WWW module.
 var warningWWW = require("warning");
@@ -6311,52 +6311,63 @@ function writeStyleResourceAttributeInJS(destination, name, value) {
       return;
     // Attribute renames
 
-    case "className":
+    case "className": {
       attributeName = "class";
+
+      {
+        checkAttributeStringCoercion(value, attributeName);
+      }
+
+      attributeValue = "" + value;
       break;
+    }
     // Booleans
 
-    case "hidden":
+    case "hidden": {
       if (value === false) {
         return;
       }
 
       attributeValue = "";
       break;
+    }
     // Santized URLs
 
     case "src":
     case "href": {
+      value = sanitizeURL(value);
+
       {
         checkAttributeStringCoercion(value, attributeName);
       }
 
-      value = sanitizeURL(value);
+      attributeValue = "" + value;
       break;
     }
 
     default: {
+      if (
+        // unrecognized event handlers are not SSR'd and we (apparently)
+        // use on* as hueristic for these handler props
+        name.length > 2 &&
+        (name[0] === "o" || name[0] === "O") &&
+        (name[1] === "n" || name[1] === "N")
+      ) {
+        return;
+      }
+
       if (!isAttributeNameSafe(name)) {
         return;
       }
-    }
-  }
 
-  if (
-    // shouldIgnoreAttribute
-    // We have already filtered out null/undefined and reserved words.
-    name.length > 2 &&
-    (name[0] === "o" || name[0] === "O") &&
-    (name[1] === "n" || name[1] === "N")
-  ) {
-    return;
-  }
+      {
+        checkAttributeStringCoercion(value, attributeName);
+      }
 
-  {
-    checkAttributeStringCoercion(value, attributeName);
+      attributeValue = "" + value;
+    }
   }
 
-  attributeValue = "" + value;
   writeChunk(destination, arrayInterstitial);
   writeChunk(
     destination,
@@ -6502,52 +6513,63 @@ function writeStyleResourceAttributeInAttr(destination, name, value) {
       return;
     // Attribute renames
 
-    case "className":
+    case "className": {
       attributeName = "class";
+
+      {
+        checkAttributeStringCoercion(value, attributeName);
+      }
+
+      attributeValue = "" + value;
       break;
+    }
     // Booleans
 
-    case "hidden":
+    case "hidden": {
       if (value === false) {
         return;
       }
 
       attributeValue = "";
       break;
+    }
     // Santized URLs
 
     case "src":
     case "href": {
+      value = sanitizeURL(value);
+
       {
         checkAttributeStringCoercion(value, attributeName);
       }
 
-      value = sanitizeURL(value);
+      attributeValue = "" + value;
       break;
     }
 
     default: {
+      if (
+        // unrecognized event handlers are not SSR'd and we (apparently)
+        // use on* as hueristic for these handler props
+        name.length > 2 &&
+        (name[0] === "o" || name[0] === "O") &&
+        (name[1] === "n" || name[1] === "N")
+      ) {
+        return;
+      }
+
       if (!isAttributeNameSafe(name)) {
         return;
       }
-    }
-  }
 
-  if (
-    // shouldIgnoreAttribute
-    // We have already filtered out null/undefined and reserved words.
-    name.length > 2 &&
-    (name[0] === "o" || name[0] === "O") &&
-    (name[1] === "n" || name[1] === "N")
-  ) {
-    return;
-  }
+      {
+        checkAttributeStringCoercion(value, attributeName);
+      }
 
-  {
-    checkAttributeStringCoercion(value, attributeName);
+      attributeValue = "" + value;
+    }
   }
 
-  attributeValue = "" + value;
   writeChunk(destination, arrayInterstitial);
   writeChunk(
     destination,

diff --git a/compiled/facebook-www/ReactDOMServer-dev.modern.js b/compiled/facebook-www/ReactDOMServer-dev.modern.js
@@ -19,7 +19,7 @@ if (__DEV__) {
 var React = require("react");
 var ReactDOM = require("react-dom");
 
-var ReactVersion = "18.3.0-www-modern-4e6bdd84";
+var ReactVersion = "18.3.0-www-modern-06decfab";
 
 // This refers to a WWW module.
 var warningWWW = require("warning");
@@ -6311,52 +6311,63 @@ function writeStyleResourceAttributeInJS(destination, name, value) {
       return;
     // Attribute renames
 
-    case "className":
+    case "className": {
       attributeName = "class";
+
+      {
+        checkAttributeStringCoercion(value, attributeName);
+      }
+
+      attributeValue = "" + value;
       break;
+    }
     // Booleans
 
-    case "hidden":
+    case "hidden": {
       if (value === false) {
         return;
       }
 
       attributeValue = "";
       break;
+    }
     // Santized URLs
 
     case "src":
     case "href": {
+      value = sanitizeURL(value);
+
       {
         checkAttributeStringCoercion(value, attributeName);
       }
 
-      value = sanitizeURL(value);
+      attributeValue = "" + value;
       break;
     }
 
     default: {
+      if (
+        // unrecognized event handlers are not SSR'd and we (apparently)
+        // use on* as hueristic for these handler props
+        name.length > 2 &&
+        (name[0] === "o" || name[0] === "O") &&
+        (name[1] === "n" || name[1] === "N")
+      ) {
+        return;
+      }
+
       if (!isAttributeNameSafe(name)) {
         return;
       }
-    }
-  }
 
-  if (
-    // shouldIgnoreAttribute
-    // We have already filtered out null/undefined and reserved words.
-    name.length > 2 &&
-    (name[0] === "o" || name[0] === "O") &&
-    (name[1] === "n" || name[1] === "N")
-  ) {
-    return;
-  }
+      {
+        checkAttributeStringCoercion(value, attributeName);
+      }
 
-  {
-    checkAttributeStringCoercion(value, attributeName);
+      attributeValue = "" + value;
+    }
   }
 
-  attributeValue = "" + value;
   writeChunk(destination, arrayInterstitial);
   writeChunk(
     destination,
@@ -6502,52 +6513,63 @@ function writeStyleResourceAttributeInAttr(destination, name, value) {
       return;
     // Attribute renames
 
-    case "className":
+    case "className": {
       attributeName = "class";
+
+      {
+        checkAttributeStringCoercion(value, attributeName);
+      }
+
+      attributeValue = "" + value;
       break;
+    }
     // Booleans
 
-    case "hidden":
+    case "hidden": {
       if (value === false) {
         return;
       }
 
       attributeValue = "";
       break;
+    }
     // Santized URLs
 
     case "src":
     case "href": {
+      value = sanitizeURL(value);
+
       {
         checkAttributeStringCoercion(value, attributeName);
       }
 
-      value = sanitizeURL(value);
+      attributeValue = "" + value;
       break;
     }
 
     default: {
+      if (
+        // unrecognized event handlers are not SSR'd and we (apparently)
+        // use on* as hueristic for these handler props
+        name.length > 2 &&
+        (name[0] === "o" || name[0] === "O") &&
+        (name[1] === "n" || name[1] === "N")
+      ) {
+        return;
+      }
+
       if (!isAttributeNameSafe(name)) {
         return;
       }
-    }
-  }
 
-  if (
-    // shouldIgnoreAttribute
-    // We have already filtered out null/undefined and reserved words.
-    name.length > 2 &&
-    (name[0] === "o" || name[0] === "O") &&
-    (name[1] === "n" || name[1] === "N")
-  ) {
-    return;
-  }
+      {
+        checkAttributeStringCoercion(value, attributeName);
+      }
 
-  {
-    checkAttributeStringCoercion(value, attributeName);
+      attributeValue = "" + value;
+    }
   }
 
-  attributeValue = "" + value;
   writeChunk(destination, arrayInterstitial);
   writeChunk(
     destination,
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		7329ea81c154d40800e30104be40f050e8c2af3e
		4a1cc2ddd035f5c269e82ab6f7686e2e60d3b3ea