Set the correct initial value on input range

[Illu]

Resolves #12926

Sets the value of the input range to empty, instead of the default value from the input component.
This prevents the input from not updating when first switching to the default value (or max value in case of max being inferior to this default value (50))

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[aweary]

Thanks for this @Illu! I think this is good overall. I'm slightly worried that it sets currentValue to the wrong value in cases where it has a valid initial value.

For example, if you're hydrating server-rendered markup the value will be already be correct because the element wasn't created with DOM APIs. This solution doesn't break that because initialValue is still the source of truth for the current value, but it might be confusing if we ever use currentValue in another way here.

We could potentially avoid this by using a more explicit check like:

if (currentValue === '') {
 // ...
} else if (props.type === 'range') {
  node.value = initialValue
}

In either case it would be great if we could add a comment explaining what's happening. Something like:

With range inputs node.value may be a default value calculated from the min/max attributes. This ensures that node.value is set with the correct value coming from props.

[Illu]

Thanks a lot for your feedback ! I'll take your recommendations and update the PR.

[nhunzaker]

Hey! This is on my radar. I have to wrap some stuff at work, but then I can definite get eyes on this today.

[nhunzaker]

I worry that this will cause range inputs to revert any user interaction. Basically:

1. Server-rendered markup comes down with a range input.
2. The user slides a range input
3. JavaScript executes
4. React incorrectly reverts the value of the range input

I've created two test cases that I think prove this, do you mind if I submit them to this pull request?

[aweary]

@nhunzaker it seems like there isn't a way to identify whether a range input's value is from user interaction before hydration or calculated based on min/max.

React incorrectly reverts the value of the range input

For controlled inputs, isn't that what should happen? If the state is initialized to 0, the user changes the range input to 5, it should revert 0 when React hydrates since we don't dispatch change events for changes prior to hydration.

[nhunzaker]

@aweary take a look at this test:

https://github.com/facebook/react/blob/master/packages/react-dom/src/__tests__/ReactDOMServerIntegrationForms-test.js#L543-L552

it('should not blow away user-entered text on successful reconnect to a controlled input', async () => {
  let changeCount = 0;
  await testUserInteractionBeforeClientRender(
    <ControlledInput onChange={() => changeCount++} />,
  );
  // note that there's a strong argument to be made that the DOM revival
  // algorithm should notice that the user has changed the value and fire
  // an onChange. however, it does not now, so that's what this tests.
  expect(changeCount).toBe(0);
});

I think, right now, the current behavior is that it doesn't revert the state. And it also doesn't fire a change event (which seems like a bug to me, but prior work seems to acknowledge that).

[nhunzaker]

Is it possible that the root of the problem is that the guard to determine if a SSR input was changed is wrong?

https://github.com/facebook/react/blob/master/packages/react-dom/src/client/ReactDOMFiberInput.js#L215-L224

if (currentValue === '')

I wonder if this should be:

if (currentValue === node.defaultValue)

Server rendered markup will have a value attribute (defaultValue) so:

1. If a user changes an SSR input the currentValue will not be the defaultValue, never firing the block and avoiding reverting the value incorrectly.
2. Client-rendered inputs won't have a value attribute yet because it's stripped out of the initial props assignment, so it will always report as an empty string (and whatever a range input returns back). So this should always be true.

Either way, @Illu: do you know if it is possible to get unit test coverage on the incorrect change event firing? It would be awesome if we could automate verifying this.

[nhunzaker]

Took a bit more time to understand this. I think the root of the problem is that our check for modified SSR markup is too naive. I don't know the best way to do this, but one option is to simply check to see if a value attribute is already defined on the input.

