patternfly / patternfly-react

A set of React components for the PatternFly project.
https://react-staging.patternfly.org/
MIT License
793 stars 357 forks source link

[Bug] [2020.04] TextInput onChange lose focus #4072

Closed aljesusg closed 4 years ago

aljesusg commented 4 years ago

After upgrade when the value change the textInput lost focus, it seems that is rendering again the component in onChange event. (See Gif)

Component

<TextInput
                value={this.state.addEgressHost.host}
                type="text"
                id="addEgressHost"
                key="addEgressHost"
                aria-describedby="add egress host"
                name="addHost"
                onChange={this.onAddHost}
                isValid={this.state.validEgressHost}
              />

OnAddHost method

  onAddHost = (value: string, _) => {
    const host = value.trim();
    this.setState({
      addEgressHost: {
        host: host
      },
      validEgressHost: isServerHostValid(host)
    });
  };

GIF

ezgif-4-5a5a26e8e121

aljesusg commented 4 years ago

@redallen Do you know who I can talk to about this? Thanks !!!

redallen commented 4 years ago

Taking a look now.

redallen commented 4 years ago

I'm unable to reproduce this using your code (here's the codesandbox I made that doesn't seem to have your problem: https://codesandbox.io/s/autumn-framework-w26vd). The only change we made to TextInput in our 2020.04 release was my PR #3919 for fixing forward ref types, are you by chance passing a ref?

If you can link a reproducible example or tell me how to load this problematic page while running kiali I can help further.

aljesusg commented 4 years ago

Ok @redallen let me do some research. Thanks !!!

aljesusg commented 4 years ago

@redallen After some tests I found the problem, this happens when the component is in a table. So something changed here.

Here you have the sandbox https://codesandbox.io/s/texinput-table-ggx1l?file=/index.js

Is similar to our code https://github.com/kiali/kiali-ui/blob/master/src/pages/IstioConfigNew/GatewayForm.tsx

Somethign change here maybe we shopuld cast this to ICell ?? something change when PF render this. Notrice that all textInputs inside the row are affected

redallen commented 4 years ago

I'm investigating this now and it seems to be a problem on any <input> element inside of a table row. It also appears to occur on "@patternfly/react-table": "2.24.41" which is what kiali is using. Will update once I find a fix...

redallen commented 4 years ago

Not quite sure the root cause yet, but a temporary fix is to use TextInputBase instead of TextInput. I'm debugging more of why our Table component doesn't like forwarded refs...

aljesusg commented 4 years ago

Ok thanks @redallen , I'm going to check TextInputBase. Thanks !!!

redallen commented 4 years ago

TLDR; Another table bug.

I've spent most of the day on this and am bewildered. My next step would be to make a minimal reproduction with a stripped down version of our Table component, but I'm not quite sure how to do that yet since I lack a good understanding of how Table works with React's lifecycle methods.

To sum up what I've found, using TextInputBase causes typing in the input to only update TextInputBase inside the <Table>. However, using React.forwardRef on TextInputBase causes the whole <RowWrapper> (and maybe even higher up the tree?) to rerender, and so the <input> loses focus. I assume this has to do internally with how React implements forwardRef when diff-checking. However, the fault is in our Table since <div>{this.rows()[0].cells}</div> works just fine.

The key implementation takeaway for TextInputBase vs TextInput is that TextInputBase is a normal React Component (that is typeof TextInputBase === 'function'), while TextInput is a special React object (that is typeof TextInput === 'object') with a .render prop.

I've created a branch for debugging if anyone wants to further investigate.

aljesusg commented 4 years ago

ok @redallen is working with TextInputBase we are moving forward with this and I'm going to document this to change it in next PF4 release or after fix. Thanks

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

dgdavid commented 4 years ago

Hi there!

Just for the record, is spite of bug report is closed this still reproducible when using

    ...

    "@patternfly/patternfly": "4.50.4",
    "@patternfly/react-core": "4.63.3",
    "@patternfly/react-table": "^4.18.14",

    ...

Regards.