Add Autofill preview support for <select> input fields

Add Autofill preview support for <select> input fields

The <select> preview is set with yellow background inline with Input and
TextArea Element preview css settings.

R=dhollowa@chromium.org, isherman@chromium.org, jhawkins@chromium.org, tkent@chromium.org
BUG=58719
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169476

[tkent, 2014-01-14 23:55:07]

not lgtm

https://codereview.chromium.org/138433002/diff/1/LayoutTests/fast/forms/sugge...
File LayoutTests/fast/forms/suggested-value-expected.txt (right):

https://codereview.chromium.org/138433002/diff/1/LayoutTests/fast/forms/sugge...
LayoutTests/fast/forms/suggested-value-expected.txt:32: | "select.value: TX"
The value must not be changed even when the suggested value is set.

[Ilya Sherman, 2014-02-08 01:36:10]

As I mentioned via email, rather than changing the selectedIndex, you'll want to
introduce a new variable to track the suggestedIndex.  The goal of the suggested
values is to not leak any information to JavaScript; for a <select> element,
that includes both the value and the selected index.

[ziran.sun, 2014-02-28 15:51:53]

Code updated and addressed comments from tkent and Ilya. Please review. Thanks!

[tkent, 2014-03-04 01:09:00]

+keishi

https://codereview.chromium.org/138433002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderListBox.cpp (right):

https://codereview.chromium.org/138433002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderListBox.cpp:343: // Focus the previewed item.
How does it look like?  We need a rendering test.

[tkent, 2014-03-04 01:10:21]

Also, how should autofill preview work for <select multiple>?

[ziran.sun, 2014-03-04 16:52:34]

On 2014/03/04 01:09:00, tkent wrote:
> +keishi
> 
>
https://codereview.chromium.org/138433002/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderListBox.cpp (right):
> 
>
https://codereview.chromium.org/138433002/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderListBox.cpp:343: // Focus the previewed item.
> How does it look like?  We need a rendering test.

Sorry, this code shouldn't be there as focus on select is not called during
autofill preview. I've updated the code.

[ziran.sun, 2014-03-05 11:51:58]

On 2014/03/04 01:10:21, tkent wrote:
> Also, how should autofill preview work for <select multiple>?

Just checked with Ilya - autofill currently doesn't support <select multiple>
form fields.

[keishi, 2014-03-13 04:47:49]

lgtm.

https://codereview.chromium.org/138433002/diff/100001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/138433002/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:258: return m_suggestedValue;
It doesn't matter yet, but, HTMLInputElement seems to be returning the sanitized
value. Maybe we should return an empty string if the option with that value
doesn't exist.

[ziran.sun, 2014-03-13 11:20:51]

Code updated as per keishi's review comment. Please review.

https://codereview.chromium.org/138433002/diff/100001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/138433002/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:258: return m_suggestedValue;
On 2014/03/13 04:47:50, keishi1 wrote:
> It doesn't matter yet, but, HTMLInputElement seems to be returning the
sanitized
> value. Maybe we should return an empty string if the option with that value
> doesn't exist.

Done.

[ziran.sun, 2014-03-13 17:07:24]

On 2014/03/13 11:20:51, ziran.sun wrote:
> Code updated as per keishi's review comment. Please review.
> 
>
https://codereview.chromium.org/138433002/diff/100001/Source/core/html/HTMLSe...
> File Source/core/html/HTMLSelectElement.cpp (right):
> 
>
https://codereview.chromium.org/138433002/diff/100001/Source/core/html/HTMLSe...
> Source/core/html/HTMLSelectElement.cpp:258: return m_suggestedValue;
> On 2014/03/13 04:47:50, keishi1 wrote:
> > It doesn't matter yet, but, HTMLInputElement seems to be returning the
> sanitized
> > value. Maybe we should return an empty string if the option with that value
> > doesn't exist.
> 
> Done.

Hi tkent, 

Does the code look okay to you?

Thanks!

[tkent, 2014-03-13 23:30:49]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:30:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/138433002/120001

