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

[TextField] Handle Chrome autofill #17436

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

croraf
Copy link
Contributor

@croraf croraf commented Sep 15, 2019

Fixes #14427 as per given fix by @oliviertassinari

@mui-pr-bot
Copy link

Details of bundle changes.

Comparing: e9f26fc...1e77b3f

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.07% 🔺 +0.08% 🔺 331,688 331,926 90,532 90,607
@material-ui/core/Paper 0.00% 0.00% 68,659 68,659 20,458 20,458
@material-ui/core/Paper.esm 0.00% 0.00% 62,098 62,098 19,203 19,203
@material-ui/core/Popper 0.00% 0.00% 28,397 28,397 10,159 10,159
@material-ui/core/Textarea 0.00% 0.00% 5,082 5,082 2,132 2,132
@material-ui/core/TrapFocus 0.00% 0.00% 3,766 3,766 1,596 1,596
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,348 16,348 5,818 5,818
@material-ui/core/useMediaQuery 0.00% 0.00% 2,488 2,488 1,050 1,050
@material-ui/lab 0.00% 0.00% 154,749 154,749 46,885 46,885
@material-ui/styles 0.00% 0.00% 51,420 51,420 15,288 15,288
@material-ui/system 0.00% 0.00% 15,581 15,581 4,351 4,351
Button 0.00% 0.00% 78,610 78,610 24,024 24,024
Modal 0.00% 0.00% 14,542 14,542 5,114 5,114
Portal 0.00% 0.00% 2,907 2,907 1,318 1,318
Rating 0.00% 0.00% 69,963 69,963 21,856 21,856
Slider 0.00% 0.00% 75,084 75,084 23,184 23,184
colorManipulator 0.00% 0.00% 3,835 3,835 1,519 1,519
docs.landing 0.00% 0.00% 52,232 52,232 13,768 13,768
docs.main +0.03% 🔺 +0.05% 🔺 597,197 597,393 190,673 190,763
packages/material-ui/build/umd/material-ui.production.min.js +0.08% 🔺 +0.09% 🔺 302,599 302,831 86,969 87,049

Generated by 🚫 dangerJS against 1e77b3f

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

There is no confirmed reproduction that rules out browser issues. Please include the detailed reproduction in the PR.

@oliviertassinari
Copy link
Member

The changes should solve two structural issues: 1. the JavaScript change event was fired before we listen to it. 2. The input value is hidden for security reasons. What browser issues do you have in mind?

@eps1lon
Copy link
Member

eps1lon commented Sep 17, 2019

What browser issues do you have in mind?

That was my question. It is not obvious what issue is solved here. Only theoretical issues that have no confirmed reproduction.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 17, 2019

Only theoretical issues that have no confirmed reproduction.

We have a couple of root issues:

  1. A user writes in an input before we listen to the event. This can be reproduced on the /examples/ssr/ example with a set timeout in the hydration logic.
  2. Chrome autofill the input before React hydrates the from. This can be reproduced with [TextField] Handle Chrome autofill #14427 (comment).
  3. Chrome doesn't fire a change event. This can be reproduced in reproduced [TextField] Handle Chrome autofill #14427 (comment) with BrowserStack Windows 10, Chrome latest

@eps1lon
Copy link
Member

eps1lon commented Sep 17, 2019

To be clear: A reproduction is code + environment.

@oliviertassinari oliviertassinari merged commit 1fbbb89 into mui:master Sep 18, 2019
@eps1lon
Copy link
Member

eps1lon commented Sep 18, 2019

A user writes in an input before we listen to the event. This can be reproduced on the /examples/ssr/ example with a set timeout in the hydration logic.

What is the use case for hydrating after a timeout? Seems like a strawman to me.

Chrome autofill the input before React hydrates the from. This can be reproduced with #14427 (comment).

As I said no example in that issue has a confirmable reproduction. It might reproduce for you but without knowing the environment you cannot know if this is a library or environment issue.

Chrome doesn't fire a change event. This can be reproduced in reproduced #14427 (comment) with BrowserStack Windows 10, Chrome latest

BrowserStack is not a reliable source for reproduction. If it was we would not have tabable buttons in safari.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 18, 2019

What is the use case for hydrating after a timeout?

For instance, you might want to wait for an optional polyfill to be loaded, over the network, before hydrating your DOM. I had the very use case at Onepixel, I was frustrated Next.js has no support for it.

As I said no example in that issue has a confirmable reproduction.

Then we can call it a nice side effect of fixing the previous issue.

BrowserStack is not a reliable source for the reproduction

Don't worry, we have enough users to report if these fixes don't cut it. I don't think that we should wait to get the perfect reproduction, we might never have it. I think that we should move fast (type 2, reversible changes) by sacrificing the scope of the problems we solve and the information we collect, without sacrificing the code quality so we can learn from "production". In this case, we have an opportunity to try these changes. Worse case 1. we still have this bug, people provide a better reproduction, worse case 2. we solved the bug, but we remove the logic in the future, for some reason, people report the regression or they don't. In any case, the changes are direct application of people reported solution to the problem we can find on GitHub issues, commit, blog post or StackOverflow. @croraf and I have seen a positive impact with them. Things are imperfect and will always be :).

@eps1lon
Copy link
Member

eps1lon commented Sep 18, 2019

Well thanks for adding technical debt that we need to refactor.

@oliviertassinari
Copy link
Member

@eps1lon I'm sorry you feel this way. I understand that you have done great progress in the past simplifying the logic of this component and that you want to keep it in a great state. I will personally make sure this new logic only remains if proven valuable. Meaning:

But yes, the more problems we solve, the more complex the codebase is and the more inertia we have. Sometimes, it's better not to solve problems reported by our users. I don't think it's one of these cases.
Assuming that we want to solve the problem. I don't think that we went down a quick patch road. It should be resilient.

nrkroeker added a commit to nrkroeker/material-ui that referenced this pull request Sep 18, 2019
* [docs] Batch small changes (mui#17435)

* [docs] Add synonyms for brand icons (mui#17455)

* [docs] Add synonyms for brand icons

* Remove Ic prefixed icons

* [docs] Improve in-site search (mui#17450)

* [ButtonBase] Fix blurry text issue (mui#17453)

* [docs] Revert hits per page change (mui#17458)

* [docs] Fix heading format in CONTRIBUTING.md (mui#17460)

* [Chip] Load the right version of Avatar (mui#17469)

* [TablePagination] Merge root classes properly (mui#17467)

* [Breadcrumbs] Improve API docs (mui#17468)

* [TextField] Handle Chrome autofill (mui#17436)

* [docs] Clarify props spread for ListItem when button flag is set (mui#17466)
@jkeruzec
Copy link

this update makes me change something to still have my transparent background for my input:

Separate css

/* Chrome hack for auto set password ;) */
@-webkit-keyframes autofill {
    to {
        color: white;
        background: transparent;
    }
}

Adaptation in theming, to still have this transparent background and not a grey-blue one

MuiInputBase: {
            input: {
                '&:-webkit-autofill': {
                    animationName: 'autofill',
                    animationFillMode: 'both',
                    animationDuration: '0s'
                  },
            }
        }  

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 23, 2019

@jkeruzec Do you have a reproduction?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TextField] Handle Chrome autofill
5 participants