Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore old behavior for empty href props on anchor tags #28124

Merged
merged 3 commits into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fixtures/attribute-behavior/AttributeTableSnapshot.md
Original file line number Diff line number Diff line change
Expand Up @@ -5077,7 +5077,7 @@
| Test Case | Flags | Result |
| --- | --- | --- |
| `href=(string)`| (changed)| `"https://reactjs.com/"` |
| `href=(empty string)`| (initial, warning)| `<empty string>` |
| `href=(empty string)`| (changed)| `"http://localhost:3000/"` |
| `href=(array with string)`| (changed)| `"https://reactjs.com/"` |
| `href=(empty array)`| (changed)| `"http://localhost:3000/"` |
| `href=(object)`| (changed)| `"http://localhost:3000/result%20of%20toString()"` |
Expand Down
12 changes: 10 additions & 2 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,11 @@ function setProp(
case 'src':
case 'href': {
if (enableFilterEmptyStringAttributesDOM) {
if (value === '') {
if (
value === '' &&
// <a href=""> is fine for "reload" links.
!(tag === 'a' && key === 'href')
) {
if (__DEV__) {
if (key === 'src') {
console.error(
Expand Down Expand Up @@ -2350,7 +2354,11 @@ function diffHydratedGenericElement(
case 'src':
case 'href':
if (enableFilterEmptyStringAttributesDOM) {
if (value === '') {
if (
value === '' &&
// <a href=""> is fine for "reload" links.
!(tag === 'a' && propKey === 'href')
) {
if (__DEV__) {
if (propKey === 'src') {
console.error(
Expand Down
55 changes: 55 additions & 0 deletions packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -1508,6 +1508,54 @@ function checkSelectProp(props: any, propName: string) {
}
}

function pushStartAnchor(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
): ReactNodeList {
target.push(startChunkForTag('a'));

let children = null;
let innerHTML = null;
for (const propKey in props) {
if (hasOwnProperty.call(props, propKey)) {
const propValue = props[propKey];
if (propValue == null) {
continue;
}
switch (propKey) {
case 'children':
children = propValue;
break;
case 'dangerouslySetInnerHTML':
innerHTML = propValue;
break;
case 'href':
if (propValue === '') {
// Empty `href` is special on anchors so we're short-circuiting here.
// On other tags it should trigger a warning
pushStringAttribute(target, 'href', '');
} else {
pushAttribute(target, propKey, propValue);
}
break;
default:
pushAttribute(target, propKey, propValue);
break;
}
}
}

target.push(endOfStartTag);
pushInnerHTML(target, innerHTML, children);
if (typeof children === 'string') {
// Special case children as a string to avoid the unnecessary comment.
// TODO: Remove this special case after the general optimization is in place.
target.push(stringToChunk(encodeHTMLTextNode(children)));
return null;
}
return children;
}

function pushStartSelect(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
Expand Down Expand Up @@ -3513,7 +3561,14 @@ export function pushStartInstance(
case 'span':
case 'svg':
case 'path':
// Fast track very common tags
break;
case 'a':
if (enableFilterEmptyStringAttributesDOM) {
return pushStartAnchor(target, props);
} else {
break;
}
case 'g':
case 'p':
case 'li':
Expand Down
10 changes: 10 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,16 @@ describe('ReactDOMComponent', () => {
expect(node.hasAttribute('href')).toBe(false);
});

it('should allow an empty href attribute on anchors', async () => {
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await act(() => {
root.render(<a href="" />);
});
const node = container.firstChild;
expect(node.getAttribute('href')).toBe('');
});

it('should allow an empty action attribute', async () => {
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,38 @@ describe('ReactDOMServerIntegration', () => {
expect(e.getAttribute('width')).toBe('30');
});

itRenders('empty src on img', async render => {
const e = await render(
<img src="" />,
ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? 1 : 0,
);
expect(e.getAttribute('src')).toBe(
ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? null : '',
);
});

itRenders('empty href on anchor', async render => {
const e = await render(<a href="" />);
expect(e.getAttribute('href')).toBe('');
});

itRenders('empty href on other tags', async render => {
const e = await render(
// <link href="" /> would be more sensible.
// However, that results in a hydration warning as well.
// Our test helpers do not support different error counts for initial
// server render and hydration.
// The number of errors on the server need to be equal to the number of
// errors during hydration.
// So we use a <div> instead.
<div href="" />,
ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? 1 : 0,
);
expect(e.getAttribute('href')).toBe(
ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? null : '',
);
});

itRenders('no string prop with true value', async render => {
const e = await render(<a href={true} />, 1);
expect(e.hasAttribute('href')).toBe(false);
Expand Down