Skip to content

Commit

Permalink
Fix autofill browser by making sure we commit the value in text inputs (
Browse files Browse the repository at this point in the history
streamlit#7150)

Describe your changes
Change commitWidgetValue to take in dirty parameter as we need to change dirty to true when committing the widget value
we need to commit the widget value because when autofill is done, the widget value is not committed as thus username will show up to be an empty string without interaction
GitHub Issue Link (if applicable)
streamlit#7101
streamlit#7084

Testing Plan
Explanation of why no additional tests are needed
Unit Tests (JS and/or Python)
readded tests I deleted before with some slight additions
E2E Tests
Any manual testing needed?
yes
it doesn't seem very doable to test autofill functionality. However, I can probably add some tests for commit value being called
Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
  • Loading branch information
willhuang1997 authored and eric-skydio committed Dec 20, 2023
1 parent 5081215 commit 1a5452b
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 5 deletions.
23 changes: 22 additions & 1 deletion frontend/lib/src/components/widgets/TextInput/TextInput.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,26 @@ describe("TextInput widget", () => {
expect(textInput).toHaveValue("0123456789")
})

it("does not update widget value on text changes when outside of a form", () => {
it("does update widget value on text changes when inside of a form", async () => {
const props = getProps({ formId: "formId" })
const setStringValueSpy = jest.spyOn(props.widgetMgr, "setStringValue")

render(<TextInput {...props} />)

const textInput = screen.getByRole("textbox")
fireEvent.change(textInput, { target: { value: "TEST" } })
expect(textInput).toHaveValue("TEST")

expect(
await screen.findByText("Press Enter to submit form")
).toBeInTheDocument()

expect(setStringValueSpy).toHaveBeenCalledWith(props.element, "TEST", {
fromUi: true,
})
})

it("does not update widget value on text changes when outside of a form", async () => {
const props = getProps()
jest.spyOn(props.widgetMgr, "setStringValue")
render(<TextInput {...props} />)
Expand All @@ -238,6 +257,8 @@ describe("TextInput widget", () => {
fireEvent.change(textInput, { target: { value: "TEST" } })
expect(textInput).toHaveValue("TEST")

expect(await screen.findByText("Press Enter to apply")).toBeInTheDocument()

// Check that the last call was in componentDidMount.
expect(props.widgetMgr.setStringValue).toHaveBeenLastCalledWith(
props.element,
Expand Down
30 changes: 26 additions & 4 deletions frontend/lib/src/components/widgets/TextInput/TextInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,23 @@ class TextInput extends React.PureComponent<Props, State> {
})
}

/** Commit state.value to the WidgetStateManager. */
private commitWidgetValue = (source: Source): void => {
/**
* Commits the current state value to the WidgetStateManager.
*
* @param source - Whether or not from the UI
* @param updateState - Optional flag to determine if the state should be updated
* to reflect that the value is no longer 'dirty' or modified.
* By default, this is true, meaning the state WILL be updated.
*/
private commitWidgetValue = (source: Source, updateState = true): void => {
this.props.widgetMgr.setStringValue(
this.props.element,
this.state.value,
source
)
this.setState({ dirty: false })
if (updateState) {
this.setState({ dirty: false })
}
}

/**
Expand Down Expand Up @@ -141,10 +150,23 @@ class TextInput extends React.PureComponent<Props, State> {
return
}

// we immediately update its widgetValue on text changes in forms
// see here for why: https://github.com/streamlit/streamlit/issues/7101
// The widgetValue won't be passed to the Python script until the form
// is submitted, so this won't cause the script to re-run.
if (isInForm(this.props.element)) {
// make sure dirty is true so that enter to submit form text shows
this.setState({ dirty: true, value }, () => {
this.commitWidgetValue({ fromUi: true }, false)
})
}
// If the TextInput is *not* part of a form, we mark it dirty but don't
// update its value in the WidgetMgr. This means that individual keypresses
// won't trigger a script re-run.
this.setState({ dirty: true, value })
else {
// make sure dirty is true so that enter to apply text shows
this.setState({ dirty: true, value })
}
}

private onKeyPress = (
Expand Down

0 comments on commit 1a5452b

Please sign in to comment.