Skip to content

Commit 0850281

Browse files
committed
Add warning when using popoverTarget={Element}
1 parent f74fb57 commit 0850281

File tree

5 files changed

+59
-6
lines changed

5 files changed

+59
-6
lines changed

fixtures/attribute-behavior/AttributeTableSnapshot.md

+4-4
Original file line numberDiff line numberDiff line change
@@ -8476,11 +8476,11 @@
84768476
## `popoverTarget` (on `<button>` inside `<div>`)
84778477
| Test Case | Flags | Result |
84788478
| --- | --- | --- |
8479-
| `popoverTarget=(string)`| (initial)| `<null>` |
8479+
| `popoverTarget=(string)`| (changed)| `<HTMLDivElement>` |
84808480
| `popoverTarget=(empty string)`| (initial)| `<null>` |
8481-
| `popoverTarget=(array with string)`| (initial)| `<null>` |
8482-
| `popoverTarget=(empty array)`| (initial)| `<null>` |
8483-
| `popoverTarget=(object)`| (initial)| `<null>` |
8481+
| `popoverTarget=(array with string)`| (changed, warning, ssr warning)| `<HTMLDivElement>` |
8482+
| `popoverTarget=(empty array)`| (initial, warning, ssr warning)| `<null>` |
8483+
| `popoverTarget=(object)`| (initial, warning, ssr warning)| `<null>` |
84848484
| `popoverTarget=(numeric string)`| (initial)| `<null>` |
84858485
| `popoverTarget=(-1)`| (initial)| `<null>` |
84868486
| `popoverTarget=(0)`| (initial)| `<null>` |

fixtures/attribute-behavior/public/index.html

+1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
You need to enable JavaScript to run this app.
2727
</noscript>
2828
<div id="root"></div>
29+
<div id="popover-target" popover="auto"></div>
2930
<!--
3031
This HTML file is a template.
3132
If you open it directly in the browser, you will see an empty page.

fixtures/attribute-behavior/src/attributes.js

+10-1
Original file line numberDiff line numberDiff line change
@@ -1450,7 +1450,16 @@ const attributes = [
14501450
{name: 'popover', overrideStringValue: 'manual'},
14511451
{
14521452
name: 'popoverTarget',
1453-
read: element => element.popoverTargetElement,
1453+
read: element => {
1454+
document.body.appendChild(element);
1455+
try {
1456+
// trigger and target need to be connected for `popoverTargetElement` to read the actual value.
1457+
return element.popoverTargetElement;
1458+
} finally {
1459+
document.body.removeChild(element);
1460+
}
1461+
},
1462+
overrideStringValue: 'popover-target',
14541463
tagName: 'button',
14551464
},
14561465
{name: 'popoverTargetAction', overrideStringValue: 'show', tagName: 'button'},

packages/react-dom-bindings/src/client/ReactDOMComponent.js

+15
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ let didWarnFormActionName = false;
8282
let didWarnFormActionTarget = false;
8383
let didWarnFormActionMethod = false;
8484
let didWarnForNewBooleanPropsWithEmptyValue: {[string]: boolean};
85+
let didWarnPopoverTargetObject = false;
8586
let canDiffStyleForHydrationWarning;
8687
if (__DEV__) {
8788
didWarnForNewBooleanPropsWithEmptyValue = {};
@@ -866,6 +867,20 @@ function setProp(
866867
case 'innerText':
867868
case 'textContent':
868869
break;
870+
case 'popoverTarget':
871+
if (__DEV__) {
872+
if (
873+
!didWarnPopoverTargetObject &&
874+
value != null &&
875+
typeof value === 'object'
876+
) {
877+
didWarnPopoverTargetObject = true;
878+
console.error(
879+
'The `popoverTarget` prop expects the ID of an Element as a string. Received %s instead.',
880+
value,
881+
);
882+
}
883+
}
869884
// Fall through
870885
default: {
871886
if (

packages/react-dom/src/__tests__/DOMPropertyOperations-test.js

+29-1
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,13 @@ describe('DOMPropertyOperations', () => {
1616
let React;
1717
let ReactDOMClient;
1818
let act;
19+
let assertConsoleErrorDev;
1920

2021
beforeEach(() => {
2122
jest.resetModules();
2223
React = require('react');
2324
ReactDOMClient = require('react-dom/client');
24-
({act} = require('internal-test-utils'));
25+
({act, assertConsoleErrorDev} = require('internal-test-utils'));
2526
});
2627

2728
// Sets a value in a way that React doesn't see,
@@ -1317,6 +1318,33 @@ describe('DOMPropertyOperations', () => {
13171318
});
13181319
expect(customElement.foo).toBe(undefined);
13191320
});
1321+
1322+
it('warns when using popoverTarget={HTMLElement}', async () => {
1323+
const popoverTarget = document.createElement('div');
1324+
const container = document.createElement('div');
1325+
const root = ReactDOMClient.createRoot(container);
1326+
1327+
await act(() => {
1328+
root.render(
1329+
<button key="one" popoverTarget={popoverTarget}>
1330+
Toggle popover
1331+
</button>,
1332+
);
1333+
});
1334+
1335+
assertConsoleErrorDev([
1336+
'The `popoverTarget` prop expects the ID of an Element as a string. Received [object HTMLDivElement] instead.',
1337+
]);
1338+
1339+
// Dedupe warning
1340+
await act(() => {
1341+
root.render(
1342+
<button key="two" popoverTarget={popoverTarget}>
1343+
Toggle popover
1344+
</button>,
1345+
);
1346+
});
1347+
});
13201348
});
13211349

13221350
describe('deleteValueForProperty', () => {

0 commit comments

Comments
 (0)