Bug 66032 - Allow site authors to override autofilled fields' colors.
: Allow site authors to override autofilled fields' colors.
Status: REOPENED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Ilya Sherman
:
Depends on: 83418
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-10 23:07 PDT by Ilya Sherman
Modified: 2012-05-02 15:28 PDT (History)
17 users (show)

See Also:


Attachments
Patch (7.53 KB, patch)
2011-08-10 23:09 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (7.66 KB, patch)
2011-08-10 23:10 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (9.50 KB, patch)
2012-03-30 20:33 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (9.66 KB, patch)
2012-04-30 18:17 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Sherman 2011-08-10 23:07:57 PDT
Allow site authors to override autofilled fields' colors.
Comment 1 Ilya Sherman 2011-08-10 23:09:17 PDT
Created attachment 103578 [details]
Patch
Comment 2 Ilya Sherman 2011-08-10 23:10:25 PDT
Created attachment 103579 [details]
Patch
Comment 3 Ilya Sherman 2011-08-10 23:16:38 PDT
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 :)
Comment 4 Alexey Proskuryakov 2011-08-11 11:40:01 PDT
CC'ed some folks whose input would be essential to proceed with cross-platform changes.
Comment 5 Simon Fraser (smfr) 2011-08-11 11:43:08 PDT
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.
Comment 6 Ilya Sherman 2011-08-11 14:11:43 PDT
(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.
Comment 7 Abhishek Arya 2011-08-22 14:34:20 PDT
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.
Comment 8 Ilya Sherman 2011-09-26 15:11:24 PDT
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?
Comment 9 Darin Adler 2011-09-27 18:21:25 PDT
(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.
Comment 10 Maciej Stachowiak 2011-09-27 19:46:35 PDT
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).
Comment 11 Ilya Sherman 2011-09-27 20:21:07 PDT
> 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?
Comment 12 Simon Fraser (smfr) 2011-09-28 14:24:17 PDT
Ilya: please work with Tab to propose something on www-style
Comment 13 Ilya Sherman 2011-09-29 21:58:25 PDT
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 :/
Comment 14 Tab Atkins 2011-09-30 14:27:54 PDT
(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.
Comment 15 Ilya Sherman 2011-10-17 17:36:44 PDT
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
Comment 16 Ilya Sherman 2012-03-30 20:33:07 PDT
Created attachment 134935 [details]
Patch
Comment 17 Ilya Sherman 2012-03-30 20:40:51 PDT
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
Comment 18 WebKit Review Bot 2012-04-06 15:49:03 PDT
Comment on attachment 134935 [details]
Patch

Clearing flags on attachment: 134935

Committed r113511: <http://trac.webkit.org/changeset/113511>
Comment 19 WebKit Review Bot 2012-04-06 15:49:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 mitz@webkit.org 2012-04-28 21:10:33 PDT
(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.
Comment 21 Maciej Stachowiak 2012-04-28 22:10:17 PDT
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.
Comment 22 Ilya Sherman 2012-04-30 18:17:33 PDT
Reopening to attach new patch.
Comment 23 Ilya Sherman 2012-04-30 18:17:41 PDT
Created attachment 139562 [details]
Patch
Comment 24 Eric Seidel 2012-04-30 18:27:45 PDT
Comment on attachment 139562 [details]
Patch

rollouts don't require review. :)
Comment 25 Eric Seidel 2012-04-30 18:28:33 PDT
Make sure you re-open the original bug (unless it's this one?) and notify the original author (unless that's you?)
Comment 26 Ilya Sherman 2012-04-30 18:34:01 PDT
(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".
Comment 27 WebKit Review Bot 2012-04-30 20:21:11 PDT
Comment on attachment 139562 [details]
Patch

Clearing flags on attachment: 139562

Committed r115706: <http://trac.webkit.org/changeset/115706>
Comment 28 WebKit Review Bot 2012-04-30 20:21:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Ilya Sherman 2012-05-02 15:28:48 PDT
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.)