Bug 66032

Created attachment 103578 [details] Patch

Created attachment 103579 [details] Patch

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 :)

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

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.

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

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.

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?

(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 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).

> 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?

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

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 :/

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

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

Created attachment 134935 [details] Patch

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 on attachment 134935 [details] Patch Clearing flags on attachment: 134935 Committed r113511: <http://trac.webkit.org/changeset/113511>

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.

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.

Reopening to attach new patch.

Created attachment 139562 [details] Patch

Comment on attachment 139562 [details] Patch rollouts don't require review. :)

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

(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 on attachment 139562 [details] Patch Clearing flags on attachment: 139562 Committed r115706: <http://trac.webkit.org/changeset/115706>

All reviewed patches have been landed. Closing bug.

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