Bug 66032 – Allow site authors to override autofilled fields' colors.

• Component: Forms
• Status: REOPENED

[Ilya Sherman]

Allow site authors to override autofilled fields' colors.

[Ilya Sherman]

Created attachment 103578 [details]
Patch

[Ilya Sherman]

Created attachment 103579 [details]
Patch

[Ilya Sherman]

This is motivated by http://crbug.com/46543.  There is a lot of discussion there, but here is a brief summary of the reasoning:
  (1) Malicious websites can circumvent the element styling anyway, so the "!important" declaration adds no security benefits.
  (2) Other browsers do not impose a similar restriction on site authors.  For example, Firefox does not even distinguish autofilled fields (at all).
  (3) The yellow+black style conflicts with some site layouts, e.g. ones where yellow is used to indicate warnings.

For Chromium, we have decided to allow site authors to override the style.  If Safari devs are on board, we should go ahead and change "html.css", as done in the current patch.  Otherwise, we can just change the Chromium-specific style sheets.

I'm not sure whom to cc to get weigh-in from the Safari team.  If someone could point the right people to this bug, I would be much obliged :)

[Alexey Proskuryakov]

CC'ed some folks whose input would be essential to proceed with cross-platform changes.

[Simon Fraser (smfr)]

Maciej said:

- If we want changing the autofill color to be a supported feature, we should think through the design of the -webkit-autofill pseudo-class and propose it to standards groups. Opening this point of control to authors without a clear standards-track story may not be a great idea.

- From the security perspective, this adds a new way to make the contents of an autofilled form field invisible, since foreground and background color would both become settable.

[Ilya Sherman]

