Take care not to destroy a multi-selection when a child element of a list item receives focus.

Take care not to destroy a multi-selection when a child element of a list item receives focus.

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

[engedy, 2014-07-30 13:45:51]

@Dan, PTAL.

[arv (Not doing code reviews), 2014-07-30 18:49:24]

LGTM

I wish we had some tests for this...

[Dan Beam, 2014-07-30 19:05:34]

not lgtm

i don't like this CL because batch actions aren't differentiated visually from
single actions.

for instance, I patched this in locally and selected multiple search engines. 
when i click the X on an individual search engine with multiple items selected,
it deletes ALL rather than item that X belongs to.

visually: http://i.imgur.com/5YoLfKa.png (what happens when I click the top X?)

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

https://codereview.chromium.org/429073006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:94: handleFocus_:
function() {
if (this.selected)
  return;

[engedy, 2014-07-30 19:38:01]

I agree that this is somewhat confusing. I cannot really think of any good
visual indication. What would you prefer?

[Evan Stade, 2014-07-30 21:45:05]

On 2014/07/30 19:38:01, engedy wrote:
> I agree that this is somewhat confusing. I cannot really think of any good
> visual indication. What would you prefer?

I agree with Dan. I think the desired behavior would be that clicking an 'X'
when multiple lines are selected only deletes the line the 'X' is on, and also
removes the selection. If you want to delete a lot of stuff at once, you'd have
to press the delete key.

If this is not what other people feel is correct, then we should probably
consult an interaction designer.

[Dan Beam, 2014-07-30 21:56:12]

On 2014/07/30 19:38:01, engedy wrote:
> I agree that this is somewhat confusing. I cannot really think of any good
> visual indication. What would you prefer?

I'd prefer something that's intuitive or nothing at all.

chrome://history and chrome://bookmarks are examples I know of that use bulk
manipulation in Chrome WebUI. This is because the use case of bulk removing
history or bulk moving bookmarks is more popular.

I don't see concepts we can borrow from those pages that are immediately useful
to all webui deletable lists:

- a "Remove all selected" button that shows up when you've multi-selected seems
odd
 - we'd probably have to keep this around always but disable it until a
multi-selection is made (bleh)

- a "Remove all" right click context menu item means we'd have to replace the
native context menu (i think?)
 - probably not easy to discover

- pressing delete with multiple items selected
 - probably not particularly discoverable either
 - slightly less confusing but still potentially confused with "delete the last
selected item"

We should probably also hide or disable single-item-specific buttons while
multi-selected unless it's a reverseable or cheap action (e.g. hide "X" [delete]
but allow "Edit").

[Dan Beam, 2014-07-30 22:15:18]

On 2014/07/30 21:45:05, Evan Stade wrote:
> On 2014/07/30 19:38:01, engedy wrote:
> > I agree that this is somewhat confusing. I cannot really think of any good
> > visual indication. What would you prefer?
> 
> I agree with Dan. I think the desired behavior would be that clicking an 'X'
> when multiple lines are selected only deletes the line the 'X' is on, and also
> removes the selection. If you want to delete a lot of stuff at once, you'd
have
> to press the delete key.

+1, this seems reasonable enough

> 
> If this is not what other people feel is correct, then we should probably
> consult an interaction designer.

[engedy, 2014-07-31 09:35:48]

I have prepared https://codereview.chromium.org/435553003/ to address the
concerns with the deletion. I will land that other CL before this.

@Dan, please take another look.

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

https://codereview.chromium.org/429073006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:94: handleFocus_:
function() {
On 2014/07/30 19:05:34, Dan Beam wrote:
> if (this.selected)
>   return;

Done.

[Dan Beam, 2014-08-01 18:29:58]

lgtm

[engedy, 2014-08-01 19:13:42]

The CQ bit was checked by engedy@chromium.org

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

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

[commit-bot: I haz the power, 2014-08-02 06:03:43]

Change committed as 287162