DeletableItemList: Make 'X' button delete only the one list item that contains it.

DeletableItemList: Make 'X' button delete only the one list item that contains it, regardless of selection.

Because the close buttons do not give any visual indication that they would behave differently when multiple items are selected, we should delete only the one list item that contains the close button being clicked, regardless of which / how many other items are selected. If the user wants to delete a lot of stuff at once, he would have to press the delete key.

BUG=399209
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287005

[engedy, 2014-07-31 09:30:22]

@Dan, @Evan, I have implemented this separately, PTAL.

https://codereview.chromium.org/435553003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

https://codereview.chromium.org/435553003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:147:
this.selectionModel.clear();
What is the motivation for clearing the selection? (Even though it happens in
most all cases automatically even without this line, as we rebuild the list of
items.)

[Evan Stade, 2014-07-31 15:09:27]

lgtm

https://codereview.chromium.org/435553003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

https://codereview.chromium.org/435553003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:143: // delete a lot of
stuff at once, he would have to press the delete key.
I think this comment is too much. Better to make this part of the CL
description.

https://codereview.chromium.org/435553003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:147:
this.selectionModel.clear();
On 2014/07/31 09:30:22, engedy wrote:
> What is the motivation for clearing the selection? (Even though it happens in
> most all cases automatically even without this line, as we rebuild the list of
> items.)

It's my impression that that is the expected behavior --- any time I left click
without holding ctrl or shift, the selection will be cleared.

also based on your comment, it sounds like consistency is another good reason

[engedy, 2014-07-31 16:36:53]

@Evan, please take another look.

https://codereview.chromium.org/435553003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

https://codereview.chromium.org/435553003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:143: // delete a lot of
stuff at once, he would have to press the delete key.
On 2014/07/31 15:09:26, Evan Stade wrote:
> I think this comment is too much. Better to make this part of the CL
> description.

Moved it to the CL description, and replaced it with a much shorter one. WDYT? I
am happy to remove it entire if you still do not like it.

https://codereview.chromium.org/435553003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:147:
this.selectionModel.clear();
On 2014/07/31 15:09:27, Evan Stade wrote:
> On 2014/07/31 09:30:22, engedy wrote:
> > What is the motivation for clearing the selection? (Even though it happens
in
> > most all cases automatically even without this line, as we rebuild the list
of
> > items.)
> 
> It's my impression that that is the expected behavior --- any time I left
click
> without holding ctrl or shift, the selection will be cleared.
> 
> also based on your comment, it sounds like consistency is another good reason

Sorry, turns out my above statement was incorrect:
 1.) We currently have some lists where the selection does not go away when an
item gets deleted (e.g. Content Settings exceptions).
 2.) I have just discovered lines 102--113 in this file, which implements the
complete opposite of what we have just discussed. :-)

Actually, I now think that *not* clearing the selection here would actually make
sense. This way, the 'X' buttons would consistently have absolutely nothing to
do with the current selection: the selection would not affect which item is
deleted, and the selection would be not affected by deletions. I think the
current visual treatment does not contradict this too much.

Then, if a subclass implements deletion in such a way that the underlying model
/ selection model is reset, that would still be consistently done so. (And might
even be considered an erroneous implementation.)

WDYT?

[Evan Stade, 2014-07-31 19:36:07]

I don't care strongly about whether the selection is clobbered when you press
[x].

https://codereview.chromium.org/435553003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/deletable_item_list.js (right):

https://codereview.chromium.org/435553003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/deletable_item_list.js:140: // would be
affected by the current selection, so it should not be.
I just don't think that code comments are the best place to justify or explain
behavioral choices (except in the case where the code is super complicated for
an unapparent reason).

[engedy, 2014-08-01 08:06:25]

https://codereview.chromium.org/435553003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/deletable_item_list.js (right):

https://codereview.chromium.org/435553003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/deletable_item_list.js:140: // would be
affected by the current selection, so it should not be.
On 2014/07/31 19:36:07, Evan Stade wrote:
> I just don't think that code comments are the best place to justify or explain
> behavioral choices (except in the case where the code is super complicated for
> an unapparent reason).

Done, I have removed the comment.

[engedy, 2014-08-01 08:06:29]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2014-08-01 08:08:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/engedy@chromium.org/435553003/40001

[commit-bot: I haz the power, 2014-08-01 15:08:55]

Change committed as 287005