-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Remount ReactDOMInput when changing type #2242
Conversation
I just ran into this bug working through an placeholder implementation for IE8. The workaround we came up with is to set a different key depending on the type of the input in order to signal to redraw the entire component when the type changes. |
Accepted – @syranide feel free to merge after we branch for 0.13. |
@spicyj Hmm, I made Sounds good? |
Yeah, okay. |
This no longer works with the new implementation of ReactDOMInput. :\ |
This no longer merges cleanly and has been stale for a few months so I am closing. Additionally, we no longer support IE8 officially in React 15, so bug fixes to it have a much lower priority now. |
Addresses T168684688 (#2242). MergeReactiveScopesThatInvalidateTogether does not merge scopes if their output is not guaranteed to change when their inputs do. So for example a case such as `{session_id: bar(props.bar)}` will not merge the scopes for `t0 = bar(props.bar)` and `t1 = {session_id: t0}`, because t0 isn't guaranteed to change when `props.bar` does, and we want to avoid recreating the t1 object unless it semantically changes. But there's a special case: if a scope has no dependencies, then we'll never execute it again anyway. So it doesn't matter what kind of value it produces and it's safe to merge with subsequent scopes: ```javascript return {session_id: bar()} ``` Without the reactive input, `bar()` will always return the same value since we'll only ever call it once anyway. So it's safe to then merge with the scope for the outer object literal.
IE8 throws if you change input
type
, It applies to all values oftype
(even unsupported ones).Additionally, all other versions of IE does not respect
checked
unlesstype="checkbox"
ortype="radio"
. So changing fromtype="textfield"
totype="checkbox"
whilechecked
causes it to be unchecked. Which should be an issue for React, except thatReactDOMInput
explicitly callsDOMPropertyOperations.setValueForProperty
for everycomponentDidUpdate
(source link). Why? Seems wasteful.It seems to me that the correct solution (to all of this) is to consider a different
type
as a different component.type
is a really weird HTML feature, which to me is akin to transformingspan
intodiv
rather than remounting... which we don't.cc @zpao @spicyj
(PS. PR just because, not saying we should take it as-is)