Filter certain DOM attributes (e.g. src) if value is empty string #18513
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2399b82:
|
Details of bundled changes.Comparing: 3278d24...2399b82 react-dom
ReactDOM: size: 🔺+0.3%, gzip: 🔺+0.3% Size changes (experimental) |
Details of bundled changes.Comparing: 3278d24...2399b82 react-dom
Size changes (stable) |
|
Is |
|
SSR should probably just work, in the sense that it just removes it from the HTML, since it calls this. You could add some tests to the server integration tests. Note sure if that causes problems in practice but I can't think of one other than maybe |
| ) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This should move below line 178 here into the propertyInfo !== null check so we only do that once.
We also probably should not filter this for isCustomComponentTag since custom elements could have all kinds of semantics where this matters. We only try to fix the built-ins.
There was a problem hiding this comment.
This should move below line 178 here into the propertyInfo !== null check so we only do that once.
Oh, yeah. Duh.
We also probably should not filter this for isCustomComponentTag
Moving it to where we already check propertyInfo will prevent it from applying to custom component tags. 👍
|
Should this be accompanied with a warning too since this is likely a bug and you should be setting the value to undefined or null if you want this behavior? |
|
Seems like a reasonable approach that's inline with sanitizeURL. |
I don't think so.
Based on my own testing (Chrome, Firefox, Edge) adding
Great. I was hoping that was the case, and tests seemed to confirm.
Seems reasonable to add a warning. Seems odd to suggest " How about:
|
…pty strings This prevents e.g. <img src=""> from making an unnecessar HTTP request for certain browsers.
bvaughn
force-pushed
the
dom-empty-string-attribute-filter
branch
from
06d07c0
to
f8ac921
Compare
What if there's another <base href="something/else/" />
<base href="" />vs <base href="something/else/" />
<base /> |
Same behavior. The empty one does not modify the Edit Is it even valid to have multiple ones on the same page? Looks like the browser only pays attention to the first one, regardless. |
|
The spec says:
|
|
What about |
|
The warning should probably recommend not rendering the element at all as the primary recommendation and only null if that doesn’t work (i.e. if someone is too lazy to refactor). |
|
I could see someone using |
Yeah, that sounds like kind of a long and confusing warning 😉 I can change the rec. to "don't render at all" though I guess.
Yeah... that's the use case that seems most likely I guess. They could always use
@JeromeDeLeon No. This is something different. |
|
I dunno. Is this warning better?
|
|
I'm wondering why it would be necessary to filter these? |
|
@koenpunt to prevent unnecessary network requests, for example |
|
@bvaughn We were recently going through React warnings with @rachelnabors and she mentioned “no-op” is unfamiliar jargon. We’ll need to take a pass over them. But for now, I suggest:
|
|
I appreciate the re-worked error message, @gaearon! These things are difficult to write 😅 That being said, I think the middle sentence of the one you mentioned is too I think this makes sense for every case except maybe |
Maybe, but can we leave this wording for |
|
Sure. |
Treat `<a href="" />` the same with and without `enableFilterEmptyStringAttributesDOM` in #18513 we started to warn and ignore for empty `href` and `src` props since it usually hinted at a mistake. However, for anchor tags there's a valid use case since `<a href=""></a>` will by spec render a link to the current page. It could be used to reload the page without having to rely on browser affordances. The implementation for Fizz is in the spirit of #21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off.
Treat `<a href="" />` the same with and without `enableFilterEmptyStringAttributesDOM` in #18513 we started to warn and ignore for empty `href` and `src` props since it usually hinted at a mistake. However, for anchor tags there's a valid use case since `<a href=""></a>` will by spec render a link to the current page. It could be used to reload the page without having to rely on browser affordances. The implementation for Fizz is in the spirit of #21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off. DiffTrain build for [f3ce87a](f3ce87a)
Treat `<a href="" />` the same with and without `enableFilterEmptyStringAttributesDOM` in #18513 we started to warn and ignore for empty `href` and `src` props since it usually hinted at a mistake. However, for anchor tags there's a valid use case since `<a href=""></a>` will by spec render a link to the current page. It could be used to reload the page without having to rely on browser affordances. The implementation for Fizz is in the spirit of #21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off.
…28124) Treat `<a href="" />` the same with and without `enableFilterEmptyStringAttributesDOM` in facebook#18513 we started to warn and ignore for empty `href` and `src` props since it usually hinted at a mistake. However, for anchor tags there's a valid use case since `<a href=""></a>` will by spec render a link to the current page. It could be used to reload the page without having to rely on browser affordances. The implementation for Fizz is in the spirit of facebook#21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off.
This is a breaking change!
So I have put it behind a new feature flag,
enableFilterEmptyStringAttributesDOM.Scope
This flag affects the following attributes/elements:
actionformformActionbutton,inputhrefa,area,base,linksrcaudio,embed,iframe,img,input,script,source,,track,videoBehavior before
<img src={emptyStringValue} /><img src="">Behavior after
<img src={emptyStringValue} /><img>Implementation
I am not very familiar with the details of the DOM render, but I think it makes the most sense to add this to the
DOMPropertyfile. It seems to fit with the other filtering logic we have there.This approach has a possible downside, since the logic only applies to attributes and does not consider the element type. (It's possible we may want to make an exception for some of the impacted elements I listed above?)
Loosely related to PR #15047