Skip to content
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

fix(#2336): Fields without description are losing focus when an error is shown for the first time. #2422

Merged
merged 3 commits into from
Oct 15, 2021

Conversation

majornista
Copy link
Collaborator

πŸ› Bug Report
If you have a TextField without a description, the focus on the field is lost as soon as an error is shown for the first time. This probably happens for all the fields that have Helper Text option.

πŸ€” Expected Behavior
Field should keep focus.

😯 Current Behavior
Field is losing the focus when the error is shown.

πŸ’ Possible Solution
Probably use a useLayoutEffect to restore focus to the input if the input had focus previously.

πŸ”¦ Context
πŸ’» Code Sample
https://codesandbox.io/s/lost-focus-44wtq

🌍 Your Environment
Software Version(s)
react-spectrum 3.14.0
Browser Chrome
Operating System macOS 11.6

Closes #2336

βœ… Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue 2336.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

πŸ“ Test Instructions:

🧒 Your Project:

Adobe/Accessibility

… is shown for the first time.

πŸ› Bug Report
If you have a TextField without a description, the focus on the field is lost as soon as an error is shown for the first time. This probably happens for all the fields that have Helper Text option.

πŸ€” Expected Behavior
Field should keep focus.

😯 Current Behavior
Field is losing the focus when the error is shown.

πŸ’ Possible Solution
Probably use a useLayoutEffect to restore focus to the input if the input had focus previously.

πŸ”¦ Context
πŸ’» Code Sample
https://codesandbox.io/s/lost-focus-44wtq

🌍 Your Environment
Software	Version(s)
react-spectrum	3.14.0
Browser	Chrome
Operating System	macOS 11.6
@adobe-bot
Copy link

Build successful! πŸŽ‰

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Chromatic identified one issue https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=141
I'm not sure if we'll be able to do anything about it, though it's weird that it's only affecting the large scale

@majornista
Copy link
Collaborator Author

majornista commented Oct 6, 2021

Chromatic identified one issue https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=141 I'm not sure if we'll be able to do anything about it, though it's weird that it's only affecting the large scale

I think this has to do with:

/* TODO: By default, vertical flex wrapper for `labelPosition: side` should have default field width.
* This is a workaround until we find a better way to set the width of the field & help text.
* Should default to form field's default width and and allow users to override with custom width. */
width: var(--spectrum-field-default-width);
in .spectrum-Field.spectrum-Field--positionSide .spectrum-Field-wrapper, but if I remove it or change it based on hasHelpText, I get more chromatic errors in other components. like ColorField.

Can we accept the ComboBox changes as a new baseline?

Also, I notice that the NumberField layout is broken by the use of display: table-cell overriding display: flex at

@snowystinger
Copy link
Member

@majornista yeah, Forms are pretty broken already #2324
It may be that this is fine because they're already pretty bad, just pointing it out for reviews

@adobe-bot
Copy link

Build successful! πŸŽ‰

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally by porting over the example from the codesandbox into our storybook. Is there an existing story that covers this bug (couldn't find one myself)? If so, I can approve otherwise would be good to add a test story to the HelpText stories

@adobe-bot
Copy link

Build successful! πŸŽ‰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fields without description are losing focus when an error is shown for the first time.
5 participants