Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we distinguishing href from src in the hydration branch but not here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot. But (tag !== 'a' || propKey !== 'href') is not very nice to read so I went with !(tag === 'a' && key === 'href') to match the comment better.

) {
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: 54 additions & 1 deletion 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,12 +3561,17 @@ export function pushStartInstance(
case 'span':
case 'svg':
case 'path':
case 'a':
case 'g':
case 'p':
case 'li':
// Fast track very common tags
break;
case 'a':
if (enableFilterEmptyStringAttributesDOM) {
return pushStartAnchor(target, props);
} else {
break;
}
Comment on lines +3569 to +3571
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure the ordering was picked with scientific rigor but since a is pretty common maybe put it back where it was and just add the extra break before it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I put it back up I have to make an additional check that we're dealing with an a tag so that e.g. div wouldn't fall into pushStartAnchor.

As far as I can tell, I can only put it before the common group or after but not in between without having an additional check. But then maybe I'm missing some neat tricks with switch statements that I haven't thought of.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just add the extra break before it?

That was the important bit I missed.

// Special tags
case 'select':
return pushStartSelect(target, props);
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