DOMUI Prefs: Replace search engine edit overlay with inline editing.

DOMUI Prefs: Replace search engine edit overlay with inline editing.

Validation feedback still needs polish, but this is completely functional, so I'll leave that for a follow-up.

This introduces a new shared class for editable lists; we should migrate the existing editable lists to it, expanding as necessary, but I didn't want to make this patch too unwieldly.

BUG=63825, 61742
TEST=Search engines should be editable and addable inline in DOMUI prefs.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71121

[Evan Stade, 2011-01-11 21:06:01]

lgtm

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:78: 
remove extra line

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:96:
this.currentInputIsValid())
curlies for multi-line conditional

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:235: 
extra line here and at eof

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_engine_manager.css (right):

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager.css:95: #searchEngineList
> [editing] [editmode=false] {
nice

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_engine_manager_engine_list.js
(right):

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:200:
return (!this.waitingForValidation_ && this.currentlyValid_)
no (), need trailing ;

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:206:
return (this.nameField_.value != engine['name'] ||
ditto

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:213: var
editIndex = this.searchEngine_['modelIndex']
;

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:279:
this.nameField_.classList.add('invalid');
nit: can you put a blank line after each else clause

[arv (Not doing code reviews), 2011-01-11 21:52:44]

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:54:
this.addEventListener('selectedChange', function(event) {
You should be able to override the selected setter instead of listening to an
event.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:82:
this.onEditStarted();
cr.dispatchEvent

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:122:
initialFocusElement: function() {
Use getter?

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:132:
currentInputIsValid: function() {
use getter?

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/list_single_selection_model.js
(left):

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list_single_selection_model.js:143:
indexes.sort();
Why was this removed?

[stuartmorgan, 2011-01-11 23:18:41]

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:54:
this.addEventListener('selectedChange', function(event) {
On 2011/01/11 21:52:45, arv wrote:
> You should be able to override the selected setter instead of listening to an
> event.

Actually, I discovered that there's already a selectionChanged() method in
ListItem designed to be overridden by subclasses, so I changed to use that.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:78: 
On 2011/01/11 21:06:01, Evan Stade wrote:
> remove extra line

Done.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:82:
this.onEditStarted();
On 2011/01/11 21:52:45, arv wrote:
> cr.dispatchEvent

Done, for all three onEdit* calls.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:96:
this.currentInputIsValid())
On 2011/01/11 21:06:01, Evan Stade wrote:
> curlies for multi-line conditional

Done.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:122:
initialFocusElement: function() {
On 2011/01/11 21:52:45, arv wrote:
> Use getter?

Done.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:132:
currentInputIsValid: function() {
On 2011/01/11 21:52:45, arv wrote:
> use getter?

Done.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:235: 
On 2011/01/11 21:06:01, Evan Stade wrote:
> extra line here and at eof

Done.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_engine_manager.css (right):

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager.css:95: #searchEngineList
> [editing] [editmode=false] {
On 2011/01/11 21:06:01, Evan Stade wrote:
> nice

:)

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_engine_manager_engine_list.js
(right):

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:200:
return (!this.waitingForValidation_ && this.currentlyValid_)
On 2011/01/11 21:06:01, Evan Stade wrote:
> no (), need trailing ;

Done.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:206:
return (this.nameField_.value != engine['name'] ||
On 2011/01/11 21:06:01, Evan Stade wrote:
> ditto

Done.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:213: var
editIndex = this.searchEngine_['modelIndex']
On 2011/01/11 21:06:01, Evan Stade wrote:
> ;

Done.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:279:
this.nameField_.classList.add('invalid');
On 2011/01/11 21:06:01, Evan Stade wrote:
> nit: can you put a blank line after each else clause

Done.

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/list_single_selection_model.js
(left):

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list_single_selection_model.js:143:
indexes.sort();
On 2011/01/11 21:52:45, arv wrote:
> Why was this removed?

If the indexes are sorted, then the new row can be told that it's selected
before the old row is told that it's no longer selected, which goes against the
idea of having a selection model that enforces only one selection at a time.

It's unclear to me why it was there in the first place.

[arv (Not doing code reviews), 2011-01-11 23:29:28]

LGTM

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/list_single_selection_model.js
(left):

http://codereview.chromium.org/6151004/diff/2001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list_single_selection_model.js:143:
indexes.sort();
On 2011/01/11 23:18:41, stuartmorgan wrote:
> On 2011/01/11 21:52:45, arv wrote:
> > Why was this removed?
> 
> If the indexes are sorted, then the new row can be told that it's selected
> before the old row is told that it's no longer selected, which goes against
the
> idea of having a selection model that enforces only one selection at a time.
> 
> It's unclear to me why it was there in the first place.

Probably just a remnant from the multiple selection model.

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
File chrome/browser/resources/options/inline_editable_list.js (right):

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/inline_editable_list.js:6: const DeletableItem
= options.DeletableItem;
It would be good if we could move these to cr.ui in a follow up CL.

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.js (right):

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:40: // This is a
temporary hack to allow the "Make Default" button to
Why are we removing the selection again?

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:63:
placeholder['modelIndex'] = '-1';
Should this be a number instead of a string?

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:63:
placeholder['modelIndex'] = '-1';
var placeHolder = {
  'modelIndex': -1
};

or just

model.push({
  'modelIndex': -1
});

[stuartmorgan, 2011-01-12 00:26:51]

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
File chrome/browser/resources/options/inline_editable_list.js (right):

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/inline_editable_list.js:6: const DeletableItem
= options.DeletableItem;
On 2011/01/11 23:29:29, arv wrote:
> It would be good if we could move these to cr.ui in a follow up CL.

Sure; added to my TODO list.

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.js (right):

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:40: // This is a
temporary hack to allow the "Make Default" button to
On 2011/01/11 23:29:29, arv wrote:
> Why are we removing the selection again?

In the DOMUI pref list model, selection == edit. The options for inline-editable
lists are:
1) Leave the row in edit mode when the list is no longer focused; this feels
really weird
2) Make edit require both selection and list focus.
3) Remove selection when unfocusing a list.

Evan implemented 3 in exceptions, and I was being consistent with that. Once we
everything is implemented with the new base class it will be easy enough to
switch to 2 if we decide that's better.

(As we've added more of the new mock behaviors to the lists more interaction
edge cases are showing up that weren't obvious for the mocks, so my plan is to
move forward getting all the capabilities implemented and then we can test
everything together and discuss with the UX folks as necessary.)

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:63:
placeholder['modelIndex'] = '-1';
On 2011/01/11 23:29:29, arv wrote:
> Should this be a number instead of a string?

modelIndex only really used as a chrome.send argument, so leaving it as a string
simplifies everything.

http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:63:
placeholder['modelIndex'] = '-1';
On 2011/01/11 23:29:29, arv wrote:
> var placeHolder = {
>   'modelIndex': -1
> };
> 
> or just
> 
> model.push({
>   'modelIndex': -1
> });

Done.

[arv (Not doing code reviews), 2011-01-12 00:38:57]

LGTM

erik



On Tue, Jan 11, 2011 at 16:26,  <stuartmorgan@chromium.org> wrote:
>
>
http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
> File chrome/browser/resources/options/inline_editable_list.js (right):
>
>
http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
> chrome/browser/resources/options/inline_editable_list.js:6: const
> DeletableItem = options.DeletableItem;
> On 2011/01/11 23:29:29, arv wrote:
>>
>> It would be good if we could move these to cr.ui in a follow up CL.
>
> Sure; added to my TODO list.
>
>
http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
> File chrome/browser/resources/options/search_engine_manager.js (right):
>
>
http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
> chrome/browser/resources/options/search_engine_manager.js:40: // This is
> a temporary hack to allow the "Make Default" button to
> On 2011/01/11 23:29:29, arv wrote:
>>
>> Why are we removing the selection again?
>
> In the DOMUI pref list model, selection == edit. The options for
> inline-editable lists are:
> 1) Leave the row in edit mode when the list is no longer focused; this
> feels really weird
> 2) Make edit require both selection and list focus.
> 3) Remove selection when unfocusing a list.
>
> Evan implemented 3 in exceptions, and I was being consistent with that.
> Once we everything is implemented with the new base class it will be
> easy enough to switch to 2 if we decide that's better.
>
> (As we've added more of the new mock behaviors to the lists more
> interaction edge cases are showing up that weren't obvious for the
> mocks, so my plan is to move forward getting all the capabilities
> implemented and then we can test everything together and discuss with
> the UX folks as necessary.)
>
>
http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
> chrome/browser/resources/options/search_engine_manager.js:63:
> placeholder['modelIndex'] = '-1';
> On 2011/01/11 23:29:29, arv wrote:
>>
>> Should this be a number instead of a string?
>
> modelIndex only really used as a chrome.send argument, so leaving it as
> a string simplifies everything.
>
>
http://codereview.chromium.org/6151004/diff/11001/chrome/browser/resources/op...
> chrome/browser/resources/options/search_engine_manager.js:63:
> placeholder['modelIndex'] = '-1';
> On 2011/01/11 23:29:29, arv wrote:
>>
>> var placeHolder = {
>>   'modelIndex': -1
>> };
>
>> or just
>
>> model.push({
>>   'modelIndex': -1
>> });
>
> Done.
>
> http://codereview.chromium.org/6151004/
>