(In reply to comment #5)
> Maciej said:
> 
> - If we want changing the autofill color to be a supported feature, we should think through the design of the -webkit-autofill pseudo-class and propose it to standards groups. Opening this point of control to authors without a clear standards-track story may not be a great idea.

Sure, I have no objection to standardizing this pseudo-class (or something like it); though I also don't understand why it would be unwise to leave this as a rendering engine-specific selector.  If we want to pursue standardization, who would be the right people to loop in?

> - From the security perspective, this adds a new way to make the contents of an autofilled form field invisible, since foreground and background color would both become settable.

As long as users have JavaScript enabled, the foreground and background colors are already settable -- just in a hacky way.  For example, the site author could add and remove a character from the value each time onChange() is fired; or just rip out the existing <input> elements and replace them with new ones (again, copying the text values).

Since JavaScript is enabled by default, I'm not sure that this change affects the security story much.  CC'ing inferno@chromium in case he has any additional insights.

[Abhishek Arya]

I worked on the initial audit of autofill. I was discussing this with IIya. One of my major concerns here was that the autofill dropdown colors should not be controlled by website. So, it will be still on top and cannot be hided. I think that should mitigate most of the security concerns, because user knows he is navigating b/w various autofill choices before making a selection.

[Ilya Sherman]

Dear Apple folks, please add your thoughts in response to comment #6.  Or should I just move forward on this issue by making this a Chromium-only change?

[Darin Adler]

(In reply to comment #8)
> Dear Apple folks, please add your thoughts in response to comment #6. Or should I just move forward on this issue by making this a Chromium-only change?

Since Maciej is the one who said those things, I think he should probably be the one to reply.

I don’t think the security issue is really all that strong, but Maciej is far more savvy than I about such things.

[Maciej Stachowiak]

Comment on attachment 103579 [details]
Patch

(In reply to comment #9)
> (In reply to comment #8)
> > Dear Apple folks, please add your thoughts in response to comment #6. Or should I just move forward on this issue by making this a Chromium-only change?
> 
> Since Maciej is the one who said those things, I think he should probably be the one to reply.
> 
> I don’t think the security issue is really all that strong, but Maciej is far more savvy than I about such things.

I don't think the security issue is that serious either. I just gave my opinion when asked.

I *do* think the issue of making sure we have a supportable standards-track API for this, if we are going to expose the capability to Web content authors, instead of exposing a random piece of engine internals. I don't think that concern has been addressed. The right working group to contact would be the CSS Working Group. It would be easy to get at least informal input on www-style on whether an autofill pseudoclass is the right way to go).

I don't think making this a Chromium-only change would remove our responsibility to make a supportable, standards-track design. Since people apparently want me to turn my offhanded remarks into a decision of some sort, I guess I have to mark this patch r- based on what I said above. I would also r- a version of the patch that did the same thing with Chromium ifdefs if the above concern was not addressed (in fact, even more so since it would be gratuitously forking Web platform behavior).

[Ilya Sherman]

> I *do* think the issue of making sure we have a supportable standards-track API for this, if we are going to expose the capability to Web content authors, instead of exposing a random piece of engine internals. I don't think that concern has been addressed. The right working group to contact would be the CSS Working Group. It would be easy to get at least informal input on www-style on whether an autofill pseudoclass is the right way to go).

Just to clarify here:  My expectation would be that very few site authors would try to style autofilled fields differently from non-autofilled fields.  Rather -- and this is based on the comments on http://crbug.com/46543 -- most site authors want to have all fields be styled the same, whether they're autofilled or not.  Is your concern simply that authors *could* use the WebKit autofill pseudoclass after this change, whereas they could not previously?

[Simon Fraser (smfr)]

Ilya: please work with Tab to propose something on www-style

[Ilya Sherman]

Tab, how should we move forward on this?  Should I draft a brief message and run it by you?

Note to self: The pseudoattribute is already being used in the wild, e.g. for http://code.google.com/p/chromium/issues/detail?id=46543#c22 :/

[Tab Atkins]

(In reply to comment #13)
> Tab, how should we move forward on this?  Should I draft a brief message and run it by you?

Yup, that sounds great.

[Ilya Sherman]

I posted a proposal [1] to www-style, but it has pretty much been ignored.  Where to from here?

[1] http://lists.w3.org/Archives/Public/www-style/2011Oct/0409.html

[Ilya Sherman]

Created attachment 134935 [details]
Patch

[Ilya Sherman]

Caught Tantek on IRC today, and he expressed general support for exposing :-webkit-autofill (and eventually just the unprefixed :autofill) as a pseudo-class selector.  It's currently listed as a proposed selector for the CSS4-UI feature list [1], though Tantek said it might be appropriate to move it over to the CSS4 Selectors working draft [2].  There's also now a tracking page on the Mozilla Wiki [3].
[1] http://wiki.csswg.org/spec/css4-ui#more-selectors
[2] http://www.w3.org/TR/selectors4/
[3] https://wiki.mozilla.org/CSS/:autofill

[WebKit Review Bot]

Comment on attachment 134935 [details]
Patch

Clearing flags on attachment: 134935

Committed r113511: <http://trac.webkit.org/changeset/113511>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #18)
> (From update of attachment 134935 [details])
> Clearing flags on attachment: 134935
> 
> Committed r113511: <http://trac.webkit.org/changeset/113511>

This caused bug 85150.

[Maciej Stachowiak]

This may need to be reverted. Given how the CSS cascade works, there's no way to avoid bug 85150, short of changing the hook for customizing autofill style to something other than a psuedoclass. A non-!important pseudo-class rule in the UA stylesheet will always be superseded by a non-pseudo-class rule in the author stylesheet.

[Ilya Sherman]

Reopening to attach new patch.

[Ilya Sherman]

Created attachment 139562 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 139562 [details]
Patch

rollouts don't require review. :)

[Eric Seidel (no email)]

Make sure you re-open the original bug (unless it's this one?) and notify the original author (unless that's you?)

[Ilya Sherman]

(In reply to comment #25)
> Make sure you re-open the original bug (unless it's this one?) and notify the original author (unless that's you?)

Aye, this is the original bug, and I'm the original author.  I tried "webkit-patch rollout ...", but it got hung somewhere in a Python script, so I fell back to the tried and true path of "webkit-patch upload".

[WebKit Review Bot]

Comment on attachment 139562 [details]
Patch

Clearing flags on attachment: 139562

Committed r115706: <http://trac.webkit.org/changeset/115706>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ilya Sherman]

The first attempt at a fix introduced regressions, so re-opening.

Copying in two relevant comments from [ https://bugs.webkit.org/show_bug.cgi?id=85150 ]:

> Ilya Sherman

Hmm, based on my reading of [ http://www.w3.org/TR/CSS21/cascade.html#cascading-order ], the user agent style should have the lowest precedence, so this is behaving "correctly".  Do you have a recommendation on how to implement the more desired behavior, which is the the user agent style overrides the author-specified style *unless the author overrides input:-webkit-autofill* without violating the CSS spec?

> Tab Atkins Jr.

There *is* a way to get this to work, as long as the state is exclusive with other similar states.  I think it is here.

Placeholder has a basically identical problem.  The way that was discussed to fix that was to add some additional pseudo-elements.

<input> gets a ::value that wraps its normal text contents, and only accepts a handful of properties.  (This part isn't strictly necessary.)

While an <input> is showing a placeholder, it generates a ::placeholder pseudo which wraps the placeholder text.  Since this only happens when the input doesn't have any normal text, this acts "normally".

Similarly, an auto-filled input would generate an ::autofill pseudo-element that wraps its autofilled text.  Once the user starts interacting with it, the text would move from ::autofill to ::value.

This handles the specificity problem by ignoring it, because rules on "input" would only affect the ::autofill pseudo by inheritance, and inheritance is the weakest possible form of rule.

Possibly, to enforce that these states are exclusive, we should do these three as a single pseudo with an argument:  ::value(normal), ::value(placeholder), and ::value(autofill).  The input would generate one and only one of these at a time.

(Theoretically, we could do this with pseudo-classes on the pseudo-element, but that's not allowed by current Selector syntax, and it would still potentially suffer problems with authors just styling "::value" and overriding all the states.)