New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 742689 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 756297



Sign in to add a comment

Files app should exit check-select mode after deleting selected files

Project Member Reported by yamaguchi@chromium.org, Jul 14 2017

Issue description

Chrome Version: 61.0.3156.0

Steps To Reproduce:
(1) Open a directory and create some dummy files / folders by copy-paste
(2) Select 1 or more files/folders by check-select mode.
(3) Delete the selected items by delete command. (either by the toolbar button or context menu, or alt+backspace)


Expected Result:
It gets out of the check-select mode.

Actual Result:
It stays the check-select mode. The item under the cursor (in the file list after deletion) is selected.

How frequently does this problem reproduce? (Always, sometimes, hard to
reproduce?)
100%

What is the impact to the user, and is there a workaround? If so, what is
it?
It can be confusing because it may look as if some of the selected files were not deleted.

This can be more problematic when we renew the touch UI ( Issue 730232 ,  740842 ), because user will always use check-select mode for deleting files.
 
Status: Available (was: Untriaged)
Summary: Files app should exit check-select mode after deleting selected files (was: Item under cursor selected automatically after deleting check-selected files)
I think having the selection cursor in place of the file right after the deleted ones would be the expected behavior.
The right description of this issue would be that it should exit the check-select mode after deletion. 

We have "lead index" concept. It is virtually a "cursor" that the user can move by arrow keys outside check-select mode.
In Chrome OS, we require the lead index to be a selected file, with our current design. This is common to other UIs using cr.ui.List.
(cf. in Windows, you can move cursor without changing selections by ctrl + arrow key.)
If we unselect all the files, it means that it forget the position of that cursor. It will disable the use scenario like this: When attempting to remove the 20th, 22th and 23th files in the directory having 30 files using keyboard.
1. move cursor by [down arrow] to the 20th file and then [delete]. Cursor stays on the 21th file.
2. [down arrow] [delete] [enter] to delete the 22th file.
3. [delete] [enter] to delete the 23th file.

Blocking: 730232
Labels: -Pri-2 Pri-3
Status: Unconfirmed (was: Available)
Adding this as a blocker for 730232 because this will become more apparent in touch mode.

Comment 3 by oka@chromium.org, Aug 28 2017

Labels: Hotlist-GoodFirstBug
Expected/actual results from user's perspective are like these:

Expected Result:
The toolbar is blue.
A file next to the deleted ones is focused. It is highlighted blue to indicate the cursor position. It is not selected by the check mark.

Actual Result:
The toolbar is white (multiple selection mode).
The file is selected by the check mark.

Comment 5 by oka@chromium.org, Aug 29 2017

Cc: marian...@google.com

Comment 6 by oka@chromium.org, Aug 29 2017

Owner: marian...@google.com
Status: Assigned (was: Unconfirmed)
Labels: M-63
Status: Started (was: Assigned)
Cc: yamaguchi@chromium.org oka@chromium.org fukino@chromium.org
Current result: I have adjusted file_list_selection_model to disable multiple selection mode if only one item is selected. I'm not sure this is desired behavior, as it will force a user out of multiple selection mode the moment he only has 1 file selected - what do others think on that?

While waiting for answers I'll see if I can restrict that behavior to only happen upon moving/deleting files.
It should also allow check-select mode (=multiple selection mode) even when only 1 item is selected.
One of the reasons is that it will change the toolbar buttons and overflow menu items while in the check-select mode. Similarly, when operating it with touchscreen UI, we always apply the check-select mode by design as long as 1+ item is selected.
Thanks for the input! Will work on it :)

"Similarly, when operating it with touchscreen UI, we always apply the check-select mode by design as long as 1+ item is selected." - how does that come into play for this bug?
You don't need to consider the touchscreen UI specifically. It is just one of the examples where we should see "single file selected in check-select mode".
Okay.

State of my research:
I have looked through different call stacks on different situations in which FileListSelectionModel.onChangeEvent_ gets called.

