Add warning when using popoverTarget={Element}

facebook · May 20, 2024 · 8724b59 · 8724b59
1 parent f74fb57
commit 8724b59
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 11 deletions.
diff --git a/fixtures/attribute-behavior/AttributeTableSnapshot.md b/fixtures/attribute-behavior/AttributeTableSnapshot.md
@@ -8462,25 +8462,25 @@
 | `popover=(integer)`| (changed)| `"manual"` |
 | `popover=(NaN)`| (changed, warning)| `"manual"` |
 | `popover=(float)`| (changed)| `"manual"` |
-| `popover=(true)`| (initial, warning)| `<null>` |
-| `popover=(false)`| (initial, warning)| `<null>` |
+| `popover=(true)`| (initial)| `<null>` |
+| `popover=(false)`| (initial)| `<null>` |
 | `popover=(string 'true')`| (changed)| `"manual"` |
 | `popover=(string 'false')`| (changed)| `"manual"` |
 | `popover=(string 'on')`| (changed)| `"manual"` |
 | `popover=(string 'off')`| (changed)| `"manual"` |
-| `popover=(symbol)`| (initial, warning)| `<null>` |
-| `popover=(function)`| (initial, warning)| `<null>` |
+| `popover=(symbol)`| (initial)| `<null>` |
+| `popover=(function)`| (initial)| `<null>` |
 | `popover=(null)`| (initial)| `<null>` |
 | `popover=(undefined)`| (initial)| `<null>` |
 
 ## `popoverTarget` (on `<button>` inside `<div>`)
 | Test Case | Flags | Result |
 | --- | --- | --- |
-| `popoverTarget=(string)`| (initial)| `<null>` |
+| `popoverTarget=(string)`| (changed)| `<HTMLDivElement>` |
 | `popoverTarget=(empty string)`| (initial)| `<null>` |
-| `popoverTarget=(array with string)`| (initial)| `<null>` |
-| `popoverTarget=(empty array)`| (initial)| `<null>` |
-| `popoverTarget=(object)`| (initial)| `<null>` |
+| `popoverTarget=(array with string)`| (changed, warning, ssr warning)| `<HTMLDivElement>` |
+| `popoverTarget=(empty array)`| (initial, warning, ssr warning)| `<null>` |
+| `popoverTarget=(object)`| (initial, warning, ssr warning)| `<null>` |
 | `popoverTarget=(numeric string)`| (initial)| `<null>` |
 | `popoverTarget=(-1)`| (initial)| `<null>` |
 | `popoverTarget=(0)`| (initial)| `<null>` |
@@ -8493,8 +8493,8 @@
 | `popoverTarget=(string 'false')`| (initial)| `<null>` |
 | `popoverTarget=(string 'on')`| (initial)| `<null>` |
 | `popoverTarget=(string 'off')`| (initial)| `<null>` |
-| `popoverTarget=(symbol)`| (initial, warning)| `<null>` |
-| `popoverTarget=(function)`| (initial, warning)| `<null>` |
+| `popoverTarget=(symbol)`| (initial)| `<null>` |
+| `popoverTarget=(function)`| (initial)| `<null>` |
 | `popoverTarget=(null)`| (initial)| `<null>` |
 | `popoverTarget=(undefined)`| (initial)| `<null>` |
 

diff --git a/fixtures/attribute-behavior/public/index.html b/fixtures/attribute-behavior/public/index.html
@@ -26,6 +26,7 @@
       You need to enable JavaScript to run this app.
     </noscript>
     <div id="root"></div>
+    <div id="popover-target" popover="auto"></div>
     <!--
       This HTML file is a template.
       If you open it directly in the browser, you will see an empty page.

diff --git a/fixtures/attribute-behavior/src/attributes.js b/fixtures/attribute-behavior/src/attributes.js
@@ -1450,7 +1450,16 @@ const attributes = [
   {name: 'popover', overrideStringValue: 'manual'},
   {
     name: 'popoverTarget',
-    read: element => element.popoverTargetElement,
+    read: element => {
+      document.body.appendChild(element);
+      try {
+        // trigger and target need to be connected for `popoverTargetElement` to read the actual value.
+        return element.popoverTargetElement;
+      } finally {
+        document.body.removeChild(element);
+      }
+    },
+    overrideStringValue: 'popover-target',
     tagName: 'button',
   },
   {name: 'popoverTargetAction', overrideStringValue: 'show', tagName: 'button'},

diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js
@@ -82,6 +82,7 @@ let didWarnFormActionName = false;
 let didWarnFormActionTarget = false;
 let didWarnFormActionMethod = false;
 let didWarnForNewBooleanPropsWithEmptyValue: {[string]: boolean};
+let didWarnPopoverTargetObject = false;
 let canDiffStyleForHydrationWarning;
 if (__DEV__) {
   didWarnForNewBooleanPropsWithEmptyValue = {};
@@ -866,6 +867,20 @@ function setProp(
     case 'innerText':
     case 'textContent':
       break;
+    case 'popoverTarget':
+      if (__DEV__) {
+        if (
+          !didWarnPopoverTargetObject &&
+          value != null &&
+          typeof value === 'object'
+        ) {
+          didWarnPopoverTargetObject = true;
+          console.error(
+            'The `popoverTarget` prop expects the ID of an Element as a string. Received %s instead.',
+            value,
+          );
+        }
+      }
     // Fall through
     default: {
       if (

diff --git a/packages/react-dom/src/__tests__/DOMPropertyOperations-test.js b/packages/react-dom/src/__tests__/DOMPropertyOperations-test.js
@@ -9,6 +9,7 @@
 
 'use strict';
 
+const {assertConsoleErrorDev} = require('internal-test-utils');
 // Set by `yarn test-fire`.
 const {disableInputAttributeSyncing} = require('shared/ReactFeatureFlags');
 
@@ -1317,6 +1318,33 @@ describe('DOMPropertyOperations', () => {
       });
       expect(customElement.foo).toBe(undefined);
     });
+
+    it('warns when using popoverTarget={HTMLElement}', async () => {
+      const popoverTarget = document.createElement('div');
+      const container = document.createElement('div');
+      const root = ReactDOMClient.createRoot(container);
+
+      await act(() => {
+        root.render(
+          <button key="one" popoverTarget={popoverTarget}>
+            Toggle popover
+          </button>,
+        );
+      });
+
+      assertConsoleErrorDev([
+        'The `popoverTarget` prop expects the ID of an Element as a string. Received [object HTMLDivElement] instead.',
+      ]);
+
+      // Dedupe warning
+      await act(() => {
+        root.render(
+          <button key="two" popoverTarget={popoverTarget}>
+            Toggle popover
+          </button>,
+        );
+      });
+    });
   });
 
   describe('deleteValueForProperty', () => {