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.

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

Jump to bottom

[material-ui][TextField] Fix filled state to be synced with autofill #44135

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[material-ui][TextField] Fix filled state to be synced with autofill #44135

wants to merge 1 commit into from

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Oct 16, 2024 •
edited
Loading

Revert some of the TextField changes in #34849.

Fixes #36448
Explanation of the fix: #36448 (comment)

Repro instructions:

  1. Open sandbox
  2. Fill both TextFields and submit
  3. Save password in Google Passwords Manager
  4. Reload

Before

Sandbox: https://codesandbox.io/p/sandbox/pr-44135-before-zrzxy8

State after reload:
Screenshot 2024-10-16 at 17 38 27

After

Sandbox: https://codesandbox.io/p/sandbox/pr-44135-after-48gdc3

State after reload:
The background color is set by Chrome, we don't handle it at the moment. The fix is that the labels are correctly placed.

Screenshot 2024-10-16 at 17 41 18

@DiegoAndai DiegoAndai added component: text field This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Oct 16, 2024
@DiegoAndai DiegoAndai self-assigned this Oct 16, 2024
@mui-bot
Copy link

mui-bot commented Oct 16, 2024 •
edited
Loading

Netlify deploy preview

https://deploy-preview-44135--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 6b32256

@DiegoAndai DiegoAndai changed the title [material-ui][TextField] Move onFilled and onEmpty out of shared useMemo [material-ui][TextField] Fix filled state to be synced with autofill Oct 16, 2024
@DiegoAndai
Copy link
Member Author

cc: @Janpot as the author of #34849. Let us know if there's something this PR overlooks 😊.

@DiegoAndai DiegoAndai marked this pull request as ready for review October 16, 2024 20:46
@DiegoAndai
Copy link
Member Author

@aarongarciah, in #36448 (comment), I mention that the eventual correct solution to this is relying on the :autofill pseudoselector. Sadly, this won't work as the pseudoselector is also set after user interaction, so it would suffer from the same issue of the label being misplaced until the user interacts with the page.

This was referenced Oct 16, 2024
Open
Open
@DiegoAndai
Copy link
Member Author

Sadly, testing autofill is not supported ATM in Playwright: microsoft/playwright#26831. Testing with onChange wouldn't mimic the behavior either.

@Janpot
Copy link
Member

Janpot commented Oct 17, 2024

Sadly, testing autofill is not supported ATM in Playwright: microsoft/playwright#26831. Testing with onChange wouldn't mimic the behavior either.

@DiegoAndai Would it be possible to mimic the behavior in jsdom? We have fine grained control over which events fire exactly.

@DiegoAndai
Copy link
Member Author

DiegoAndai commented Oct 17, 2024 •
edited
Loading

While trying to figure out how to test this, I realized there's another problem here I hadn't considered: The controlled state is unsynced until user interaction:

Screen.Recording.2024-10-17.at.14.47.28.mov

This has not yet been solved in this PR. I'm putting it on hold and come back to it after my OOO period.

@DiegoAndai DiegoAndai added the on hold There is a blocker, we need to wait label Oct 17, 2024
@DiegoAndai DiegoAndai marked this pull request as draft October 17, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarongarciah aarongarciah Awaiting requested review from aarongarciah

At least 1 approving review is required to merge this pull request.

Assignees

@DiegoAndai DiegoAndai

Labels
component: text field This is the name of the generic UI component, not the React module! on hold There is a blocker, we need to wait package: material-ui Specific to @mui/material
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TextField] Styling bug when controlled component uses autoFill
3 participants
@DiegoAndai @mui-bot @Janpot