New issue
Advanced search Search tips

Issue 839404 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Autofilling select with value collision

Project Member Reported by olivierrobin@chromium.org, May 3 2018

Issue description

HTML specification does not prevent different options to have the same value.

Example: This select would be valid

<select>
<option value="">Option 1</option>
<option value="">Option 2</option>
<option value="option3">Option 3</option>
<option value="option3">Option 4</option>
<select>

Setting a select by value would select the first option for a given value.
But it is also possible to set the selected index of the select.

Currently, autofill sends to the renderer the value of the select, which fail in this scenario.

Proposed solution 1:
When extracting option value, add an &index at the end of each value.
The extracted values would become
&0, &1, option3&2, option3&3
When autofilling, select the index corresponding to this name

Proposed solution 2:
Similar to solution 1, but only for entries with collisions.
The extracted options would become
&0, &1, option3, options3&1
Note: the first "" would become &0 as "" is already a "nothing selected" value.
This is more robust in case options are inserted.

Proposed solution 3:
Have autofill return the selected_index in autofill_field. Use this selected_index instead of value to set select value.




 
Cc: rogerm@chromium.org se...@chromium.org mahmadi@chromium.org
My preference goes to solution 2, more robust and closer to the current behavior.
Please tell me if you have a different solution or preference.
Labels: OS-Android OS-Linux OS-Mac OS-Windows
Note: I use & as a separator because &[0-9]+ is invalid in attributes values.
Looks like we are able to identify the best match index for select options on the browser side, but don't seem to be actually using it:
https://cs.chromium.org/chromium/src/components/autofill/core/browser/field_filler.cc?l=451

I'm more for solution 3, where we send the selected index back to the renderer to use rather than the selected value. Do you the implications of setting the 'selectedIndex' property of <select> elements vs 'value' when it comes to websites listening for change events on the select?
I will have to do more tests.
The good side of methods 1 and 2 is that we can use the selectindex method only when site actually has collisions.

I also continued my investigation to do the same change on desktop.
It seems that this require a change in blink as the WebSelectElement:setSelectedIndex is not exported in the public API.
I am not sure the change is worth it as it affect only one site in the 275 list.

WDYT?
We should have all the information on the browser side to detect collisions when we figure out the best match index, can we send the selected_index information as part of the autofill_field and act on it only when there are collisions? Otherwise it can have a negative value or something to indicate the return value is good enough. That way we don't change the filling behavior for websites that work. 

I feel like it's a lot more work to detect collisions on the renderer side, parse the return value, and detect the appropriate index to select. wdyt? 
Yes, the solution you describe is definitely possible.

But we will still need to expose a SetSelectedIndex for the HTMLFormControlElement to actually set the value after we send the autofill result to the renderer.
sebsg@ should know the best way to handle cases like this. He interacts with the Blink owners often. 

Comment 9 by se...@chromium.org, May 7 2018

I'm still unclear how solution proposed in comment#6 would work. How you would pass that value to the renderer so that is selects the appropriate value.

The blink folks are very open to our requests, we can ask tkent@ if we go with that route. 

Can we clarify a bit more how that communication between browser and rederer would work in regards to the selectedIndex?

We can also try to contact the site, since they are the only case I've seen of that and it looks like a mistake :)
Talked with Moe offline, the idea to pass the index sounds good to me :)
Thanks

I tried to read the HTML spec, and I don't think anything in it prevents collisions in options values.
If you use the form submission, it is better to have different values as the value is what is transmitted on form submission.
But if you use JS handlers, you can use the selected index.

So even if it is a mistake, we should probably handle this.

I will write an email to tkent@ to ask if we can expose setSelectedIndex to handle this case.

Sign in to add a comment