// Now we know we don't have SSR markup
if (!node.hasAttribute('value')) {

For the SSR case, the value attribute does the job of assigning an initial value to the input. But is this the best check to verify markup is hydrating? Could we pass this information into ReactDOMFiberInput?

Here's a diff of my proposed changes, which I maintains the fix provided in this PR:
Illu/react@init-input-range-value...nhunzaker:nh-range-input-update

[aweary]

take a look at this test

😱 I didn't realize that. I can see why we'd do that to avoid losing user input, but it totally puts the DOM out of sync with state which seems really bad. It assumes the user will continue updating the value after hydration.

Client-rendered inputs won't have a value attribute yet because it's stripped out of the initial props assignment, so it will always report as an empty string (and whatever a range input returns back). So this should always be true.

The problem with that is that defaultValue doesn't reflect the "default" value for range inputs. Which sounds totally insane now that I write it out.

const input = document.createElement('input');
input.type = 'range';
input.value;
// > "50"
input.defaultValue
// > ""

[nhunzaker]

@aweary you know... I wonder if we could fire a change event if we know an input is already on the page and its value differs from what React things. That could totally live in the postMountHook.

[aweary]

@nhunzaker I was just thinking the same thing. We could check if node.value and node.getAttribute('value') were different. If they were, assume that the input was changed before hydration.

Also, I think the hasAttribute check seems like a good solution.

[nhunzaker]

Sorry to rapid fire....

I think it would be a huge win if we could avoid a heuristic to check of an input was rendered server side. Like pass a boolean if React.hydrate was used for the current working tree. Then we could fire a change event, with complete confidence, if the value of the input differed from the value React prop.

[aweary]

@nhunzaker the call sites for the post mount hooks differ for CSR (setInitialProperties) and hydration (diffHydratedProperties), so it should be a simple change to add an additional isHydrating argument to the hooks.

[whs-dot-hk]

Hi, sorry for this late comment.
It was bed time in HK.

Is this test valid?

whs-dot-hk@5dc4073

I been able to compute value is equal to 0

and both onInput and onChange fired

---

and onInput did always fire in the browser

https://codesandbox.io/s/81m5w3q4ql (max=60)
https://codesandbox.io/s/llx9714o7m (max=10)

but onChange

---

MOST IMPORTANT! the defaultValue reflects the previous value, the value tracker and node value are corrent

https://codesandbox.io/s/5zon211194

---

onInput gives the correct results

https://codesandbox.io/s/lr2x77np87

---

@aweary @nhunzaker So, I think it is not related to value, but event binding

Which related to this function

react/packages/react-dom/src/client/inputValueTracking.js

Line 115 in dd5fad2

export function updateValueIfChanged(node: ElementWithValueTracker) {

I think the problem is the tracker doesn't exists, which return false in the function above

[Illu]

I've applied @nhunzaker 's changes and added the isHydrating argument.

However I didn't manage to create a unit test for the incorrect event firing yet.

[whs-dot-hk]

@awery Could you explain more why this works? Why we need to set the value of range here?

Because, the value of the range was always zero. Or the control of the range will be at end (value = 10).

Let me show you

https://whs-dot-hk.github.io/react-bug-12926-test/

with this change

export function postMountWrapper(element: Element, props: Object) {
  const node = ((element: any): InputWithWrapperState);

  if (props.hasOwnProperty('value') || props.hasOwnProperty('defaultValue')) {
    const initialValue = '' + node._wrapperState.initialValue;
    const currentValue = node.value;

    // Do not assign value if it is already set. This prevents user text input
    // from being lost during SSR hydration.
    if (currentValue === '') {
      // Do not re-assign the value property if there is no change. This
      // potentially avoids a DOM write and prevents Firefox (~60.0.1) from
      // prematurely marking required inputs as invalid
      if (initialValue !== currentValue) {
        node.value = initialValue;
      }
    }

    // value must be assigned before defaultValue. This fixes an issue where the
    // visually displayed value of date inputs disappears on mobile Safari and Chrome:
    // https://github.com/facebook/react/issues/7233
    node.defaultValue = initialValue;

    node.value = 10;
  }

I just added node.value = 10 at the end as you explained

[nhunzaker]

Woah nice. You added the isHydrating part! Very cool. I have a few questions:

1. Can you use isHydrating as the condition on line 129, like if (!isHydrating)?
2. Are the lines between (218-225) necessary? I think the bug was that range inputs never reported "", as required by the old code on line 229, that prevented the value assignment from happening.

Basically, I think the only change we need to make is on line 229:

if (!isHydrating) {
  // ....
}

[Illu]

Oh, you're absolutely right! I'll update it right away.

[nhunzaker]

This looks 💰 to me. @aweary do you mind taking a quick look?

Separately, we now have a pretty convenient place to trigger a change event for SSR hydrated inputs. I've jotted down a reminder to write up a ticket for that.

[whs-dot-hk]

I dig into the factory ReactDOMFiberComponent.js

and make this fix

master...whs-dot-hk:master

hostProps happen to be before the value tracker is register

The demo is here

https://whs-dot-hk.github.io/react-bug-12926-test/

old approval

[aweary]

Looks great! I love how more much explicit the hydration case is. This does make ReactDOMFiberInput.postMountWrapper inconsistent with the other postMountWrapper methods, but that's probably fine.

Separately, we now have a pretty convenient place to trigger a change event for SSR hydrated inputs. I've jotted down a reminder to write up a ticket for that.

@nhunzaker I opened #12955 to track.

@gaearon does this look OK to you?

[gaearon]

I think this makes sense

[nhunzaker]

Awesome. Thanks @Illu! This was a wonderful fix and a very insightful discussion.