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

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

Merged
merged 4 commits into from
Jan 20, 2025

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Oct 16, 2024

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:
Screenshot 2025-01-15 at 11 59 58

@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

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 fd75829

@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.

@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

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
@mj12albert
Copy link
Member

mj12albert commented Dec 10, 2024

The controlled state is unsynced until user interaction:

These lines in InputBase should expose a JS animation hook, that seems intended to be used as a autofill event, from SO. This may provide a way to sync the controlled state @DiegoAndai

Related issues: #14453, #14427

An alternative way relies on doing a computed style check, though we probably don't want to incur the performance cost on TextField: https://github.com/clerk/javascript/pull/4560/files

@DiegoAndai DiegoAndai requested a review from Janpot January 15, 2025 15:00
@DiegoAndai DiegoAndai marked this pull request as ready for review January 15, 2025 15:00
@DiegoAndai
Copy link
Member Author

I finally got the time to get back to this one.

  • About testing: I figured out how to test it by firing the animationStart ({ animationName: 'mui-auto-fill' }) event. The test fails without the fix
  • About the controlled state becoming unsynced: I couldn't find a way to improve this, Chrome simply doesn't trigger the onChange event before user interaction. It also won't allow accessing the input value on the animationStart event, that's why we mock the value:
    checkDirty(event.animationName === 'mui-auto-fill-cancel' ? inputRef.current : { value: 'x' });

    Also, it's different from browser to browser: https://stackoverflow.com/a/11710295. InputBase provides a JS hook like @mj12albert suggested, onFilled, but the value is unavailable at that point, so it would only work for styling purposes.

I propose merging this PR to fix the filled state synchronization issue, and opening an issue about the controlled state that we can come back to after migrating to Base UI (to either fix it or add a warning on the docs).

Ready for review @Janpot @aarongarciah

@DiegoAndai DiegoAndai removed the on hold There is a blocker, we need to wait label Jan 15, 2025
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

looks good

Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

Nice improvement!

@DiegoAndai DiegoAndai merged commit cb2a191 into mui:master Jan 20, 2025
19 checks passed
@DiegoAndai DiegoAndai deleted the text-field-autofill-style branch January 20, 2025 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TextField] Styling bug when controlled component uses autoFill
5 participants