[Fizz] Implement all the DOM attributes and special cases #21153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: f8ef4ff...dbc8fb9 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
gaearon
reviewed
Mar 31, 2021
gaearon
reviewed
Mar 31, 2021
gaearon
reviewed
Mar 31, 2021
gaearon
reviewed
Mar 31, 2021
gaearon
reviewed
Mar 31, 2021
gaearon
reviewed
Mar 31, 2021
gaearon
reviewed
Mar 31, 2021
We read the currently selected value from the FormatContext.
We don't change to lower case at runtime anymore but keep the warning.
sebmarkbage
commented
Mar 31, 2021
gaearon
reviewed
Mar 31, 2021
gaearon
reviewed
Mar 31, 2021
gaearon
reviewed
Apr 1, 2021
This is because if you do the equivalent on the client using innerHTML, this is the effect you'd get.
sebmarkbage
force-pushed
the
fizzattributes
branch
from
fb90e51
to
dbc8fb9
Compare
gaearon
reviewed
Apr 1, 2021
gaearon
approved these changes
Apr 1, 2021
acdlite
pushed a commit
to acdlite/react
that referenced
this pull request
Apr 11, 2021
…1153) * Implement DOM format config structure * Styles * Input warnings * Textarea special cases * Select special cases * Option special cases We read the currently selected value from the FormatContext. * Warning for non-lower case HTML We don't change to lower case at runtime anymore but keep the warning. * Pre tags innerHTML needs to be prefixed This is because if you do the equivalent on the client using innerHTML, this is the effect you'd get. * Extract errors
acdlite
pushed a commit
to acdlite/react
that referenced
this pull request
Apr 13, 2021
…1153) * Implement DOM format config structure * Styles * Input warnings * Textarea special cases * Select special cases * Option special cases We read the currently selected value from the FormatContext. * Warning for non-lower case HTML We don't change to lower case at runtime anymore but keep the warning. * Pre tags innerHTML needs to be prefixed This is because if you do the equivalent on the client using innerHTML, this is the effect you'd get. * Extract errors
acdlite
pushed a commit
to acdlite/react
that referenced
this pull request
Apr 16, 2021
…1153) * Implement DOM format config structure * Styles * Input warnings * Textarea special cases * Select special cases * Option special cases We read the currently selected value from the FormatContext. * Warning for non-lower case HTML We don't change to lower case at runtime anymore but keep the warning. * Pre tags innerHTML needs to be prefixed This is because if you do the equivalent on the client using innerHTML, this is the effect you'd get. * Extract errors
acdlite
pushed a commit
to acdlite/react
that referenced
this pull request
Apr 16, 2021
…1153) * Implement DOM format config structure * Styles * Input warnings * Textarea special cases * Select special cases * Option special cases We read the currently selected value from the FormatContext. * Warning for non-lower case HTML We don't change to lower case at runtime anymore but keep the warning. * Pre tags innerHTML needs to be prefixed This is because if you do the equivalent on the client using innerHTML, this is the effect you'd get. * Extract errors
acdlite
pushed a commit
to acdlite/react
that referenced
this pull request
Apr 19, 2021
…1153) * Implement DOM format config structure * Styles * Input warnings * Textarea special cases * Select special cases * Option special cases We read the currently selected value from the FormatContext. * Warning for non-lower case HTML We don't change to lower case at runtime anymore but keep the warning. * Pre tags innerHTML needs to be prefixed This is because if you do the equivalent on the client using innerHTML, this is the effect you'd get. * Extract errors
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Apr 20, 2021
Summary: This sync includes the following changes: - **[f7cdc8936](facebook/react@f7cdc8936 )**: Also turn off enableSyncDefaultUpdates in RN test renderer ([#21293](facebook/react#21293)) //<Ricky>// - **[4c9eb2af1](facebook/react@4c9eb2af1 )**: Add dynamic flags to React Native ([#21291](facebook/react#21291)) //<Ricky>// - **[9eddfbf5a](facebook/react@9eddfbf5a )**: [Fizz] Two More Fixes ([#21288](facebook/react#21288)) //<Sebastian Markbåge>// - **[11b07597e](facebook/react@11b07597e )**: Fix classes ([#21283](facebook/react#21283)) //<Sebastian Markbåge>// - **[96d00b9bb](facebook/react@96d00b9bb )**: [Fizz] Random Fixes ([#21277](facebook/react#21277)) //<Sebastian Markbåge>// - **[81ef53953](facebook/react@81ef53953 )**: Always insert a dummy node with an ID into fallbacks ([#21272](facebook/react#21272)) //<Sebastian Markbåge>// - **[a4a940d7a](facebook/react@a4a940d7a )**: [Fizz] Add unsupported Portal/Scope components ([#21261](facebook/react#21261)) //<Sebastian Markbåge>// - **[f4d7a0f1e](facebook/react@f4d7a0f1e )**: Implement useOpaqueIdentifier ([#21260](facebook/react#21260)) //<Sebastian Markbåge>// - **[dde875dfb](facebook/react@dde875dfb )**: [Fizz] Implement Hooks ([#21257](facebook/react#21257)) //<Sebastian Markbåge>// - **[a597c2f5d](facebook/react@a597c2f5d )**: [Fizz] Fix reentrancy bug ([#21270](facebook/react#21270)) //<Sebastian Markbåge>// - **[15e779d92](facebook/react@15e779d92 )**: Reconciler should inject its own version into DevTools hook ([#21268](facebook/react#21268)) //<Brian Vaughn>// - **[4f76a28c9](facebook/react@4f76a28c9 )**: [Fizz] Implement New Context ([#21255](facebook/react#21255)) //<Sebastian Markbåge>// - **[82ef450e0](facebook/react@82ef450e0 )**: remove obsolete SharedArrayBuffer ESLint config ([#21259](facebook/react#21259)) //<Henry Q. Dineen>// - **[dbadfa2c3](facebook/react@dbadfa2c3 )**: [Fizz] Classes Follow Up ([#21253](facebook/react#21253)) //<Sebastian Markbåge>// - **[686b635b7](facebook/react@686b635b7 )**: Prevent reading canonical property of null ([#21242](facebook/react#21242)) //<Joshua Gross>// - **[bb88ce95a](facebook/react@bb88ce95a )**: Bugfix: Don't rely on `finishedLanes` for passive effects ([#21233](facebook/react#21233)) //<Andrew Clark>// - **[343710c92](facebook/react@343710c92 )**: [Fizz] Fragments and Iterable support ([#21228](facebook/react#21228)) //<Sebastian Markbåge>// - **[933880b45](facebook/react@933880b45 )**: Make time-slicing opt-in ([#21072](facebook/react#21072)) //<Ricky>// - **[b0407b55f](facebook/react@b0407b55f )**: Support more empty types ([#21225](facebook/react#21225)) //<Sebastian Markbåge>// - **[39713716a](facebook/react@39713716a )**: Merge isObject branches ([#21226](facebook/react#21226)) //<Sebastian Markbåge>// - **[8a4a59c72](facebook/react@8a4a59c72 )**: Remove textarea special case from child fiber ([#21222](facebook/react#21222)) //<Sebastian Markbåge>// - **[dc108b0f5](facebook/react@dc108b0f5 )**: Track which fibers scheduled the current render work ([#15658](facebook/react#15658)) //<Brian Vaughn>// - **[6ea749170](facebook/react@6ea749170 )**: Fix typo in comment ([#21214](facebook/react#21214)) //<inokawa>// - **[b38ac13f9](facebook/react@b38ac13f9 )**: DevTools: Add post-commit hook ([#21183](facebook/react#21183)) //<Brian Vaughn>// - **[b943aeba8](facebook/react@b943aeba8 )**: Fix: Passive effect updates are never sync ([#21215](facebook/react#21215)) //<Andrew Clark>// - **[d389c54d1](facebook/react@d389c54d1 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21211](facebook/react#21211)) //<Brian Vaughn>// - **[c486dc1a4](facebook/react@c486dc1a4 )**: Remove unnecessary processUpdateQueue ([#21199](facebook/react#21199)) //<Sebastian Markbåge>// - **[cf45a623a](facebook/react@cf45a623a )**: [Fizz] Implement Classes ([#21200](facebook/react#21200)) //<Sebastian Markbåge>// - **[75c616554](facebook/react@75c616554 )**: Include actual type of `Profiler#id` on type mismatch ([#20306](facebook/react#20306)) //<Sebastian Silbermann>// - **[1214b302e](facebook/react@1214b302e )**: test: Fix "couldn't locate all inline snapshots" ([#21205](facebook/react#21205)) //<Sebastian Silbermann>// - **[1a02d2792](facebook/react@1a02d2792 )**: style: delete unused isHost check ([#21203](facebook/react#21203)) //<wangao>// - **[782f689ca](facebook/react@782f689ca )**: Don't double invoke getDerivedStateFromProps for module pattern ([#21193](facebook/react#21193)) //<Sebastian Markbåge>// - **[e90c76a65](facebook/react@e90c76a65 )**: Revert "Offscreen: Use JS stack to track hidden/unhidden subtree state" ([#21194](facebook/react#21194)) //<Brian Vaughn>// - **[1f8583de8](facebook/react@1f8583de8 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21192](facebook/react#21192)) //<Brian Vaughn>// - **[ad6e6ec7b](facebook/react@ad6e6ec7b )**: [Fizz] Prepare Recursive Loop for More Types ([#21186](facebook/react#21186)) //<Sebastian Markbåge>// - **[172e89b4b](facebook/react@172e89b4b )**: Reland Remove redundant initial of isArray ([#21188](facebook/react#21188)) //<Sebastian Markbåge>// - **[7c1ba2b57](facebook/react@7c1ba2b57 )**: Proposed new Suspense layout effect semantics ([#21079](facebook/react#21079)) //<Brian Vaughn>// - **[316aa3686](facebook/react@316aa3686 )**: [Scheduler] Fix de-opt caused by out-of-bounds access ([#21147](facebook/react#21147)) //<Andrey Marchenko>// - **[b4f119cdf](facebook/react@b4f119cdf )**: Revert "Remove redundant initial of isArray ([#21163](facebook/react#21163))" //<Sebastian Markbage>// - **[c03197063](facebook/react@c03197063 )**: Revert "apply prettier ([#21165](facebook/react#21165))" //<Sebastian Markbage>// - **[94fd1214d](facebook/react@94fd1214d )**: apply prettier ([#21165](facebook/react#21165)) //<Behnam Mohammadi>// - **[b130a0f5c](facebook/react@b130a0f5c )**: Remove redundant initial of isArray ([#21163](facebook/react#21163)) //<Behnam Mohammadi>// - **[2c9fef32d](facebook/react@2c9fef32d )**: Remove redundant initial of hasOwnProperty ([#21134](facebook/react#21134)) //<Behnam Mohammadi>// - **[1cf9978d8](facebook/react@1cf9978d8 )**: Don't pass internals to callbacks ([#21161](facebook/react#21161)) //<Sebastian Markbåge>// - **[b9e4c10e9](facebook/react@b9e4c10e9 )**: [Fizz] Implement all the DOM attributes and special cases ([#21153](facebook/react#21153)) //<Sebastian Markbåge>// - **[f8ef4ff57](facebook/react@f8ef4ff57 )**: Flush discrete passive effects before paint ([#21150](facebook/react#21150)) //<Andrew Clark>// - **[b48b38af6](facebook/react@b48b38af6 )**: Support nesting of startTransition and flushSync (alt) ([#21149](facebook/react#21149)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions c9aab1c...f7cdc89 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D27740113 fbshipit-source-id: 6e27204d78e3e16ed205170006cb97c0d6bfa957
koto
pushed a commit
to koto/react
that referenced
this pull request
Jun 15, 2021
…1153) * Implement DOM format config structure * Styles * Input warnings * Textarea special cases * Select special cases * Option special cases We read the currently selected value from the FormatContext. * Warning for non-lower case HTML We don't change to lower case at runtime anymore but keep the warning. * Pre tags innerHTML needs to be prefixed This is because if you do the equivalent on the client using innerHTML, this is the effect you'd get. * Extract errors
eps1lon
added a commit
that referenced
this pull request
Jan 30, 2024
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.
github-actions bot
pushed a commit
that referenced
this pull request
Jan 30, 2024
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)
gaearon
pushed a commit
that referenced
this pull request
Feb 3, 2024
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.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements all the DOM bindings parts as far as I know (except the reactroot attribute). I can't run it against our test suite yet. But I carefully ported everything I could.
There's only one breakage I decided to do. We don't force lower case HTML tags before comparing them. This means that special cases like
<select />,<textarea />etc. don't work if you use a different case. We warned about this before and warn about it after, so shouldn't matter.I did a very different structure from what we've done in the DOM bindings before though.
A problem with how we used to do it is that the logic is spread out into a lot of different concepts. This causes a lot of extra checks for each element because we check those things over and over. It also leads to unnecessary checks. A lot of the helpers check things like
== nullover and over or they overcheck because in that context it can never happen.Instead of a bunch of
isSomething(...)helpers. I structured it fully based on a switch over each kind of value once and from there the code flow determines what to do next.The structure is something like this:
First we switch on the type, then we loop over each property, we switch over the property name to find special cases, then we switch over the propertyInfo and finally the value type and that determines what we do.
So the goal is that we only check each of these once along that path.
All special cases are encoded along with that specific element type. This ends up needing to fork the loop. So there's a lot of duplication there. This is controversial because if you make a change to these things you have to replicate them. However, I've noticed subtle little issues because we don't consider how each special case applies to each tag. So this is actually a benefit. It's also better for perf.
Large switches might not be better than Map of Functions depending on VM but that's something we can automate if needed if it's expressed as a simple switch.