Skip to content

Fix: Form validation runs on blur in New task details page #24011

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Aug 7, 2023
Merged

Fix: Form validation runs on blur in New task details page #24011

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f7bab13
fix: form validation runs on blur
tienifr Aug 1, 2023
c16ef56
fix lint
tienifr Aug 1, 2023
dde8208
refocus text input in Chrome only
tienifr Aug 1, 2023
671ca30
fix lint
tienifr Aug 1, 2023
81010c0
fix lint
tienifr Aug 1, 2023
4897c4d
Merge branch 'main' into fix/23552
tienifr Aug 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions src/components/Form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import ScrollViewWithContext from './ScrollViewWithContext';
import stylePropTypes from '../styles/stylePropTypes';
import {withNetwork} from './OnyxProvider';
import networkPropTypes from './networkPropTypes';
import Visibility from '../libs/Visibility';

const propTypes = {
/** A unique Onyx key identifying the form */
Expand Down Expand Up @@ -301,18 +302,21 @@ function Form(props) {
defaultValue: undefined,
errorText: errors[inputID] || fieldErrorMessage,
onBlur: (event) => {
// We delay the validation in order to prevent Checkbox loss of focus when
// the user are focusing a TextInput and proceeds to toggle a CheckBox in
// web and mobile web platforms.
setTimeout(() => {
setTouchedInput(inputID);

// To prevent server errors from being cleared inadvertently, we only run validation on blur if any form values have changed since the last validation/submit
const shouldValidate = !_.isEqual(inputValues, lastValidatedValues.current);
if (shouldValidate) {
onValidate(inputValues);
}
}, 200);
// Only run validation when user proactively blurs the input.
if (Visibility.isVisible() && Visibility.hasFocus()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always prefer early returns.

Copy link
Contributor Author

@tienifr tienifr Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks but we still have other codes below which are not relating to the check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move the other code above this if block if there are no issue with it and then use return.

// We delay the validation in order to prevent Checkbox loss of focus when
// the user are focusing a TextInput and proceeds to toggle a CheckBox in
// web and mobile web platforms.
setTimeout(() => {
setTouchedInput(inputID);

// To prevent server errors from being cleared inadvertently, we only run validation on blur if any form values have changed since the last validation/submit
const shouldValidate = !_.isEqual(inputValues, lastValidatedValues.current);
if (shouldValidate) {
onValidate(inputValues);
}
}, 200);
}

if (_.isFunction(child.props.onBlur)) {
child.props.onBlur(event);
Expand Down
3 changes: 2 additions & 1 deletion src/components/TextInput/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import BaseTextInput from './BaseTextInput';
import * as baseTextInputPropTypes from './baseTextInputPropTypes';
import DomUtils from '../../libs/DomUtils';
import Visibility from '../../libs/Visibility';
import * as Browser from '../../libs/Browser';

class TextInput extends React.Component {
componentDidMount() {
Expand All @@ -19,7 +20,7 @@ class TextInput extends React.Component {

// Forcefully activate the soft keyboard when the user switches between tabs while input was focused.
this.removeVisibilityListener = Visibility.onVisibilityChange(() => {
if (!Visibility.isVisible() || !this.textInput || DomUtils.getActiveElement() !== this.textInput) {
if (!Browser.isMobileChrome() || !Visibility.isVisible() || !this.textInput || DomUtils.getActiveElement() !== this.textInput) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the original implementation. You will find more summary on the issue and PR.

Copy link
Contributor Author

@tienifr tienifr Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you specifically point out what it did break? As I've tested on both Safari (attached above) and Chrome (attached below), it still worked.

video_2023-08-03_18-09-19.mp4

In short, the original issue only happened on Chrome, Safari would be fine without the PR. The only thing that is not right without it is:

iOS Safari: The soft keyboard does not remain open (due to a system limitation which only allows the soft keyboard to be open from a user interaction event). The screen automatically scrolls to the email input.

On iOS Safari, the keyboard still opens but screen won't scroll automatically until we type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check it and let you know later. But it was my intuition that it might break. I wasn't sure on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks in advance!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run the tests from #21656 and make sure they are intact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @eVoloshchak

return;
}
this.textInput.blur();
Expand Down