I'm in the process of extracting the point where it is unique to Deletions (a hell lot of cs to look through) - I have two suggestions on how to fix the bug, and I don't really like any of it - ordering is least bad first:
1 introduce a parameter that gets passed down and that indicates whether the change was triggered by deletion or not. Downside: Add a member to the event object that is quite hidden
2 alter the code for deletion to trigger disabling check mode. Downside: that would introduce a new signal that is quite hidden
3 Big refactoring for the structure to make this easier. Downside: a hell lot of code to touch.

I guess that 1 is still the best option out of these, and I'll work on it once I have identified the best point in the callstack to add it, if nobody suggests a better idea.

Comment 14 by oka@chromium.org, Aug 31 2017

I responded to your change (sorry for the delay).
Files app manages items and selections separately, as FileListModel and ListSelectionModel.
So actually it feels strange to introduce the notion of 'delete' to ListSelectionModel or its base class.
I suggest having a controller listen to an event from FileListModel and update ListSelectionModel.

Cc: weifaungsun@chromium.org mcirimele@chromium.org
Hi Weifang, hi Maria!
I am unsure on what the exact requirements are as to when to switch to single-select mode and when to stay in check-select mode.

The different situations:
* Files get deleted or moved directly by the user in question using the mouse
* File get deleted or moved directly by the user in question using touch
* Files get deleted or moved indirectly through some other source
* Special case of the last one: 2 files were selected, one of them gets deleted through some other source

Thanks for your input!


Comment 16 by oka@chromium.org, Sep 5 2017

Cc: -weifaungsun@chromium.org weifangsun@chromium.org

Comment 17 by oka@chromium.org, Sep 5 2017

The address was misspelled:) Weifang, could you answer Marianne's question above?
Sorry for the delay! mariannet@ -

The default behavior here should always be to exist the check-select mode on successful completion of an event (delete, move, copy, etc).
- I believe this should always hold true in the case of using touch.
- When using mouse/keyboard, it may make sense to focus on the "next" item. Maria and I are reviewing further, but let's move forward with implementing the default state first.

For the your second 2 use cases, can you clarify when this may occur? Is this a Drive specific behavior you are referring to?
Blocking: -730232
Blocking: 756297
I think the straightforward solution is to exit check-select mode only when all the selected item is deleted, regardless of whether those were selected before or after the deletion command. In such case, cr.ui.SelectionModel.adjustToReordering() currently moves the leading index to the next item that is not deleted. I think we should hook this event (by overriding the function or so).

This satisfies the requirement in the typical usage, when user doesn't do anything until all deletion is complete.
Additionally, it will not interfere user's operation even when the deletion happens asynchronously.
weifang@
Don't worry, I have 2 bugs I'm working on right now :)
I'm not sure what you mean by "2nd use case" - I'll just answer both situations I think are plausible:
> File get deleted or moved directly by the user in question using touch
I have heard that when using touch, the usual select mode is check-select (because single tapping will open the file). So my question here is: should the next item still be selected (in single-select mode), or should nothing be selected at all?
> Files get deleted or moved indirectly through some other source
This is Drive specific. The question is basically: what happens if you select the files on one device, but another device deletes them (usually, the other device would belong to another user, but the same events will fire if you only use one account for testing)?

> When using mouse/keyboard, it may make sense to focus on the "next" item. Maria and I are reviewing further, but let's move forward with implementing the default state first.
It is actually not hard to select the next item in single-select mode, just like not selecting anything at all is easy. The only difficult case would be to have different behavior for touch and mouse, as I would need to pass on information for that - so feel free to tell me which of the two you prefer (next file in single-select or nothing at all), and I'll do that :)

yamaguchi@
adjustToReordering() is exactly the point I'm touching right now. I'm trying to go with overriding it and call the superclass implementation, but I'm afraid I'll have to switch to copy the method at some point because the state gets cleaned up in endChange() so that I don't know exactly what has happened - right now I basically check if there is exactly one file selected after the super call to adjustToReordering() has happened, but that situation is broader than "all selected files got deleted". Inspiration was the bit of code in adjustToReordering that moves the selection to the next item that didn't get deleted.
Thanks for the clarification mariannet@!

First just to clarify: I call "single-select" (blue highlighted row) "focus" and "check-select" "selected". This is consistent with accessibility and (I think) a little clearer than check-select/single-select. 

