Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
361833004:
autofill: be more open minded about filling in states in <option>s. (Closed)
|
Created:
6 years, 5 months ago by Evan Stade Modified:
6 years, 5 months ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptionautofill: be more open minded about filling in states in <option>s.
BUG=389747
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281793
Patch Set 1 #
Total comments: 11
Patch Set 2 : choose the shortest match #Patch Set 3 : base:: #
Messages
Total messages: 19 (0 generated)
+dbeam for review unfortunately, even with this fix, Magento sites still won't work because a) autocomplete="off" b) no method="POST" on the form +isherman: do you know why (b) is a requirement for autofill?
lgtm (inb4 edge case bugs) https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field.cc:143: } why so curly? https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field_unittest.cc:238: "CA - California", "NC - North Carolina", can you reverse the order of these?
On 2014/07/01 00:01:52, Evan Stade wrote: > unfortunately, even with this fix, Magento sites still won't work because > > a) autocomplete="off" > b) no method="POST" on the form > > +isherman: do you know why (b) is a requirement for autofill? I've asked others the same question, and never gotten back a good reason. Well, I mean, if the form has method="GET", then it's supposed to not have side-effects, which means it's almost certainly not relevant for autofill. But if the form is submitted via AJAX, then it doesn't really matter what the method type is, so arguably we shouldn't pay attention to it. Also, TIL that formmethod can override a form's method. Who knew? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-formmethod https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field.cc:70: // for |value|. For example, "NC - North Carolina" would match "north carolina". Would "WV - West Virginia" match "virginia"? It seems like it would, and it seems like that's problematic... https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field_unittest.cc:238: "CA - California", "NC - North Carolina", On 2014/07/01 00:34:32, Dan Beam wrote: > can you reverse the order of these? (Or, alternately, include an empty default item.)
https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field.cc:70: // for |value|. For example, "NC - North Carolina" would match "north carolina". On 2014/07/01 00:59:08, Ilya Sherman wrote: > Would "WV - West Virginia" match "virginia"? It seems like it would, and it > seems like that's problematic... hmm, yes I suppose it would. Perhaps I could make it choose the shortest option that has a substring match, so it would prefer "VA - Virginia" over "WV - West Virginia" if both were present. If only "WV - West Virginia" is present, it would fill it incorrectly, but that seems like a rare case. https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field_unittest.cc:238: "CA - California", "NC - North Carolina", On 2014/07/01 00:59:08, Ilya Sherman wrote: > On 2014/07/01 00:34:32, Dan Beam wrote: > > can you reverse the order of these? > > (Or, alternately, include an empty default item.) I don't understand the purpose of either request.
https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field.cc:70: // for |value|. For example, "NC - North Carolina" would match "north carolina". On 2014/07/01 01:04:56, Evan Stade wrote: > On 2014/07/01 00:59:08, Ilya Sherman wrote: > > Would "WV - West Virginia" match "virginia"? It seems like it would, and it > > seems like that's problematic... > > hmm, yes I suppose it would. Perhaps I could make it choose the shortest option > that has a substring match, so it would prefer "VA - Virginia" over "WV - West > Virginia" if both were present. If only "WV - West Virginia" is present, it > would fill it incorrectly, but that seems like a rare case. Yeah, maybe. Alternately, we could just kinda assume that lists will be alphabetically sorted, and hence things will Just Work(TM). But it's at least worth documenting this potential failure case. (The only other pair with this property that I can think of is "Washington" and "Washington, D.C.". Though, our canonical list of state names lists DC as "district of columbia", so in practice that pair shouldn't tickle this particular issue.) https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field_unittest.cc:238: "CA - California", "NC - North Carolina", On 2014/07/01 01:04:56, Evan Stade wrote: > On 2014/07/01 00:59:08, Ilya Sherman wrote: > > On 2014/07/01 00:34:32, Dan Beam wrote: > > > can you reverse the order of these? > > > > (Or, alternately, include an empty default item.) > > I don't understand the purpose of either request. My reasoning, which might or might not match Dan's, is: The first value in a <select> field is the default value. So, if you start with "CA - California" as the default value, and end with it as the autofilled value, it's a little less obvious that you've actually successfully autofilled the field. If you start with something else as the default value, and then arrive at "CA - California", it's more obvious that the test is correct.
https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field_unittest.cc:238: "CA - California", "NC - North Carolina", On 2014/07/01 01:04:56, Evan Stade wrote: > On 2014/07/01 00:59:08, Ilya Sherman wrote: > > On 2014/07/01 00:34:32, Dan Beam wrote: > > > can you reverse the order of these? > > > > (Or, alternately, include an empty default item.) > > I don't understand the purpose of either request. it currently looks like this test will select "CA - California" and stop before ignoring "NC - North Carolina" (which is the point of the test, I assume).
https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field_unittest.cc:238: "CA - California", "NC - North Carolina", On 2014/07/01 01:18:46, Dan Beam wrote: > On 2014/07/01 01:04:56, Evan Stade wrote: > > On 2014/07/01 00:59:08, Ilya Sherman wrote: > > > On 2014/07/01 00:34:32, Dan Beam wrote: > > > > can you reverse the order of these? > > > > > > (Or, alternately, include an empty default item.) > > > > I don't understand the purpose of either request. > > it currently looks like this test will select "CA - California" and stop before > ignoring "NC - North Carolina" (which is the point of the test, I assume). The main point of the test is to make sure "CA - California" works, because before this patch it didn't. The inverted order case is tested in FillSelectControlWithInexactAbbreviations.
https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/361833004/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_field_unittest.cc:238: "CA - California", "NC - North Carolina", On 2014/07/01 01:17:47, Ilya Sherman wrote: > On 2014/07/01 01:04:56, Evan Stade wrote: > > On 2014/07/01 00:59:08, Ilya Sherman wrote: > > > On 2014/07/01 00:34:32, Dan Beam wrote: > > > > can you reverse the order of these? > > > > > > (Or, alternately, include an empty default item.) > > > > I don't understand the purpose of either request. > > My reasoning, which might or might not match Dan's, is: The first value in a > <select> field is the default value. So, if you start with "CA - California" as > the default value, and end with it as the autofilled value, it's a little less > obvious that you've actually successfully autofilled the field. If you start > with something else as the default value, and then arrive at "CA - California", > it's more obvious that the test is correct. But other tests (such as the one that uses kNotStates below) rely on no match ==> empty field.value. Anyway, I added a "SC - South Carolina" before California.
ping isherman
LGTM. Sorry, didn't realize you were waiting for my review.
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/361833004/40001
On 2014/07/07 23:25:47, Ilya Sherman wrote: > LGTM. Sorry, didn't realize you were waiting for my review. no worries, I was ooo the whole time
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/361833004/40001
Message was sent while issue was closed.
Change committed as 281793 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