[commit-bot: I haz the power, 2014-03-13 23:41:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:41:48]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[tkent, 2014-03-14 00:06:40]

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:259: for (unsigned i = 0; i <
items.size(); i++) {
We prefer ++i.

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:277: for (unsigned i = 0; i <
items.size(); i++) {
Ditto.

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:847: setNeedsValidityCheck();
I don't think validity check is necessary. Updating suggestion value/index
should not affect element validity.

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.h (right):

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.h:212: String m_suggestedValue;
Why m_suggestedValue is necessary?  Isn't m_suggestedIndex enough?

https://codereview.chromium.org/138433002/diff/120001/public/web/WebSelectEle...
File public/web/WebSelectElement.h (left):

https://codereview.chromium.org/138433002/diff/120001/public/web/WebSelectEle...
public/web/WebSelectElement.h:67: 
This change is unnecessary.

[ziran.sun, 2014-03-14 11:32:36]

Updated code as per tkent's review comments. Please review. Thanks!

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:259: for (unsigned i = 0; i <
items.size(); i++) {
On 2014/03/14 00:06:41, tkent wrote:
> We prefer ++i.

Done.

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:277: for (unsigned i = 0; i <
items.size(); i++) {
On 2014/03/14 00:06:41, tkent wrote:
> Ditto.

Done.

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:847: setNeedsValidityCheck();
On 2014/03/14 00:06:41, tkent wrote:
> I don't think validity check is necessary. Updating suggestion value/index
> should not affect element validity.

Done.

https://codereview.chromium.org/138433002/diff/120001/public/web/WebSelectEle...
File public/web/WebSelectElement.h (left):

https://codereview.chromium.org/138433002/diff/120001/public/web/WebSelectEle...
public/web/WebSelectElement.h:67: 
On 2014/03/14 00:06:41, tkent wrote:
> This change is unnecessary.

Done.

[ziran.sun, 2014-03-14 11:33:40]

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.h (right):

https://codereview.chromium.org/138433002/diff/120001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.h:212: String m_suggestedValue;
On 2014/03/14 00:06:41, tkent wrote:
> Why m_suggestedValue is necessary?  Isn't m_suggestedIndex enough?

Done.

[tkent, 2014-03-16 23:34:03]

https://codereview.chromium.org/138433002/diff/140001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/138433002/diff/140001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:261: if (i == (unsigned)m_suggestedIndex)
We don't use C-style cast.
static_cast<unsigned>(m_suggestedIndex)

[Inactive, 2014-03-16 23:49:42]

https://codereview.chromium.org/138433002/diff/140001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/138433002/diff/140001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:260: if
(items[i]->hasLocalName(optionTag) && m_suggestedIndex >= 0) {
isHTMLOptionElement()

https://codereview.chromium.org/138433002/diff/140001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:278: if
(items[i]->hasLocalName(optionTag)) {
Ditto

[Inactive, 2014-03-16 23:58:45]

https://codereview.chromium.org/138433002/diff/140001/Source/web/WebFormContr...
File Source/web/WebFormControlElement.cpp (right):

https://codereview.chromium.org/138433002/diff/140001/Source/web/WebFormContr...
Source/web/WebFormControlElement.cpp:122: if
(m_private->hasTagName(HTMLNames::selectTag))
This will also be isHTMLSelectElement(*m_private) once
https://codereview.chromium.org/200723002/ lands (currently in the cq).

https://codereview.chromium.org/138433002/diff/140001/Source/web/WebFormContr...
Source/web/WebFormControlElement.cpp:132: if
(m_private->hasTagName(HTMLNames::selectTag))
Ditto.

[ziran.sun, 2014-03-17 16:41:10]

Code updated as per tkent and Chris's review comments. Please review. Thanks!

https://codereview.chromium.org/138433002/diff/140001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/138433002/diff/140001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:260: if
(items[i]->hasLocalName(optionTag) && m_suggestedIndex >= 0) {
On 2014/03/16 23:49:43, Chris Dumez wrote:
> isHTMLOptionElement()

Done.

https://codereview.chromium.org/138433002/diff/140001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:261: if (i == (unsigned)m_suggestedIndex)
On 2014/03/16 23:34:04, tkent wrote:
> We don't use C-style cast.
> static_cast<unsigned>(m_suggestedIndex)

Done.

https://codereview.chromium.org/138433002/diff/140001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:278: if
(items[i]->hasLocalName(optionTag)) {
On 2014/03/16 23:49:43, Chris Dumez wrote:
> Ditto

Done.

https://codereview.chromium.org/138433002/diff/140001/Source/web/WebFormContr...
File Source/web/WebFormControlElement.cpp (right):

https://codereview.chromium.org/138433002/diff/140001/Source/web/WebFormContr...
Source/web/WebFormControlElement.cpp:122: if
(m_private->hasTagName(HTMLNames::selectTag))
On 2014/03/16 23:58:47, Chris Dumez wrote:
> This will also be isHTMLSelectElement(*m_private) once
> https://codereview.chromium.org/200723002/ lands (currently in the cq).

Done.

https://codereview.chromium.org/138433002/diff/140001/Source/web/WebFormContr...
Source/web/WebFormControlElement.cpp:132: if
(m_private->hasTagName(HTMLNames::selectTag))
On 2014/03/16 23:58:47, Chris Dumez wrote:
> Ditto.

Done.

[tkent, 2014-03-17 20:51:34]

lgtm

[commit-bot: I haz the power, 2014-03-17 20:51:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/138433002/160001

[commit-bot: I haz the power, 2014-03-17 20:52:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-17 20:53:01]

Failed to apply patch for Source/core/rendering/RenderListBox.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderListBox.cpp
  Hunk #1 FAILED at 426.
  Hunk #2 FAILED at 460.
  2 out of 2 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderListBox.cpp.rej

Patch:       Source/core/rendering/RenderListBox.cpp
Index: Source/core/rendering/RenderListBox.cpp
diff --git a/Source/core/rendering/RenderListBox.cpp
b/Source/core/rendering/RenderListBox.cpp
index
2e8b3be39cafffe3b4205f1174e05cb6d9c08203..8293870ba54ecb9d43c5019640df2e646504fd99
100644
--- a/Source/core/rendering/RenderListBox.cpp
+++ b/Source/core/rendering/RenderListBox.cpp
@@ -426,7 +426,7 @@ void RenderListBox::paintItemForeground(PaintInfo&
paintInfo, const LayoutPoint&
     applyTextTransform(style(), itemText, ' ');
 
     Color textColor = element->renderStyle() ?
resolveColor(element->renderStyle(), CSSPropertyColor) :
resolveColor(CSSPropertyColor);
-    if (isOptionElement && toHTMLOptionElement(element)->selected()) {
+    if (isOptionElement && ((toHTMLOptionElement(element)->selected() &&
select->suggestedIndex() < 0) || listIndex == select->suggestedIndex())) {
         if (frame()->selection().isFocusedAndActive() &&
document().focusedElement() == node())
             textColor =
RenderTheme::theme().activeListBoxSelectionForegroundColor();
         // Honor the foreground color for disabled items
@@ -460,7 +460,7 @@ void RenderListBox::paintItemBackground(PaintInfo&
paintInfo, const LayoutPoint&
     HTMLElement* element = listItems[listIndex];
 
     Color backColor;
-    if (element->hasTagName(optionTag) &&
toHTMLOptionElement(element)->selected()) {
+    if (element->hasTagName(optionTag) &&
((toHTMLOptionElement(element)->selected() && selectElement()->suggestedIndex()
< 0) || listIndex == selectElement()->suggestedIndex())) {
         if (frame()->selection().isFocusedAndActive() &&
document().focusedElement() == node())
             backColor =
RenderTheme::theme().activeListBoxSelectionBackgroundColor();
         else

[ziran.sun, 2014-03-18 11:32:11]

The CQ bit was checked by ziran.sun@samsung.com

[commit-bot: I haz the power, 2014-03-18 11:32:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/138433002/180001

[commit-bot: I haz the power, 2014-03-18 11:39:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 11:39:21]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[ziran.sun, 2014-03-18 11:46:07]

The CQ bit was checked by ziran.sun@samsung.com

[commit-bot: I haz the power, 2014-03-18 11:46:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/138433002/180001

[commit-bot: I haz the power, 2014-03-18 11:52:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 11:52:54]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[tkent, 2014-03-18 13:56:25]

https://codereview.chromium.org/138433002/diff/180001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.h (right):

https://codereview.chromium.org/138433002/diff/180001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.h:212: int m_suggestedIndex;
This should be initialized in the constructor.

[ziran.sun, 2014-03-18 14:06:39]

initialized m_suggestedIndex in constructor. Please review. Thanks!

https://codereview.chromium.org/138433002/diff/180001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.h (right):

https://codereview.chromium.org/138433002/diff/180001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.h:212: int m_suggestedIndex;
On 2014/03/18 13:56:26, tkent wrote:
> This should be initialized in the constructor.

Done.

[tkent, 2014-03-18 19:46:24]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-18 19:46:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/138433002/190001

[commit-bot: I haz the power, 2014-03-18 20:34:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:34:36]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[tkent, 2014-03-18 20:39:18]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:39:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/138433002/190001

[commit-bot: I haz the power, 2014-03-18 21:25:53]

Change committed as 169476

[tkent, 2014-03-19 01:14:06]

What happens if a suggested value was set, then <option>s are deleted/inserted
in the <select>?
We should have a pixel test to check <select> with -webktit-autofill style.

[ziran.sun, 2014-03-21 13:41:15]

On 2014/03/19 01:14:06, tkent wrote:
> What happens if a suggested value was set, then <option>s are deleted/inserted
> in the <select>?
> We should have a pixel test to check <select> with -webktit-autofill style.

I will check on these. Thanks very much for the thoughts!

[ziran.sun, 2014-03-24 15:21:43]

On 2014/03/21 13:41:15, ziran.sun wrote:
> On 2014/03/19 01:14:06, tkent wrote:
> > What happens if a suggested value was set, then <option>s are
deleted/inserted
> > in the <select>?

Hi tkent, what happens for the case that a value rather than a suggested value
was set? Is there any test code for this use case?

> > We should have a pixel test to check <select> with -webktit-autofill style.
> 
> I will check on these. Thanks very much for the thoughts!

[ziran.sun, 2014-03-27 14:34:30]

On 2014/03/19 01:14:06, tkent wrote:
> We should have a pixel test to check <select> with -webktit-autofill style.

https://codereview.chromium.org/214693004/

Added Layout test for <select>. I feel that this layout test has tested the
-webkit-autofill style. Do let me know if you still think that a pixel test is
still necessary. Thanks!