Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
243013004:
i18n address editing in chrome://settings/autofillEditAddress. (Closed)
|
Created:
6 years, 8 months ago by please use gerrit instead Modified:
6 years, 7 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, benquan, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioni18n address editing in chrome://settings/autofillEditAddress.
BUG=333387
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268095
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Templates. #
Total comments: 2
Patch Set 3 : Manual templates. #
Total comments: 21
Patch Set 4 : Address comments. #
Total comments: 16
Patch Set 5 : Addressed comments. #
Total comments: 3
Patch Set 6 : Use instanceof in tests. #Patch Set 7 : Fix phone validation. #
Total comments: 4
Patch Set 8 : Address nits. #Patch Set 9 : Fix windows build. #
Total comments: 21
Messages
Total messages: 38 (0 generated)
Evan: PTAL Patch Set 1. Still todo: add some browser tests.
https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:235: item = 'admin-area'; ditto https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:299: clearInputFields_: function() { you should be able to use a clever query selector + a loop https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:326: item = 'admin-area'; this is missing a dollar sign https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:406: getRecipientNameHtml_: function() { ditto the below comment about templates https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:430: rebuildInputFields_: function(components) { please don't write html in js strings. You can use a template (html defined in an html file that you clone and then update the attributes/content/etc.). https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:461: content += '<input id="' + components[i].field + '" type="text">'; the id should be more specific, "company" is not specific enough for all of settings IMO. In fact I would just make these classes instead of IDs, then select them with something like "#someIdForThePage input.street-address-list" https://codereview.chromium.org/243013004/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/243013004/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:136: } vertical whitespace in this function
Evan: PTAL Patch Set 2. https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:235: item = 'admin-area'; On 2014/04/22 00:51:58, Evan Stade wrote: > ditto Done. https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:299: clearInputFields_: function() { On 2014/04/22 00:51:58, Evan Stade wrote: > you should be able to use a clever query selector + a loop Done. https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:326: item = 'admin-area'; On 2014/04/22 00:51:58, Evan Stade wrote: > this is missing a dollar sign Done. https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:406: getRecipientNameHtml_: function() { On 2014/04/22 00:51:58, Evan Stade wrote: > ditto the below comment about templates Done. https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:430: rebuildInputFields_: function(components) { On 2014/04/22 00:51:58, Evan Stade wrote: > please don't write html in js strings. You can use a template (html defined in > an html file that you clone and then update the attributes/content/etc.). Done. https://codereview.chromium.org/243013004/diff/160001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:461: content += '<input id="' + components[i].field + '" type="text">'; On 2014/04/22 00:51:58, Evan Stade wrote: > the id should be more specific, "company" is not specific enough for all of > settings IMO. In fact I would just make these classes instead of IDs, then > select them with something like "#someIdForThePage input.street-address-list" Done. https://codereview.chromium.org/243013004/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/243013004/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:136: } On 2014/04/22 00:51:58, Evan Stade wrote: > vertical whitespace in this function Done.
https://codereview.chromium.org/243013004/diff/270001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/270001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:7: <div jsdisplay="field == 'fullName'" jsselect="$this"> jst is not what I was talking about. <label id="address-field-template" hidden> <div></div> <input type="text"> </label> then in js var field = $('addres-field-template').cloneNode(); node.querySelector('div').textContent = foo; row.appendChild(node); etc.
Evan: PTAL Patch Set 3. https://codereview.chromium.org/243013004/diff/270001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/270001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:7: <div jsdisplay="field == 'fullName'" jsselect="$this"> On 2014/04/24 21:05:51, Evan Stade wrote: > jst is not what I was talking about. > > <label id="address-field-template" hidden> > <div></div> > <input type="text"> > </label> > > then in js > > var field = $('addres-field-template').cloneNode(); > node.querySelector('div').textContent = foo; > row.appendChild(node); > > etc. Done.
https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> I think we should standardize the number of name fields (i.e. just have one full name field instead of 3 name pieces here, to match the dialog) https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:51: <list class="full-name-list" field="fullName"></list> why is this a list inside a div, whereas below you just have a list? https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:55: <div id="autofill-addr-lines-template"> hmm, this should be a textarea, if we are going to follow our own recommendation to web devs. https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:379: input.setAttribute('class', components[i][j].length); classList.add https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:388: if (list) { shouldn't list always be truthy? checking it here implies it is ok for it to be missing. I understand |components| might not have it, but that would be an error, no? Maybe you should just do this in the appropriate case in the switch block.
Evan: PTAL Patch Set 4. Since I removed the three-part name input (first, middle, last), the HTML templates became an overkill. Therefore, the patch now builds the input fields in JS. https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/25 20:02:18, Evan Stade wrote: > I think we should standardize the number of name fields (i.e. just have one full > name field instead of 3 name pieces here, to match the dialog) Done. https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:51: <list class="full-name-list" field="fullName"></list> On 2014/04/25 20:02:18, Evan Stade wrote: > why is this a list inside a div, whereas below you just have a list? No longer relevant, as we're no longer using first, middle, last template for names. https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:55: <div id="autofill-addr-lines-template"> On 2014/04/25 20:02:18, Evan Stade wrote: > hmm, this should be a textarea, if we are going to follow our own recommendation > to web devs. Done. https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:379: input.setAttribute('class', components[i][j].length); On 2014/04/25 20:02:18, Evan Stade wrote: > classList.add Done. https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:388: if (list) { On 2014/04/25 20:02:18, Evan Stade wrote: > shouldn't list always be truthy? checking it here implies it is ok for it to be > missing. I understand |components| might not have it, but that would be an > error, no? > > Maybe you should just do this in the appropriate case in the switch block. Done.
https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > On 2014/04/25 20:02:18, Evan Stade wrote: > > I think we should standardize the number of name fields (i.e. just have one > full > > name field instead of 3 name pieces here, to match the dialog) > > Done. +isherman, any opinion on this change? https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:8: /** @const */ var OverlaySelector = '#autofill-edit-address-overlay '; not useful. Use this.querySelector in most places. https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:246: for (var i = 0; i < fields.length; i++) loops should always have curlies https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:354: var customFieldElements = {'fullName': 'div'}; what's wrong with making this a label? https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:370: var label = document.createElement('div'); it's confusing that field is a label while label is a div https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:381: if (input.tagName == 'LIST') { This seems vaguely better: if (input instanceOf HTMLListElement) https://codereview.chromium.org/243013004/diff/420001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/243013004/diff/420001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:59: static const char kLanguageCode[] = "languageCOde"; s/COde/Code https://codereview.chromium.org/243013004/diff/420001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:607: if (args->GetList(10, &list_value)) because of these types of changes, I prefer arg_counter++ instead of numerals
https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/28 23:23:16, Evan Stade wrote: > On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > > On 2014/04/25 20:02:18, Evan Stade wrote: > > > I think we should standardize the number of name fields (i.e. just have one > > full > > > name field instead of 3 name pieces here, to match the dialog) > > > > Done. > > +isherman, any opinion on this change? Websites will often request first, middle, and last names separately. At the same time, Chrome has very basic heuristics in place to parse a full name into components. If we offer just a single full name field, then users have no ability to correct Chrome's mistakes if they want to. That's not great -- it means that Autofill will *never* work correctly for users like "Leonardo da Vinci", or any other name that our simple heuristics fail on. For rAc, we have the opportunity to explicitly discourage websites from asking for tokenized names. For regular Autofill, we have to deal with the common legacy case. So, I think we need to keep some way for users to correct Chrome's tokenization of their name. If we want a single field by default, we could do something complex like what https://contacts.google.com does.
https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/28 23:43:44, Ilya Sherman wrote: > On 2014/04/28 23:23:16, Evan Stade wrote: > > On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > > > On 2014/04/25 20:02:18, Evan Stade wrote: > > > > I think we should standardize the number of name fields (i.e. just have > one > > > full > > > > name field instead of 3 name pieces here, to match the dialog) > > > > > > Done. > > > > +isherman, any opinion on this change? > > Websites will often request first, middle, and last names separately. At the > same time, Chrome has very basic heuristics in place to parse a full name into > components. If we offer just a single full name field, then users have no > ability to correct Chrome's mistakes if they want to. That's not great -- it > means that Autofill will *never* work correctly for users like "Leonardo da > Vinci", or any other name that our simple heuristics fail on. > Isn't the inverse also true? If we have multiple input fields, then Yao Ming can add his name in chrome://settings/autofill, which will work for autocomplete="given-name" and "family-name", but when we try to fill it into a full name field it will come out as Ming Yao, and he can't fix that. Also, wouldn't Leonardo da Vinci work with our heuristics? First name is the first word (Leonardo), last name is everything else (da Vinci). But yea, Billy Bob Thornton wouldn't work. Seems like either way, it won't work for someone. > For rAc, we have the opportunity to explicitly discourage websites from asking > for tokenized names. For regular Autofill, we have to deal with the common > legacy case. So, I think we need to keep some way for users to correct Chrome's > tokenization of their name. If we want a single field by default, we could do > something complex like what https://contacts.google.com does. But data typed into a rAc dialog feeds into the legacy cases as well...
https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/28 23:56:37, Evan Stade wrote: > On 2014/04/28 23:43:44, Ilya Sherman wrote: > > On 2014/04/28 23:23:16, Evan Stade wrote: > > > On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > > > > On 2014/04/25 20:02:18, Evan Stade wrote: > > > > > I think we should standardize the number of name fields (i.e. just have > > one > > > > full > > > > > name field instead of 3 name pieces here, to match the dialog) > > > > > > > > Done. > > > > > > +isherman, any opinion on this change? > > > > Websites will often request first, middle, and last names separately. At the > > same time, Chrome has very basic heuristics in place to parse a full name into > > components. If we offer just a single full name field, then users have no > > ability to correct Chrome's mistakes if they want to. That's not great -- it > > means that Autofill will *never* work correctly for users like "Leonardo da > > Vinci", or any other name that our simple heuristics fail on. > > > > Isn't the inverse also true? If we have multiple input fields, then Yao Ming can > add his name in chrome://settings/autofill, which will work for > autocomplete="given-name" and "family-name", but when we try to fill it into a > full name field it will come out as Ming Yao, and he can't fix that. I'm not following this example. I think you mean that the full name field always fills as "first name + middle name + last name", whereas for some names the family name should come first? If so, I think that's something that we should fix as best we can as part of the localization work. > Also, wouldn't Leonardo da Vinci work with our heuristics? First name is the > first word (Leonardo), last name is everything else (da Vinci). But yea, Billy > Bob Thornton wouldn't work. "da" would be treated as a middle name, rather than as part of the last name. > Seems like either way, it won't work for someone. > > > For rAc, we have the opportunity to explicitly discourage websites from asking > > for tokenized names. For regular Autofill, we have to deal with the common > > legacy case. So, I think we need to keep some way for users to correct > Chrome's > > tokenization of their name. If we want a single field by default, we could do > > something complex like what https://contacts.google.com does. > > But data typed into a rAc dialog feeds into the legacy cases as well... True, and users can edit data originally entered into an rAc via the chrome://settings/autofill UI.
https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/29 00:08:29, Ilya Sherman wrote: > On 2014/04/28 23:56:37, Evan Stade wrote: > > On 2014/04/28 23:43:44, Ilya Sherman wrote: > > > On 2014/04/28 23:23:16, Evan Stade wrote: > > > > On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > > > > > On 2014/04/25 20:02:18, Evan Stade wrote: > > > > > > I think we should standardize the number of name fields (i.e. just > have > > > one > > > > > full > > > > > > name field instead of 3 name pieces here, to match the dialog) > > > > > > > > > > Done. > > > > > > > > +isherman, any opinion on this change? > > > > > > Websites will often request first, middle, and last names separately. At > the > > > same time, Chrome has very basic heuristics in place to parse a full name > into > > > components. If we offer just a single full name field, then users have no > > > ability to correct Chrome's mistakes if they want to. That's not great -- > it > > > means that Autofill will *never* work correctly for users like "Leonardo da > > > Vinci", or any other name that our simple heuristics fail on. > > > > > > > Isn't the inverse also true? If we have multiple input fields, then Yao Ming > can > > add his name in chrome://settings/autofill, which will work for > > autocomplete="given-name" and "family-name", but when we try to fill it into a > > full name field it will come out as Ming Yao, and he can't fix that. > > I'm not following this example. I think you mean that the full name field > always fills as "first name + middle name + last name", whereas for some names > the family name should come first? If so, I think that's something that we > should fix as best we can as part of the localization work. yes, that is what I mean. > > > Also, wouldn't Leonardo da Vinci work with our heuristics? First name is the > > first word (Leonardo), last name is everything else (da Vinci). But yea, Billy > > Bob Thornton wouldn't work. > > "da" would be treated as a middle name, rather than as part of the last name. That seems like a mistake. How often do people actually type middle names? Probably less often than having 2-word last names at least according to my totally unfounded guesstimates. If site (a) uses full-name and then site (b) uses separate names, the default should be to use ALL the name from (a) on (b), without forcing the user to navigate to chrome://settings/autofill, which realistically is rarely if ever going to happen. > > > Seems like either way, it won't work for someone. > > > > > For rAc, we have the opportunity to explicitly discourage websites from > asking > > > for tokenized names. For regular Autofill, we have to deal with the common > > > legacy case. So, I think we need to keep some way for users to correct > > Chrome's > > > tokenization of their name. If we want a single field by default, we could > do > > > something complex like what https://contacts.google.com does. > > > > But data typed into a rAc dialog feeds into the legacy cases as well... > > True, and users can edit data originally entered into an rAc via the > chrome://settings/autofill UI.
https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/29 00:24:41, Evan Stade wrote: > On 2014/04/29 00:08:29, Ilya Sherman wrote: > > On 2014/04/28 23:56:37, Evan Stade wrote: > > > On 2014/04/28 23:43:44, Ilya Sherman wrote: > > > > On 2014/04/28 23:23:16, Evan Stade wrote: > > > > > On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > > > > > > On 2014/04/25 20:02:18, Evan Stade wrote: > > > > > > > I think we should standardize the number of name fields (i.e. just > > have > > > > one > > > > > > full > > > > > > > name field instead of 3 name pieces here, to match the dialog) > > > > > > > > > > > > Done. > > > > > > > > > > +isherman, any opinion on this change? > > > > > > > > Websites will often request first, middle, and last names separately. At > > the > > > > same time, Chrome has very basic heuristics in place to parse a full name > > into > > > > components. If we offer just a single full name field, then users have no > > > > ability to correct Chrome's mistakes if they want to. That's not great -- > > it > > > > means that Autofill will *never* work correctly for users like "Leonardo > da > > > > Vinci", or any other name that our simple heuristics fail on. > > > > > > > > > > Isn't the inverse also true? If we have multiple input fields, then Yao Ming > > can > > > add his name in chrome://settings/autofill, which will work for > > > autocomplete="given-name" and "family-name", but when we try to fill it into > a > > > full name field it will come out as Ming Yao, and he can't fix that. > > > > I'm not following this example. I think you mean that the full name field > > always fills as "first name + middle name + last name", whereas for some names > > the family name should come first? If so, I think that's something that we > > should fix as best we can as part of the localization work. > > yes, that is what I mean. > > > > > > Also, wouldn't Leonardo da Vinci work with our heuristics? First name is the > > > first word (Leonardo), last name is everything else (da Vinci). But yea, > Billy > > > Bob Thornton wouldn't work. > > > > "da" would be treated as a middle name, rather than as part of the last name. > > That seems like a mistake. How often do people actually type middle names? > Probably less often than having 2-word last names at least according to my > totally unfounded guesstimates. If site (a) uses full-name and then site (b) > uses separate names, the default should be to use ALL the name from (a) on (b), > without forcing the user to navigate to chrome://settings/autofill, which > realistically is rarely if ever going to happen. Lots of web forms ask for a middle initial, so people type middle names frequently. If we think that people never go to chrome://settings/autofill, then how about we just delete all of this code and call it a day? </strawman> > > > Seems like either way, it won't work for someone. > > > > > > > For rAc, we have the opportunity to explicitly discourage websites from > > asking > > > > for tokenized names. For regular Autofill, we have to deal with the > common > > > > legacy case. So, I think we need to keep some way for users to correct > > > Chrome's > > > > tokenization of their name. If we want a single field by default, we > could > > do > > > > something complex like what https://contacts.google.com does. > > > > > > But data typed into a rAc dialog feeds into the legacy cases as well... > > > > True, and users can edit data originally entered into an rAc via the > > chrome://settings/autofill UI.
I agree with Ilya. We should keep the first, middle, last name in chrome://settings/autofill to help users fix incorrect tokenization of their names. Evan: would you agree?
https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/29 00:37:29, Ilya Sherman wrote: > On 2014/04/29 00:24:41, Evan Stade wrote: > > On 2014/04/29 00:08:29, Ilya Sherman wrote: > > > On 2014/04/28 23:56:37, Evan Stade wrote: > > > > On 2014/04/28 23:43:44, Ilya Sherman wrote: > > > > > On 2014/04/28 23:23:16, Evan Stade wrote: > > > > > > On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > > > > > > > On 2014/04/25 20:02:18, Evan Stade wrote: > > > > > > > > I think we should standardize the number of name fields (i.e. just > > > have > > > > > one > > > > > > > full > > > > > > > > name field instead of 3 name pieces here, to match the dialog) > > > > > > > > > > > > > > Done. > > > > > > > > > > > > +isherman, any opinion on this change? > > > > > > > > > > Websites will often request first, middle, and last names separately. > At > > > the > > > > > same time, Chrome has very basic heuristics in place to parse a full > name > > > into > > > > > components. If we offer just a single full name field, then users have > no > > > > > ability to correct Chrome's mistakes if they want to. That's not great > -- > > > it > > > > > means that Autofill will *never* work correctly for users like "Leonardo > > da > > > > > Vinci", or any other name that our simple heuristics fail on. > > > > > > > > > > > > > Isn't the inverse also true? If we have multiple input fields, then Yao > Ming > > > can > > > > add his name in chrome://settings/autofill, which will work for > > > > autocomplete="given-name" and "family-name", but when we try to fill it > into > > a > > > > full name field it will come out as Ming Yao, and he can't fix that. > > > > > > I'm not following this example. I think you mean that the full name field > > > always fills as "first name + middle name + last name", whereas for some > names > > > the family name should come first? If so, I think that's something that we > > > should fix as best we can as part of the localization work. > > > > yes, that is what I mean. > > > > > > > > > Also, wouldn't Leonardo da Vinci work with our heuristics? First name is > the > > > > first word (Leonardo), last name is everything else (da Vinci). But yea, > > Billy > > > > Bob Thornton wouldn't work. > > > > > > "da" would be treated as a middle name, rather than as part of the last > name. > > > > That seems like a mistake. How often do people actually type middle names? > > Probably less often than having 2-word last names at least according to my > > totally unfounded guesstimates. If site (a) uses full-name and then site (b) > > uses separate names, the default should be to use ALL the name from (a) on > (b), > > without forcing the user to navigate to chrome://settings/autofill, which > > realistically is rarely if ever going to happen. > > Lots of web forms ask for a middle initial, so people type middle names > frequently. Into full name fields? If the site has a middle initial field, sure, but we're talking about a single name field. Since you brought up the contacts.google.com example, yea, that's a pretty fancy control. But it also has good heuristics. Somehow it gets Alex de la Hidalga correct (Alex/de la Hidalga) and Billy Bob Thornton does something reasonable as well (Billy/Bob/Thornton). I think we should make this a full name field, and improve our heuristics to the contacts.google.com level of smarts. As a side note, it parses Leonardo da Vinci as Leonardo/da/Vinci, but I can't say I've ever actually met a da Vinci, so I don't know if this matters. > > If we think that people never go to chrome://settings/autofill, then how about > we just delete all of this code and call it a day? </strawman> I don't think that, but I do think we should optimize for the common case of "user never goes to chrome://settings/autofill". > > > > > Seems like either way, it won't work for someone. This is still the bottom line for me. Specifically, we think that websites /should/ just ask for full name. Thus if we just ask for full name, autofill works well with good websites and poorly with the bad sites that ask for tokenized names. On the other hand, we can ask for tokenized names, and then autofill works well with bad websites, but not as well with good websites. > > > > > > > > > For rAc, we have the opportunity to explicitly discourage websites from > > > asking > > > > > for tokenized names. For regular Autofill, we have to deal with the > > common > > > > > legacy case. So, I think we need to keep some way for users to correct > > > > Chrome's > > > > > tokenization of their name. If we want a single field by default, we > > could > > > do > > > > > something complex like what https://contacts.google.com does. > > > > > > > > But data typed into a rAc dialog feeds into the legacy cases as well... > > > > > > True, and users can edit data originally entered into an rAc via the > > > chrome://settings/autofill UI.
Ok, let's optimize for the case of users rarely visiting chrome://settings/autofill and keep the UI simple. I'm going with full names instead of first, middle, last.
Suggestion: How about we unblock Rouslan's work by keeping the status quo for the name fields for now, and discuss in a separate thread? https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/29 18:19:03, Evan Stade wrote: > On 2014/04/29 00:37:29, Ilya Sherman wrote: > > On 2014/04/29 00:24:41, Evan Stade wrote: > > > On 2014/04/29 00:08:29, Ilya Sherman wrote: > > > > On 2014/04/28 23:56:37, Evan Stade wrote: > > > > > On 2014/04/28 23:43:44, Ilya Sherman wrote: > > > > > > On 2014/04/28 23:23:16, Evan Stade wrote: > > > > > > > On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > > > > > > > > On 2014/04/25 20:02:18, Evan Stade wrote: > > > > > > > > > I think we should standardize the number of name fields (i.e. > just > > > > have > > > > > > one > > > > > > > > full > > > > > > > > > name field instead of 3 name pieces here, to match the dialog) > > > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > +isherman, any opinion on this change? > > > > > > > > > > > > Websites will often request first, middle, and last names separately. > > At > > > > the > > > > > > same time, Chrome has very basic heuristics in place to parse a full > > name > > > > into > > > > > > components. If we offer just a single full name field, then users > have > > no > > > > > > ability to correct Chrome's mistakes if they want to. That's not > great > > -- > > > > it > > > > > > means that Autofill will *never* work correctly for users like > "Leonardo > > > da > > > > > > Vinci", or any other name that our simple heuristics fail on. > > > > > > > > > > > > > > > > Isn't the inverse also true? If we have multiple input fields, then Yao > > Ming > > > > can > > > > > add his name in chrome://settings/autofill, which will work for > > > > > autocomplete="given-name" and "family-name", but when we try to fill it > > into > > > a > > > > > full name field it will come out as Ming Yao, and he can't fix that. > > > > > > > > I'm not following this example. I think you mean that the full name field > > > > always fills as "first name + middle name + last name", whereas for some > > names > > > > the family name should come first? If so, I think that's something that > we > > > > should fix as best we can as part of the localization work. > > > > > > yes, that is what I mean. > > > > > > > > > > > > Also, wouldn't Leonardo da Vinci work with our heuristics? First name is > > the > > > > > first word (Leonardo), last name is everything else (da Vinci). But yea, > > > Billy > > > > > Bob Thornton wouldn't work. > > > > > > > > "da" would be treated as a middle name, rather than as part of the last > > name. > > > > > > That seems like a mistake. How often do people actually type middle names? > > > Probably less often than having 2-word last names at least according to my > > > totally unfounded guesstimates. If site (a) uses full-name and then site (b) > > > uses separate names, the default should be to use ALL the name from (a) on > > (b), > > > without forcing the user to navigate to chrome://settings/autofill, which > > > realistically is rarely if ever going to happen. > > > > Lots of web forms ask for a middle initial, so people type middle names > > frequently. > > Into full name fields? If the site has a middle initial field, sure, but we're > talking about a single name field. > > Since you brought up the http://contacts.google.com example, yea, that's a pretty fancy > control. But it also has good heuristics. Somehow it gets Alex de la Hidalga > correct (Alex/de la Hidalga) and Billy Bob Thornton does something reasonable as > well (Billy/Bob/Thornton). I think we should make this a full name field, and > improve our heuristics to the http://contacts.google.com level of smarts. I definitely agree that we should improve our heuristics. > As a side note, it parses Leonardo da Vinci as Leonardo/da/Vinci, but I can't > say I've ever actually met a da Vinci, so I don't know if this matters. > > > > > If we think that people never go to chrome://settings/autofill, then how about > > we just delete all of this code and call it a day? </strawman> > > I don't think that, but I do think we should optimize for the common case of > "user never goes to chrome://settings/autofill". I don't understand this comment. This CL is strictly limited to changing the behavior of chrome://settings/autofill. Optimizing for the common case of users who never go to chrome://settings/autofill means this code can do literally anything, because in the common case users won't see it. I think that for the chrome://settings/autofill code, we should optimize for the use cases of users who will ever actually look at chrome://settings/autofill. For those users, I think more control is better. > > > > > Seems like either way, it won't work for someone. > > This is still the bottom line for me. > > Specifically, we think that websites /should/ just ask for full name. Thus if we > just ask for full name, autofill works well with good websites and poorly with > the bad sites that ask for tokenized names. On the other hand, we can ask for > tokenized names, and then autofill works well with bad websites, but not as well > with good websites. It's true that both tokenizing and recombining tokenized names are heuristic/imperfect operations. However, I think that combining tokens correctly is an order of magnitude easier than tokenizing correctly. Hence, I think we should let users correct the case that Chrome is far more likely to get wrong based on heuristics. Or, if we want to be fancy, we can do what https://contacts.google.com does, and allow users to explicitly specify both the tokenized and the non-tokenized format for their name. > > > > > > > > > > > > For rAc, we have the opportunity to explicitly discourage websites > from > > > > asking > > > > > > for tokenized names. For regular Autofill, we have to deal with the > > > common > > > > > > legacy case. So, I think we need to keep some way for users to > correct > > > > > Chrome's > > > > > > tokenization of their name. If we want a single field by default, we > > > could > > > > do > > > > > > something complex like what https://contacts.google.com does. > > > > > > > > > > But data typed into a rAc dialog feeds into the legacy cases as well... > > > > > > > > True, and users can edit data originally entered into an rAc via the > > > > chrome://settings/autofill UI. >
yea, we shouldn't block Rouslan on this. But I am not convinced separate name fields are preferable, so writing complicated code just to delete it next week seems like a bit of wasted effort. https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/29 18:59:20, Ilya Sherman wrote: > On 2014/04/29 18:19:03, Evan Stade wrote: > > On 2014/04/29 00:37:29, Ilya Sherman wrote: > > > On 2014/04/29 00:24:41, Evan Stade wrote: > > > > On 2014/04/29 00:08:29, Ilya Sherman wrote: > > > > > On 2014/04/28 23:56:37, Evan Stade wrote: > > > > > > On 2014/04/28 23:43:44, Ilya Sherman wrote: > > > > > > > On 2014/04/28 23:23:16, Evan Stade wrote: > > > > > > > > On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > > > > > > > > > On 2014/04/25 20:02:18, Evan Stade wrote: > > > > > > > > > > I think we should standardize the number of name fields (i.e. > > just > > > > > have > > > > > > > one > > > > > > > > > full > > > > > > > > > > name field instead of 3 name pieces here, to match the dialog) > > > > > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > +isherman, any opinion on this change? > > > > > > > > > > > > > > Websites will often request first, middle, and last names > separately. > > > At > > > > > the > > > > > > > same time, Chrome has very basic heuristics in place to parse a full > > > name > > > > > into > > > > > > > components. If we offer just a single full name field, then users > > have > > > no > > > > > > > ability to correct Chrome's mistakes if they want to. That's not > > great > > > -- > > > > > it > > > > > > > means that Autofill will *never* work correctly for users like > > "Leonardo > > > > da > > > > > > > Vinci", or any other name that our simple heuristics fail on. > > > > > > > > > > > > > > > > > > > Isn't the inverse also true? If we have multiple input fields, then > Yao > > > Ming > > > > > can > > > > > > add his name in chrome://settings/autofill, which will work for > > > > > > autocomplete="given-name" and "family-name", but when we try to fill > it > > > into > > > > a > > > > > > full name field it will come out as Ming Yao, and he can't fix that. > > > > > > > > > > I'm not following this example. I think you mean that the full name > field > > > > > always fills as "first name + middle name + last name", whereas for some > > > names > > > > > the family name should come first? If so, I think that's something that > > we > > > > > should fix as best we can as part of the localization work. > > > > > > > > yes, that is what I mean. > > > > > > > > > > > > > > > Also, wouldn't Leonardo da Vinci work with our heuristics? First name > is > > > the > > > > > > first word (Leonardo), last name is everything else (da Vinci). But > yea, > > > > Billy > > > > > > Bob Thornton wouldn't work. > > > > > > > > > > "da" would be treated as a middle name, rather than as part of the last > > > name. > > > > > > > > That seems like a mistake. How often do people actually type middle names? > > > > Probably less often than having 2-word last names at least according to my > > > > totally unfounded guesstimates. If site (a) uses full-name and then site > (b) > > > > uses separate names, the default should be to use ALL the name from (a) on > > > (b), > > > > without forcing the user to navigate to chrome://settings/autofill, which > > > > realistically is rarely if ever going to happen. > > > > > > Lots of web forms ask for a middle initial, so people type middle names > > > frequently. > > > > Into full name fields? If the site has a middle initial field, sure, but we're > > talking about a single name field. > > > > Since you brought up the http://contacts.google.com example, yea, that's a > pretty fancy > > control. But it also has good heuristics. Somehow it gets Alex de la Hidalga > > correct (Alex/de la Hidalga) and Billy Bob Thornton does something reasonable > as > > well (Billy/Bob/Thornton). I think we should make this a full name field, and > > improve our heuristics to the http://contacts.google.com level of smarts. > > I definitely agree that we should improve our heuristics. > > > As a side note, it parses Leonardo da Vinci as Leonardo/da/Vinci, but I can't > > say I've ever actually met a da Vinci, so I don't know if this matters. > > > > > > > > If we think that people never go to chrome://settings/autofill, then how > about > > > we just delete all of this code and call it a day? </strawman> > > > > I don't think that, but I do think we should optimize for the common case of > > "user never goes to chrome://settings/autofill". > > I don't understand this comment. This CL is strictly limited to changing the > behavior of chrome://settings/autofill. Optimizing for the common case of users > who never go to chrome://settings/autofill means this code can do literally > anything, because in the common case users won't see it. Yes, this is a tangent. But it also matters to this CL in that if we make this a full name field, the quality of our heuristics in parsing that input matters. > > I think that for the chrome://settings/autofill code, we should optimize for the > use cases of users who will ever actually look at chrome://settings/autofill. > For those users, I think more control is better. Lots of metrics are better to have more of. Control, consistency, simplicity, ... > > > > > > > Seems like either way, it won't work for someone. > > > > This is still the bottom line for me. > > > > Specifically, we think that websites /should/ just ask for full name. Thus if > we > > just ask for full name, autofill works well with good websites and poorly with > > the bad sites that ask for tokenized names. On the other hand, we can ask for > > tokenized names, and then autofill works well with bad websites, but not as > well > > with good websites. > > It's true that both tokenizing and recombining tokenized names are > heuristic/imperfect operations. However, I think that combining tokens > correctly is an order of magnitude easier than tokenizing correctly. Hence, I > think we should let users correct the case that Chrome is far more likely to get > wrong based on heuristics. If it's so much easier, why don't we do either one correctly? > > Or, if we want to be fancy, we can do what https://contacts.google.com does, and > allow users to explicitly specify both the tokenized and the non-tokenized > format for their name. > > > > > > > > > > > > > > > > For rAc, we have the opportunity to explicitly discourage websites > > from > > > > > asking > > > > > > > for tokenized names. For regular Autofill, we have to deal with the > > > > common > > > > > > > legacy case. So, I think we need to keep some way for users to > > correct > > > > > > Chrome's > > > > > > > tokenization of their name. If we want a single field by default, > we > > > > could > > > > > do > > > > > > > something complex like what https://contacts.google.com does. > > > > > > > > > > > > But data typed into a rAc dialog feeds into the legacy cases as > well... > > > > > > > > > > True, and users can edit data originally entered into an rAc via the > > > > > chrome://settings/autofill UI. > > >
https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/29 19:14:31, Evan Stade wrote: > On 2014/04/29 18:59:20, Ilya Sherman wrote: > > On 2014/04/29 18:19:03, Evan Stade wrote: > > > On 2014/04/29 00:37:29, Ilya Sherman wrote: > > > > On 2014/04/29 00:24:41, Evan Stade wrote: > > > > > On 2014/04/29 00:08:29, Ilya Sherman wrote: > > > > > > On 2014/04/28 23:56:37, Evan Stade wrote: > > > > > > > On 2014/04/28 23:43:44, Ilya Sherman wrote: > > > > > > > > On 2014/04/28 23:23:16, Evan Stade wrote: > > > > > > > > > On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > > > > > > > > > > On 2014/04/25 20:02:18, Evan Stade wrote: > > > > > > > > > > > I think we should standardize the number of name fields > (i.e. > > > just > > > > > > have > > > > > > > > one > > > > > > > > > > full > > > > > > > > > > > name field instead of 3 name pieces here, to match the > dialog) > > > > > > > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > +isherman, any opinion on this change? > > > > > > > > > > > > > > > > Websites will often request first, middle, and last names > > separately. > > > > At > > > > > > the > > > > > > > > same time, Chrome has very basic heuristics in place to parse a > full > > > > name > > > > > > into > > > > > > > > components. If we offer just a single full name field, then users > > > have > > > > no > > > > > > > > ability to correct Chrome's mistakes if they want to. That's not > > > great > > > > -- > > > > > > it > > > > > > > > means that Autofill will *never* work correctly for users like > > > "Leonardo > > > > > da > > > > > > > > Vinci", or any other name that our simple heuristics fail on. > > > > > > > > > > > > > > > > > > > > > > Isn't the inverse also true? If we have multiple input fields, then > > Yao > > > > Ming > > > > > > can > > > > > > > add his name in chrome://settings/autofill, which will work for > > > > > > > autocomplete="given-name" and "family-name", but when we try to fill > > it > > > > into > > > > > a > > > > > > > full name field it will come out as Ming Yao, and he can't fix that. > > > > > > > > > > > > I'm not following this example. I think you mean that the full name > > field > > > > > > always fills as "first name + middle name + last name", whereas for > some > > > > names > > > > > > the family name should come first? If so, I think that's something > that > > > we > > > > > > should fix as best we can as part of the localization work. > > > > > > > > > > yes, that is what I mean. > > > > > > > > > > > > > > > > > > Also, wouldn't Leonardo da Vinci work with our heuristics? First > name > > is > > > > the > > > > > > > first word (Leonardo), last name is everything else (da Vinci). But > > yea, > > > > > Billy > > > > > > > Bob Thornton wouldn't work. > > > > > > > > > > > > "da" would be treated as a middle name, rather than as part of the > last > > > > name. > > > > > > > > > > That seems like a mistake. How often do people actually type middle > names? > > > > > Probably less often than having 2-word last names at least according to > my > > > > > totally unfounded guesstimates. If site (a) uses full-name and then site > > (b) > > > > > uses separate names, the default should be to use ALL the name from (a) > on > > > > (b), > > > > > without forcing the user to navigate to chrome://settings/autofill, > which > > > > > realistically is rarely if ever going to happen. > > > > > > > > Lots of web forms ask for a middle initial, so people type middle names > > > > frequently. > > > > > > Into full name fields? If the site has a middle initial field, sure, but > we're > > > talking about a single name field. > > > > > > Since you brought up the http://contacts.google.com example, yea, that's a > > pretty fancy > > > control. But it also has good heuristics. Somehow it gets Alex de la Hidalga > > > correct (Alex/de la Hidalga) and Billy Bob Thornton does something > reasonable > > as > > > well (Billy/Bob/Thornton). I think we should make this a full name field, > and > > > improve our heuristics to the http://contacts.google.com level of smarts. > > > > I definitely agree that we should improve our heuristics. > > > > > As a side note, it parses Leonardo da Vinci as Leonardo/da/Vinci, but I > can't > > > say I've ever actually met a da Vinci, so I don't know if this matters. > > > > > > > > > > > If we think that people never go to chrome://settings/autofill, then how > > about > > > > we just delete all of this code and call it a day? </strawman> > > > > > > I don't think that, but I do think we should optimize for the common case of > > > "user never goes to chrome://settings/autofill". > > > > I don't understand this comment. This CL is strictly limited to changing the > > behavior of chrome://settings/autofill. Optimizing for the common case of > users > > who never go to chrome://settings/autofill means this code can do literally > > anything, because in the common case users won't see it. > > Yes, this is a tangent. But it also matters to this CL in that if we make this a > full name field, the quality of our heuristics in parsing that input matters. > > > > > I think that for the chrome://settings/autofill code, we should optimize for > the > > use cases of users who will ever actually look at chrome://settings/autofill. > > For those users, I think more control is better. > > Lots of metrics are better to have more of. Control, consistency, simplicity, > ... > > > > > > > > > > Seems like either way, it won't work for someone. > > > > > > This is still the bottom line for me. > > > > > > Specifically, we think that websites /should/ just ask for full name. Thus > if > > we > > > just ask for full name, autofill works well with good websites and poorly > with > > > the bad sites that ask for tokenized names. On the other hand, we can ask > for > > > tokenized names, and then autofill works well with bad websites, but not as > > well > > > with good websites. > > > > It's true that both tokenizing and recombining tokenized names are > > heuristic/imperfect operations. However, I think that combining tokens > > correctly is an order of magnitude easier than tokenizing correctly. Hence, I > > think we should let users correct the case that Chrome is far more likely to > get > > wrong based on heuristics. > > If it's so much easier, why don't we do either one correctly? For names in English, we don't tokenize correctly, but we do recombine names correctly (at least, AFAIK). We don't support international names as well because this part of the code was never internationalized. > > Or, if we want to be fancy, we can do what https://contacts.google.com does, > and > > allow users to explicitly specify both the tokenized and the non-tokenized > > format for their name. > > > > > > > > > > > > > > > > > > > > For rAc, we have the opportunity to explicitly discourage websites > > > from > > > > > > asking > > > > > > > > for tokenized names. For regular Autofill, we have to deal with > the > > > > > common > > > > > > > > legacy case. So, I think we need to keep some way for users to > > > correct > > > > > > > Chrome's > > > > > > > > tokenization of their name. If we want a single field by default, > > we > > > > > could > > > > > > do > > > > > > > > something complex like what https://contacts.google.com does. > > > > > > > > > > > > > > But data typed into a rAc dialog feeds into the legacy cases as > > well... > > > > > > > > > > > > True, and users can edit data originally entered into an rAc via the > > > > > > chrome://settings/autofill UI. > > > > > >
Coincidentally, somebody bumped [ http://crbug.com/132332 ] today. From that bug, it sounds like our heuristics quickly regress even those users who bother to go into Settings and fix things up. Given that, I don't care as much about maintaining the status quo of the settings page -- it sounds like we don't currently give users meaningful control anyway. So, Rouslan, I think it makes sense for you to implement whichever approach is simpler for now. However, let's definitely at least file a bug to come back to the question of how best to support users for whom our heuristics fail -- including possibly just making our heuristics way better.
Evan: PTAL Patch Set 5. Ilya: I filed http://crbug.com/368536. https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:8: /** @const */ var OverlaySelector = '#autofill-edit-address-overlay '; On 2014/04/28 23:23:16, Evan Stade wrote: > not useful. Use this.querySelector in most places. Done. https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:246: for (var i = 0; i < fields.length; i++) On 2014/04/28 23:23:16, Evan Stade wrote: > loops should always have curlies Done. https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:354: var customFieldElements = {'fullName': 'div'}; On 2014/04/28 23:23:16, Evan Stade wrote: > what's wrong with making this a label? This how chrome://settings/editoAutofillAddress works now. Here's my understanding why that is. When the user clicks on a <label> for a <list>, then one of two things will happen: Option #1: If the <list> has only a placeholder ("Add name"), then this placeholder will be focused. Option #2: If the <list> has some data and a placeholder, then nothing will be focused.. Such inconsistency would be strange to a user. Therefore, the <list> is inside of a <div> instead of <label>. https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:370: var label = document.createElement('div'); On 2014/04/28 23:23:16, Evan Stade wrote: > it's confusing that field is a label while label is a div Renamed field to fieldContainer. Renamed label to fieldName. https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:381: if (input.tagName == 'LIST') { On 2014/04/28 23:23:16, Evan Stade wrote: > This seems vaguely better: > > if (input instanceOf HTMLListElement) HTMLListElement does not exist. (HTMLUListELement matches only <ul> elements.) https://codereview.chromium.org/243013004/diff/420001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/243013004/diff/420001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:59: static const char kLanguageCode[] = "languageCOde"; On 2014/04/28 23:23:16, Evan Stade wrote: > s/COde/Code Done. https://codereview.chromium.org/243013004/diff/420001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:607: if (args->GetList(10, &list_value)) On 2014/04/28 23:23:16, Evan Stade wrote: > because of these types of changes, I prefer arg_counter++ instead of numerals Done. https://codereview.chromium.org/243013004/diff/460001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_browsertest.js (right): https://codereview.chromium.org/243013004/diff/460001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_browsertest.js:149: assertEquals('INPUT', field.tagName); assertEquals(field instanceof HTMLInputElement) is false for some reason, even though console.log(field instanceof HTMLInputElement); prints "true". Therefore, I'm using field.tagName here.
https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.html (right): https://codereview.chromium.org/243013004/diff/320009/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.html:45: <span i18n-content="autofillFirstNameLabel"></span> On 2014/04/29 19:25:03, Ilya Sherman wrote: > On 2014/04/29 19:14:31, Evan Stade wrote: > > On 2014/04/29 18:59:20, Ilya Sherman wrote: > > > On 2014/04/29 18:19:03, Evan Stade wrote: > > > > On 2014/04/29 00:37:29, Ilya Sherman wrote: > > > > > On 2014/04/29 00:24:41, Evan Stade wrote: > > > > > > On 2014/04/29 00:08:29, Ilya Sherman wrote: > > > > > > > On 2014/04/28 23:56:37, Evan Stade wrote: > > > > > > > > On 2014/04/28 23:43:44, Ilya Sherman wrote: > > > > > > > > > On 2014/04/28 23:23:16, Evan Stade wrote: > > > > > > > > > > On 2014/04/28 15:49:44, Rouslan Solomakhin wrote: > > > > > > > > > > > On 2014/04/25 20:02:18, Evan Stade wrote: > > > > > > > > > > > > I think we should standardize the number of name fields > > (i.e. > > > > just > > > > > > > have > > > > > > > > > one > > > > > > > > > > > full > > > > > > > > > > > > name field instead of 3 name pieces here, to match the > > dialog) > > > > > > > > > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > +isherman, any opinion on this change? > > > > > > > > > > > > > > > > > > Websites will often request first, middle, and last names > > > separately. > > > > > At > > > > > > > the > > > > > > > > > same time, Chrome has very basic heuristics in place to parse a > > full > > > > > name > > > > > > > into > > > > > > > > > components. If we offer just a single full name field, then > users > > > > have > > > > > no > > > > > > > > > ability to correct Chrome's mistakes if they want to. That's > not > > > > great > > > > > -- > > > > > > > it > > > > > > > > > means that Autofill will *never* work correctly for users like > > > > "Leonardo > > > > > > da > > > > > > > > > Vinci", or any other name that our simple heuristics fail on. > > > > > > > > > > > > > > > > > > > > > > > > > Isn't the inverse also true? If we have multiple input fields, > then > > > Yao > > > > > Ming > > > > > > > can > > > > > > > > add his name in chrome://settings/autofill, which will work for > > > > > > > > autocomplete="given-name" and "family-name", but when we try to > fill > > > it > > > > > into > > > > > > a > > > > > > > > full name field it will come out as Ming Yao, and he can't fix > that. > > > > > > > > > > > > > > I'm not following this example. I think you mean that the full name > > > field > > > > > > > always fills as "first name + middle name + last name", whereas for > > some > > > > > names > > > > > > > the family name should come first? If so, I think that's something > > that > > > > we > > > > > > > should fix as best we can as part of the localization work. > > > > > > > > > > > > yes, that is what I mean. > > > > > > > > > > > > > > > > > > > > > Also, wouldn't Leonardo da Vinci work with our heuristics? First > > name > > > is > > > > > the > > > > > > > > first word (Leonardo), last name is everything else (da Vinci). > But > > > yea, > > > > > > Billy > > > > > > > > Bob Thornton wouldn't work. > > > > > > > > > > > > > > "da" would be treated as a middle name, rather than as part of the > > last > > > > > name. > > > > > > > > > > > > That seems like a mistake. How often do people actually type middle > > names? > > > > > > Probably less often than having 2-word last names at least according > to > > my > > > > > > totally unfounded guesstimates. If site (a) uses full-name and then > site > > > (b) > > > > > > uses separate names, the default should be to use ALL the name from > (a) > > on > > > > > (b), > > > > > > without forcing the user to navigate to chrome://settings/autofill, > > which > > > > > > realistically is rarely if ever going to happen. > > > > > > > > > > Lots of web forms ask for a middle initial, so people type middle names > > > > > frequently. > > > > > > > > Into full name fields? If the site has a middle initial field, sure, but > > we're > > > > talking about a single name field. > > > > > > > > Since you brought up the http://contacts.google.com example, yea, that's a > > > pretty fancy > > > > control. But it also has good heuristics. Somehow it gets Alex de la > Hidalga > > > > correct (Alex/de la Hidalga) and Billy Bob Thornton does something > > reasonable > > > as > > > > well (Billy/Bob/Thornton). I think we should make this a full name field, > > and > > > > improve our heuristics to the http://contacts.google.com level of smarts. > > > > > > I definitely agree that we should improve our heuristics. > > > > > > > As a side note, it parses Leonardo da Vinci as Leonardo/da/Vinci, but I > > can't > > > > say I've ever actually met a da Vinci, so I don't know if this matters. > > > > > > > > > > > > > > If we think that people never go to chrome://settings/autofill, then how > > > about > > > > > we just delete all of this code and call it a day? </strawman> > > > > > > > > I don't think that, but I do think we should optimize for the common case > of > > > > "user never goes to chrome://settings/autofill". > > > > > > I don't understand this comment. This CL is strictly limited to changing > the > > > behavior of chrome://settings/autofill. Optimizing for the common case of > > users > > > who never go to chrome://settings/autofill means this code can do literally > > > anything, because in the common case users won't see it. > > > > Yes, this is a tangent. But it also matters to this CL in that if we make this > a > > full name field, the quality of our heuristics in parsing that input matters. > > > > > > > > I think that for the chrome://settings/autofill code, we should optimize for > > the > > > use cases of users who will ever actually look at > chrome://settings/autofill. > > > For those users, I think more control is better. > > > > Lots of metrics are better to have more of. Control, consistency, simplicity, > > ... > > > > > > > > > > > > > Seems like either way, it won't work for someone. > > > > > > > > This is still the bottom line for me. > > > > > > > > Specifically, we think that websites /should/ just ask for full name. Thus > > if > > > we > > > > just ask for full name, autofill works well with good websites and poorly > > with > > > > the bad sites that ask for tokenized names. On the other hand, we can ask > > for > > > > tokenized names, and then autofill works well with bad websites, but not > as > > > well > > > > with good websites. > > > > > > It's true that both tokenizing and recombining tokenized names are > > > heuristic/imperfect operations. However, I think that combining tokens > > > correctly is an order of magnitude easier than tokenizing correctly. Hence, > I > > > think we should let users correct the case that Chrome is far more likely to > > get > > > wrong based on heuristics. > > > > If it's so much easier, why don't we do either one correctly? > > For names in English, we don't tokenize correctly, but we do recombine names > correctly (at least, AFAIK). We don't support international names as well > because this part of the code was never internationalized. Yao Ming goes by surname-givenname in English. I'm not sure how prevalent this is. > > > > Or, if we want to be fancy, we can do what https://contacts.google.com does, > > and > > > allow users to explicitly specify both the tokenized and the non-tokenized > > > format for their name. > > > > > > > > > > > > > > > > > > > > > > > > For rAc, we have the opportunity to explicitly discourage > websites > > > > from > > > > > > > asking > > > > > > > > > for tokenized names. For regular Autofill, we have to deal with > > the > > > > > > common > > > > > > > > > legacy case. So, I think we need to keep some way for users to > > > > correct > > > > > > > > Chrome's > > > > > > > > > tokenization of their name. If we want a single field by > default, > > > we > > > > > > could > > > > > > > do > > > > > > > > > something complex like what https://contacts.google.com does. > > > > > > > > > > > > > > > > But data typed into a rAc dialog feeds into the legacy cases as > > > well... > > > > > > > > > > > > > > True, and users can edit data originally entered into an rAc via the > > > > > > > chrome://settings/autofill UI. > > > > > > > > > > https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:381: if (input.tagName == 'LIST') { On 2014/04/30 02:06:15, Rouslan Solomakhin wrote: > On 2014/04/28 23:23:16, Evan Stade wrote: > > This seems vaguely better: > > > > if (input instanceOf HTMLListElement) > > HTMLListElement does not exist. (HTMLUListELement matches only <ul> elements.) input instanceof cr.ui.List https://codereview.chromium.org/243013004/diff/460001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_browsertest.js (right): https://codereview.chromium.org/243013004/diff/460001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_browsertest.js:149: assertEquals('INPUT', field.tagName); On 2014/04/30 02:06:15, Rouslan Solomakhin wrote: > assertEquals(field instanceof HTMLInputElement) is false for some reason, probably because it requires two arguments and you only passed one so the second is undefined > even > though console.log(field instanceof HTMLInputElement); prints "true". Therefore, > I'm using field.tagName here.
Evan: PTAL Patch Set 6. https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/420001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:381: if (input.tagName == 'LIST') { On 2014/04/30 05:25:22, Evan Stade wrote: > On 2014/04/30 02:06:15, Rouslan Solomakhin wrote: > > On 2014/04/28 23:23:16, Evan Stade wrote: > > > This seems vaguely better: > > > > > > if (input instanceOf HTMLListElement) > > > > HTMLListElement does not exist. (HTMLUListELement matches only <ul> elements.) > > input instanceof cr.ui.List "input instanceof cr.ui.List" works only after the decorate() call below. https://codereview.chromium.org/243013004/diff/460001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_browsertest.js (right): https://codereview.chromium.org/243013004/diff/460001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_browsertest.js:149: assertEquals('INPUT', field.tagName); On 2014/04/30 05:25:22, Evan Stade wrote: > On 2014/04/30 02:06:15, Rouslan Solomakhin wrote: > > assertEquals(field instanceof HTMLInputElement) is false for some reason, > > probably because it requires two arguments and you only passed one so the second > is undefined doh
Evan: ping.
lgtm https://codereview.chromium.org/243013004/diff/520001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/520001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:385: // "input instanceof cr.ui.List" works only after the decorate() call. you don't have to put this comment, I was just being stupid https://codereview.chromium.org/243013004/diff/520001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_overlay.css (right): https://codereview.chromium.org/243013004/diff/520001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_overlay.css:20: #autofill-edit-address-overlay .short div[role='listitem'] > div > div, I don't like this CSS but the easiest thing to do is probably commit now, and let me twiddle with the appearance later.
Sending to cq. https://codereview.chromium.org/243013004/diff/520001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/520001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:385: // "input instanceof cr.ui.List" works only after the decorate() call. On 2014/05/02 19:21:50, Evan Stade wrote: > you don't have to put this comment, I was just being stupid Removed. https://codereview.chromium.org/243013004/diff/520001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_overlay.css (right): https://codereview.chromium.org/243013004/diff/520001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_overlay.css:20: #autofill-edit-address-overlay .short div[role='listitem'] > div > div, On 2014/05/02 19:21:50, Evan Stade wrote: > I don't like this CSS but the easiest thing to do is probably commit now, and > let me twiddle with the appearance later. sounds good.
The CQ bit was checked by rouslan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/243013004/540001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium
The CQ bit was checked by rouslan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/243013004/540001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium
The CQ bit was checked by rouslan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/243013004/560001
Message was sent while issue was closed.
Change committed as 268095
Message was sent while issue was closed.
for a rainy day? https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... File chrome/browser/resources/options/autofill_edit_address_overlay.js (right): https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:13: var languageCode; ^ i'm confused, where is this used? looks like dead code... (protip: |this.languageCode| is not the same thing) https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:111: * @param {element} list The list to update. !Element https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:145: * @return {element} The country selector. Element https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:148: getCountrySelector_: function() { nit: getCountrySwitcher as calling something a "selector" in a file that also has "querySelector" is confusing https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:163: * @return {NodeList} The text input elements. !NodeList in both cases (here and above) https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:173: * @return {object} The mapping from field names to values. !Object https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:178: address['country'] = this.getCountrySelector_().value; why not: var address = {country: this.getCountrySelector_().value}; ? https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:183: lists[i].dataModel.slice(0, lists[i].dataModel.length - 1); you can usually do .slice(0) if you want the whole thing... https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:196: * @param {object} address The object with values to use. !Object https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:200: this.getCountrySelector_().value = address['country'] || ''; it's preferable to use .prop rather than ['prop'] in webui code https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:235: address[argCounter++] = this.languageCode; confused, why not: var address = [ this.guid, inputFields.fullName || [], ... ]; https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:250: fields[i].oninput = function(event) { self.inputFieldChanged_(); }; fields[i].oninput = this.inputFieldChanged_.bind(this); https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:259: var disabled = true; var disabled = !this.getCountrySelector_().value; https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:340: var address = this.getInputFields_(); why is this named address? https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:350: * |components|. @param {<some_type>} components https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:357: } content.innerHTML = ''; https://codereview.chromium.org/243013004/diff/560001/chrome/browser/resource... chrome/browser/resources/options/autofill_edit_address_overlay.js:360: var customInputElements = {'fullName': 'list', 'addrLines': 'textarea'}; opt nit: don't need ' around keys in this case https://codereview.chromium.org/243013004/diff/560001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_browsertest.js (right): https://codereview.chromium.org/243013004/diff/560001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_browsertest.js:18: * @return {int} The size of the list. {number} https://codereview.chromium.org/243013004/diff/560001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_browsertest.js:96: testDone(); why are you using a async test for these cases? https://codereview.chromium.org/243013004/diff/560001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/243013004/diff/560001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:76: static const char kDefaultCountryCode[] = "US"; why not just inline? https://codereview.chromium.org/243013004/diff/560001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:166: std::string defaultCountryLanguageCode; cpp_vars_like_this |
