Bug 48382 – Autofilled form elements are assigned fixed background color but not text color

• Component: CSS
• Status: RESOLVED FIXED

[Will Hirsch]

This bug was reported as Chromium Issue 52993 - http://code.google.com/p/chromium/issues/detail?id=52993

The default html.css (and wml.css) stylesheet in WebCore assigns a background-color to elements assigned the -webkit-autofill pseudo style (i.e. that have been autofilled by the browser) but not a text color. This leads to accessibility and aesthetic issues when the inherited text color does not complement the default background color (pastel yellow).

[John Sullivan]

I think an ideal approach would be to change the text color to black only if the normal text color would not contrast enough with the autofill background color. I also think changing the text color to black in all cases would be fine.

[Abhishek Arya]

For security purposes, having a standard foreground color is best, since we dont want the evil guy to pick shades near yellow and confusing the user what is getting autofilled. Also, we should track this as low severity security bug.

[Darin Adler]

Given the large number of other ways to obscure what’s filled in -- putting elements in front, surrounding with distractions, making the fields small, transforming the field to make it tiny -- restricting the color does not seem like an effective security measure.

[Abhishek Arya]

Yeah, you are right. Plus, the filled value is already in the DOM at that point, so no obscurity is required, attacker has that anyway. removing the security tags.

[Ilya Sherman]

Created attachment 72295 [details]
Patch

[Ilya Sherman]

I didn't find an existing (set of) layout test(s) to modify or add to -- could someone please point me in the right direction?

[Adam Barth]

Comment on attachment 72295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72295&action=review

> WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

We're going to need a test for this patch.  I'm not sure how to write one, but someone on #webkit should be able to help you.  Generally, we don't accept any patches without tests.  In some sense, the tests are the most important part of the patch.

[Ilya Sherman]

Created attachment 84391 [details]
Patch

[WebKit Review Bot]

Attachment 84391 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/WebView/WebViewPrivate.h:680:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ilya Sherman]

Comment on attachment 84391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84391&action=review

2 lines of CSS, 130 lines of test code

> Source/WebKit/mac/WebView/WebView.mm:6253
> +- (void)_setAutofilled:(bool)isAutofilled forElement:(JSValueRef)element inContext:(JSContextRef)context

AFAICT, there is absolutely no platform-specific code here.  However, when I tried to implement this directly in DumpRenderTree/LayoutTestController.cpp, I got errors like the following when trying to include the appropriate header.

".../WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/JSValue.h:29:0 .../WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/JSValue.h:29:30: error: wtf/AlwaysInline.h: No such file or directory"

I'd like to share this implementation across platforms if possible, but I'm not sure how to; suggestions appreciated!

[Tony Chang]

