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
[Input][Joy] Fix autofill styles #35056
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a negative margin that only applies when autofill is triggered. I would image that it might make it harder for the developers to customize the component.
I think that we could move the change of margin outside of
:-webkit-autofill
so it always apply (to catch customization bug sooner), and maybe to see if we can remove the need to a negative margin.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but it is more complicated because the Autocomplete is affected. I think it makes more sense to put them under
:webkit-autofill
if you think of it as a module.It also looks weird for simple use case when you inspect the element and found that the negative margin is there at the beginning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it's the value of doing it. I have struggled to understand how this could work in the first place because it was hard to reproduce, I needed to trigger the autofill style. So IMHO, it would be better to move the margin change to be always there, for predictability. For example, so that as a developer, when you do a customization, you don't realize 4 days later, through customer support, that the input looks broken on the login page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the problem is about triggering the autofill styles, we should fix it or let the browser take care of it.
Using a negative margin directly should not be the solution to this.
It does not guarantee that what you customize will work with autofill. For example, this won't look good in autofill styles:
I rather keep the initial CSS plain as much as possible and add extra styles for autofill. If you don't toggle the autofill styles, there is no way to be sure that the autofill styles work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To continue on this example, my concern is about:
but, a few days later, he would notice that the autofill style is broken in his form. He doesn't get this to work "for free":
'&:-webkit-autofill': {
, this would be enough:but with the current logic, we need to do this customization to get the right style in all the cases. It's harder to figure out:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this point. To be clear, my proposal is to remove
'&:-webkit-autofill': {
so that developers don't need to be in a form with the autofill triggered to be able to customize the component.I don't share the same vison on this but no objections to move forward with the current changes.
I would expect that users' feedback will utimately surface the same pain point that I raised, and if they don't, great.
At the core I think that the difference of our perspectives is about how much the autofill use case of an input inside a form is an edge case to the component or core. I think it's core, that no customization should be done without this constraint.
I agree, but I think that it shouldn't be our default go to solution. For example, we would likely need a CSS vars for padding horizontal and one for vertical. It's also not something that developers can easily find with the dev tools. It would be simpler if raw CSS was working.
If your concern is with the use of negative margins (I personally don't mind), we could maybe change the styles to not rely on them.
Off-topic. I wonder if
marginLeft
wouldn't be better thanmarginInlineStart
. It's clearer to me what the former does. I have to google the latter to understand what's going on. I would expect it's the same for all the backend developers that uses us. With the RTL plugin that swap the properties automatically, I would expect no difference in real life.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for
marginInlineStart
, we are already usingpaddingInlineStart
. Regarding the styles being added in the:webkit-autofill
, I am fine with it, having fewer styles by default, and I agree with what @siriwatknp said, you anyway need to verify that the styles for autofill work if you are doing some customizations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnajdova I would apply the same reasoning. This CSS property has an extra mental step for a developer to understand what it does compared to
paddingLeft
. The problem, is thatpaddingInlineStart
seems to solve almost no problems in exchange for the customization overhead it creates for a developer that want to change the padding left.The problem is with how much CSS developers have to write to customization the output. In the previous comments I made the point that with how it's on HEAD, developers have to write
to get the correct output. Vs how it could look like for developers if Joy UI's input was using negative margin:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. My thought was that we would advertise:
Which is the way towards which we want to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be curious to see developers' feedback on this. On my end, I can't imagine that developers would want
when they can have:
but maybe I'm wrong, so to test out 👍