In touch mode focus should not exist, so we should always remove selection and focus after deleting a file there. For now, we should do the same outside of touch mode. If a selected file is deleted, selection and focus is removed. 

This is the approach we want to move forward with. There are many edge cases here and a group of related use cases that the behavior has not been defined for. As we continue thinking through those it's very possible that we will need exceptions to the "always remove selection and focus" rule but we will tackle those later if needed. 
Labels: -Pri-3 Pri-2
Thanks for clarifying the vocab! I'll make sure to use it from now on :)
Hi Maria, (re:#23)

I'm reluctant to clear focus on deletion outside touch mode.
When I delete several files in a folder (i.e. delete blurred photos from DCIM folder), I usually do like this:
1) Press <DOWN> key several times
2) Press <DELETE> key
3) Press <DOWN> key several times
4) Press <DELETE> key
5) ...

If we clear focus in step 2, the focused file will be back to the 1st file in step 3.
This makes these operations harder, and make it almost impossible in a big folder which has hundreds of files.

As you mentioned, there should be edge cases like this.
How about keeping the focus right now and consider clearing the focus as a separated issue? (i.e. For this issue, only exiting check-select mode)
Project Member

Comment 27 by bugdroid1@chromium.org, Sep 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/010fb868a40cda46cec67806bcdc4a0728d5a93b

commit 010fb868a40cda46cec67806bcdc4a0728d5a93b
Author: mariannet <mariannet@google.com>
Date: Thu Sep 14 05:02:40 2017

When deleting files in check-select-mode, the mode will now switch back to
single select mode.

Before:
When deleting files in check-select-mode, the mode will stay in check-select,
the selected files would be at similar indices to the deleted files.
After:
The mode now changes to single select mode.

Bug:  742689 
Test: manual tests
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I3bf102cf2582e91085690577b08e2bed66f5d621
Reviewed-on: https://chromium-review.googlesource.com/644595
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Reviewed-by: Keigo Oka <oka@chromium.org>
Commit-Queue: Marianne Thieffry <mariannet@google.com>
Cr-Commit-Position: refs/heads/master@{#501872}
[modify] https://crrev.com/010fb868a40cda46cec67806bcdc4a0728d5a93b/chrome/browser/chromeos/file_manager/file_manager_jstest.cc
[modify] https://crrev.com/010fb868a40cda46cec67806bcdc4a0728d5a93b/ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js
[add] https://crrev.com/010fb868a40cda46cec67806bcdc4a0728d5a93b/ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model_unittest.html
[add] https://crrev.com/010fb868a40cda46cec67806bcdc4a0728d5a93b/ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model_unittest.js

In case different behavior is expected, I will adjust the code
Hi Fukino-san. Yes, the case you describe is a good candidate for an exception to the "always remove focus" rule.  

I've been working on mapping out all the different operations based on initial state and what should happen when the operation succeeds, fails, or is in progress. I'll discuss with Weifang on Friday and if we agree I should be able to provide a list of rules and exceptions by early next week. No need to change the code until then (if we need to at all). 

Thanks!
Weifang and I have gone through the cases and put together resources to describe the behavior we would like. A description of the behavior is here:
https://docs.google.com/a/google.com/document/d/1lAtmq2uYAV6nhjwhmj_HSm1iFEr3_gqertm-r45K9AY/edit?usp=sharing

IxD spec:
https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZbHF01iRNpsj/files/MCHzWOPdyz7kcRFjIH5q-EVm

Let me know if you have any questions or if there are cases not covered. Fukino-san, I'll leave it to you and the team to decide if you'd like to separate in to multiple bugs. 
Status: Fixed (was: Started)
Hi mcirimele@,

It seems the issue mentioned in this bug description was already fixed by mariannet@.
Let me mark this as Fixed.

The UI changes proposed in the doc in comment #30 seems to be beyond this bug.
mcirimele@, could you file a separated crbug issue for each requests in the doc?
Sounds good. I think most of the cases are WAI right now, filed issue 769011 for copying files into the same directory. Let me know if there are any other cases missing.

Sign in to add a comment