(In reply to comment #10)
> (From update of attachment 84391 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84391&action=review
> 
> 2 lines of CSS, 130 lines of test code
> 
> > Source/WebKit/mac/WebView/WebView.mm:6253
> > +- (void)_setAutofilled:(bool)isAutofilled forElement:(JSValueRef)element inContext:(JSContextRef)context
> 
> AFAICT, there is absolutely no platform-specific code here.  However, when I tried to implement this directly in DumpRenderTree/LayoutTestController.cpp, I got errors like the following when trying to include the appropriate header.

DumpRenderTree uses the WebKit API exposed by the various ports in Source/WebKit/.  DRT can't reach into WebCore directly.

One option is to move the common code into WebCore and each WebKit API can have a method that passes the call through.  Examples of this are WebFrame::markerTextForListItem or WebFrame::selectionStartHasSpellingMarkerFor.

Also, you might want to just pass a string with the element ID and a bool to setAutofilled.  This is more like the existing test methods (e.g., elementDoesAutoCompleteForElementWithId or pauseAnimationAtTimeOnElementWithId).

[Ilya Sherman]

(In reply to comment #11)
> One option is to move the common code into WebCore and each WebKit API can have a method that passes the call through.  Examples of this are WebFrame::markerTextForListItem or WebFrame::selectionStartHasSpellingMarkerFor.

Hmm, pretty much all of the work involved is teasing the HTMLInputElement out of the JSValueRef.  Is there a good place for this sort of logic to live in WebCore?  If not, it might not be worth the effort to share this code.

It does seem odd, though, that DumpRenderTree can e.g. convert a JSValueRef to a boolean, but not to a JSValue.

> Also, you might want to just pass a string with the element ID and a bool to setAutofilled.  This is more like the existing test methods (e.g., elementDoesAutoCompleteForElementWithId or pauseAnimationAtTimeOnElementWithId).

I modeled this code after the test method computedStyleIncludingVisitedInfo, which takes an element as a parameter directly.  On the whole, I find that the resulting implementation is somewhat cleaner, which is nice.  It's also a little bit more flexible, but I guess that doesn't matter as much for tests.

[Darin Adler]

(In reply to comment #12)
> (In reply to comment #11)
> > One option is to move the common code into WebCore and each WebKit API can have a method that passes the call through.  Examples of this are WebFrame::markerTextForListItem or WebFrame::selectionStartHasSpellingMarkerFor.
> 
> Hmm, pretty much all of the work involved is teasing the HTMLInputElement out of the JSValueRef. Is there a good place for this sort of logic to live in WebCore? If not, it might not be worth the effort to share this code.

Please look at the markerTextForListItem code. You’ll see there is already code that does what you need to get to DOMElement. Getting from DOMElement to DOMHTMLInputElement is trivial.

> I modeled this code after the test method computedStyleIncludingVisitedInfo

Please model it after markerTextForListItem instead.

[David Levin]

Comment on attachment 84391 [details]
Patch

It sounds as if there are things to be addressed in the comments before this should go in so r-.

[Ilya Sherman]

Created attachment 85433 [details]
Patch

[Collabora GTK+ EWS bot]

Attachment 85433 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8133198

[Ilya Sherman]

Created attachment 85501 [details]
Patch

[Collabora GTK+ EWS bot]

Attachment 85501 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8131368

[Ilya Sherman]

Created attachment 85529 [details]
Patch

[Ilya Sherman]

PTAL =)

[Tony Chang]

Comment on attachment 85529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85529&action=review

If you fix the 2 minor issues mentioned below and upload a new patch, I'll r+ and cq+.

> LayoutTests/platform/win/fast/forms/input-autofilled-expected.txt:3
> +This tests that foreground and background colors properly change for autofilled inputs. It can only be run using DumpRenderTree.
> +
> +FAIL Foreground color did not change when autofilled.

I would remove this file and just add the file to LayoutTests/platform/win/Skipped with a comment saying that layoutTestController.setAutofilled needs to be implemented.

> Tools/DumpRenderTree/LayoutTestController.cpp:2121
> +        { "setAutofilled", setAutofilledCallback, kJSPropertyAttributeDontDelete },

Should we also set kJSPropertyAttributeReadOnly?

[Ilya Sherman]

Created attachment 85731 [details]
Patch

[Ilya Sherman]

Tony, thanks for your comments =)

(In reply to comment #21)
> I would remove this file and just add the file to LayoutTests/platform/win/Skipped with a comment saying that layoutTestController.setAutofilled needs to be implemented.

I tend to agree with you, but it seems that more reviewers prefer adding expectations for tests with consistent results -- please take a look at the comments on https://bugs.webkit.org/show_bug.cgi?id=55834

> > Tools/DumpRenderTree/LayoutTestController.cpp:2121
> > +        { "setAutofilled", setAutofilledCallback, kJSPropertyAttributeDontDelete },
> 
> Should we also set kJSPropertyAttributeReadOnly?

Ah, I was misunderstanding what this flag means.  Thanks for pointing that out =)

[Tony Chang]

(In reply to comment #23)
> Tony, thanks for your comments =)
> 
> (In reply to comment #21)
> > I would remove this file and just add the file to LayoutTests/platform/win/Skipped with a comment saying that layoutTestController.setAutofilled needs to be implemented.
> 
> I tend to agree with you, but it seems that more reviewers prefer adding expectations for tests with consistent results -- please take a look at the comments on https://bugs.webkit.org/show_bug.cgi?id=55834

Ah, OK, that seems fine.  I'm going to wait a bit before r+ cq+'ing so that the GTK+ bot can run.

[Collabora GTK+ EWS bot]

Attachment 85731 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8153015

[Ilya Sherman]

Created attachment 85781 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 85781 [details]
Patch

Clearing flags on attachment: 85781

Committed r81155: <http://trac.webkit.org/changeset/81155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.