Skip to content

Commit

Permalink
[FilesRecentFilter] Fix the drag hit logic in list view
Browse files Browse the repository at this point in the history 
* remove the ":before" css for group heading in list view to make sure
drag works naturally, it's also beneficial to the a11y (in later CL)
* move the drag hit related logic from file_table to file_table_list
because it's logically belonging to the list
* update the drag hit logic for the file list item to exclude the group
heading

before fix: http://shortn/_s8RBadkEJH
after fix: http://shortn/_MOWmAUxV4R

Bug: 1350859
Test: browser_tests --gtest_filter="*FileTableList*"
Change-Id: Ic4475c32cb6f58ef78ead391080c5ba4831b48a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3812980
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Wenbo Jie <wenbojie@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032428}
  • Loading branch information
@PinkyJie
PinkyJie authored and Chromium LUCI CQ committed Aug 8, 2022
1 parent 82d8c3d commit dc8ff69
Show file tree
Hide file tree
Showing 6 changed files with 136 additions and 77 deletions.
21 changes: 7 additions & 14 deletions ui/file_manager/file_manager/foreground/css/file_manager.css
View file
Expand Up @@ -1459,6 +1459,8 @@ body.files-ng grid .grid-title {
font-family: 'Roboto Medium';
padding-inline-start: 16px;
padding-top: 20px;
/* Make sure clicking this area can unselect file grids. */
pointer-events: none;
width: 100%;
}

Expand Down Expand Up @@ -1798,26 +1800,17 @@ body.files-ng #list-container li {
}

/* Modify GROUP_HEADING_HEIGHT in file_table_list.js if this height changes. */
#list-container li[group-heading] {
/* The desired height plus the border width. */
margin-top: calc(56px + 1px);
}

#list-container li[group-heading]::before {
#list-container .group-heading {
border-bottom: 1px solid var(--cros-separator-color);
box-sizing: border-box;
color: var(--cros-text-color-secondary);
content: attr(group-heading);
display: block;
font-family: 'Roboto Medium';
left: 0;
padding-bottom: 16px;
padding-inline-end: 20px;
/* The desired height plus the border width. */
height: calc(56px + 1px);
padding-inline-start: 16px;
padding-top: 20px;
/* Make sure clicking this area can unselect file items. */
pointer-events: none;
position: absolute;
/* Align with li[group-heading]'s margin-top. */
top: -57px;
width: 100%;
}

Expand Down
7 changes: 1 addition & 6 deletions ui/file_manager/file_manager/foreground/js/ui/BUILD.gn
View file
Expand Up @@ -399,18 +399,15 @@ js_library("file_table") {
deps = [
":a11y_announce",
":drag_selector",
":file_list_selection_model",
":file_metadata_formatter",
":file_table_list",
"table:table",
"table:table_column",
"table:table_column_model",
"table:table_list",
"//ui/file_manager/file_manager/common/js:async_util",
"//ui/file_manager/file_manager/common/js:file_type",
"//ui/file_manager/file_manager/common/js:importer_common",
"//ui/file_manager/file_manager/common/js:util",
"//ui/file_manager/file_manager/externs:entry_location",
"//ui/file_manager/file_manager/externs:files_app_entry_interfaces",
"//ui/file_manager/file_manager/externs:volume_manager",
"//ui/file_manager/file_manager/externs/background:import_history",
Expand All @@ -420,9 +417,6 @@ js_library("file_table") {
"//ui/file_manager/file_manager/foreground/js/metadata:metadata_model",
"//ui/webui/resources/js:assert.m",
"//ui/webui/resources/js:cr.m",
"//ui/webui/resources/js/cr/ui:list.m",
"//ui/webui/resources/js/cr/ui:list_item.m",
"//ui/webui/resources/js/cr/ui:list_selection_model.m",
]
}

Expand All @@ -437,6 +431,7 @@ js_unittest("file_table_unittest") {
js_library("file_table_list") {
deps = [
":a11y_announce",
":drag_selector",
":file_list_selection_model",
":file_tap_handler",
"table:table",
Expand Down
44 changes: 0 additions & 44 deletions ui/file_manager/file_manager/foreground/js/ui/file_table.js
View file
Expand Up @@ -4,16 +4,12 @@

import {assert, assertInstanceof} from 'chrome://resources/js/assert.m.js';
import {dispatchSimpleEvent} from 'chrome://resources/js/cr.m.js';
import {List} from 'chrome://resources/js/cr/ui/list.m.js';
import {ListItem} from 'chrome://resources/js/cr/ui/list_item.m.js';
import {ListSelectionModel} from 'chrome://resources/js/cr/ui/list_selection_model.m.js';

import {AsyncUtil} from '../../../common/js/async_util.js';
import {FileType} from '../../../common/js/file_type.js';
import {importer} from '../../../common/js/importer_common.js';
import {str, strf, util} from '../../../common/js/util.js';
import {importerHistoryInterfaces} from '../../../externs/background/import_history.js';
import {EntryLocation} from '../../../externs/entry_location.js';
import {FilesAppEntry} from '../../../externs/files_app_entry_interfaces.js';
import {VolumeManager} from '../../../externs/volume_manager.js';
import {FilesTooltip} from '../../elements/files_tooltip.js';
Expand All @@ -23,13 +19,11 @@ import {MetadataModel} from '../metadata/metadata_model.js';

import {A11yAnnounce} from './a11y_announce.js';
import {DragSelector} from './drag_selector.js';
import {FileListSelectionModel, FileListSingleSelectionModel} from './file_list_selection_model.js';
import {FileMetadataFormatter} from './file_metadata_formatter.js';
import {filelist, FileTableList} from './file_table_list.js';
import {Table} from './table/table.js';
import {TableColumn} from './table/table_column.js';
import {TableColumnModel} from './table/table_column_model.js';
import {TableList} from './table/table_list.js';

/**
* Custom column model for advanced auto-resizing.
Expand Down Expand Up @@ -538,31 +532,6 @@ export class FileTable extends Table {
}.bind(self), true);
self.list.shouldStartDragSelection =
self.shouldStartDragSelection_.bind(self);
self.list.hasDragHitElement = self.hasDragHitElement_.bind(self);

/**
* Obtains the index list of elements that are hit by the point or the
* rectangle.
*
* @param {number} x X coordinate value.
* @param {number} y Y coordinate value.
* @param {number=} opt_width Width of the coordinate.
* @param {number=} opt_height Height of the coordinate.
* @return {Array<number>} Index list of hit elements.
* @this {List}
*/
self.list.getHitElements = function(x, y, opt_width, opt_height) {
const currentSelection = [];
const bottom = y + (opt_height || 0);
for (let i = 0; i < this.selectionModel_.length; i++) {
const itemMetrics = this.getHeightsForIndex(i);
if (itemMetrics.top < bottom &&
itemMetrics.top + itemMetrics.height >= y) {
currentSelection.push(i);
}
}
return currentSelection;
};
}

/**
Expand Down Expand Up @@ -758,19 +727,6 @@ export class FileTable extends Table {
this.useModificationByMeTime_ = useModificationByMeTime;
}

/**
* Returns whether the drag event is inside a file entry in the list (and not
* the background padding area).
* @param {MouseEvent} event Drag start event.
* @return {boolean} True if the mouse is over an element in the list, False
* if
* it is in the background.
*/
hasDragHitElement_(event) {
const pos = DragSelector.getScrolledPosition(this.list, event);
return this.list.getHitElements(pos.x, pos.y).length !== 0;
}

/**
* Obtains if the drag selection should be start or not by referring the mouse
* event.
Expand Down
65 changes: 61 additions & 4 deletions ui/file_manager/file_manager/foreground/js/ui/file_table_list.js
View file
Expand Up @@ -17,6 +17,7 @@ import {FileListModel} from '../file_list_model.js';
import {MetadataModel} from '../metadata/metadata_model.js';

import {A11yAnnounce} from './a11y_announce.js';
import {DragSelector} from './drag_selector.js';
import {FileListSelectionModel, FileListSingleSelectionModel} from './file_list_selection_model.js';
import {FileTapHandler} from './file_tap_handler.js';
import {TableList} from './table/table_list.js';
Expand All @@ -26,7 +27,7 @@ import {TableList} from './table/table_list.js';
*/
const filelist = {};

// Group Heading height, align with CSS #list-container li[group-heading].
// Group Heading height, align with CSS #list-container .group-heading.
const GROUP_HEADING_HEIGHT = 57;

/**
Expand Down Expand Up @@ -86,9 +87,12 @@ export class FileTableList extends TableList {
}
// Check if index i is the start of a new group.
if (startIndexToGroupLabel.has(i)) {
item.setAttribute('group-heading', startIndexToGroupLabel.get(i).label);
} else {
item.removeAttribute('group-heading');
// For first item in each group, we add a title div before the element.
const title = document.createElement('div');
title.innerText = startIndexToGroupLabel.get(i).label;
title.classList.add(
'group-heading', `group-by-${fileListModel.groupByField}`);
this.insertBefore(title, item);
}
}

Expand Down Expand Up @@ -198,6 +202,12 @@ export class FileTableList extends TableList {
* Override here because previously we just need to use the index to multiply
* the item height, now we also need to add up the potential group heading
* heights included in these items.
*
* Note: for group start item, technically its height should be "all heights
* above it + current group heading height", but here we don't add the
* current group heading height (logic in getGroupHeadingCountBeforeIndex_),
* that's because it will break the "beforeFillerHeight" logic in the redraw
* of list.js.
* @override
*/
getItemTop(index) {
Expand Down Expand Up @@ -225,6 +235,53 @@ export class FileTableList extends TableList {
return (this.dataModel.length - lastIndex) * itemHeight +
countOfGroupHeadings * this.getGroupHeadingHeight_();
}

/**
* Returns whether the drag event is inside a file entry in the list (and not
* the background padding area).
* @param {MouseEvent} event Drag start event.
* @return {boolean} True if the mouse is over an element in the list, False
* if it is in the background.
*/
hasDragHitElement(event) {
const pos = DragSelector.getScrolledPosition(this, event);
return this.getHitElements(pos.x, pos.y).length !== 0;
}

/**
* Obtains the index list of elements that are hit by the point or the
* rectangle.
*
* @param {number} x X coordinate value.
* @param {number} y Y coordinate value.
* @param {number=} opt_width Width of the coordinate.
* @param {number=} opt_height Height of the coordinate.
* @return {Array<number>} Index list of hit elements.
*/
getHitElements(x, y, opt_width, opt_height) {
const fileListModel = /** @type {FileListModel} */ (this.dataModel);
const groupBySnapshot =
fileListModel ? fileListModel.getGroupBySnapshot() : [];
const startIndexToGroupLabel = new Map(groupBySnapshot.map(group => {
return [group.startIndex, group];
}));

const currentSelection = [];
const startHeight = y;
const endHeight = y + (opt_height || 0);
for (let i = 0; i < this.selectionModel.length; i++) {
const itemMetrics = this.getHeightsForIndex(i);
// For group start item, we need to explicitly add group height because
// its top doesn't take that into consideration. (check notes in
// getItemTop())
const itemTop = itemMetrics.top +
(startIndexToGroupLabel.has(i) ? this.getGroupHeadingHeight_() : 0);
if (itemTop < endHeight && itemTop + itemMetrics.height >= startHeight) {
currentSelection.push(i);
}
}
return currentSelection;
}
}

/**
Expand Down
55 changes: 54 additions & 1 deletion ui/file_manager/file_manager/foreground/js/ui/file_table_list_unittest.js
View file
Expand Up @@ -3,7 +3,7 @@
// found in the LICENSE file.

import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
import {assertEquals, assertFalse, assertTrue} from 'chrome://webui-test/chai_assert.js';
import {assertArrayEquals, assertEquals, assertFalse, assertTrue} from 'chrome://webui-test/chai_assert.js';

import {MockVolumeManager} from '../../../background/js/mock_volume_manager.js';
import {FakeEntryImpl} from '../../../common/js/files_app_entry_types.js';
Expand Down Expand Up @@ -535,3 +535,56 @@ export function testGetIndexForListOffset() {
assertEquals(tableList.getIndexForListOffset_(400), 7);
assertEquals(tableList.getIndexForListOffset_(500), 9);
}

export function testGetHitElements() {
const tableList = setupFileTableList();

// No group heading.
// index height total height
// -----------------------------------
// Item 0 40 40
// Item 1 40 80
// Item 2 40 120
// Item 3 40 160
// Item 4 40 200
// Item 5 40 240
// Item 6 40 280
// Item 7 40 320
// Item 8 40 360
// Item 9 40 400

// Passing -1 for 1st/3rd parameter because we don't care the x coordinates
// and the width of the drag selection.
assertArrayEquals(tableList.getHitElements(-1, 10), [0]);
assertArrayEquals(
tableList.getHitElements(-1, 50, -1, 200), [1, 2, 3, 4, 5, 6]);
assertArrayEquals(tableList.getHitElements(-1, 240, -1, 100), [5, 6, 7, 8]);

// Enable group by.
enableGroupByForDataModel(tableList.dataModel);
// index height total height
// -----------------------------------
// Heading 1 20 20
// Item 0 40 60
// Item 1 40 100
// Heading 2 20 120
// Item 2 40 160
// Heading 3 20 180
// Item 3 40 220
// Item 4 40 260
// Heading 4 20 280
// Item 5 40 320
// Item 6 40 360
// Heading 5 20 380
// Item 7 40 420
// Item 8 40 460
// Heading 6 20 480
// Item 9 40 520

// Passing -1 for 1st/3rd parameter because we don't care the x coordinates
// and the width of the drag selection.
assertArrayEquals(tableList.getHitElements(-1, 10), []);
assertArrayEquals(tableList.getHitElements(-1, 40), [0]);
assertArrayEquals(tableList.getHitElements(-1, 50, -1, 200), [0, 1, 2, 3, 4]);
assertArrayEquals(tableList.getHitElements(-1, 220, -1, 100), [3, 4, 5]);
}
21 changes: 13 additions & 8 deletions ui/file_manager/integration_tests/file_manager/recents.js
View file
Expand Up @@ -1010,19 +1010,24 @@ testcase.recentsTimePeriodHeadings = async () => {
await remoteCall.waitForFiles(
appId, TestEntryInfo.getExpectedRows([todayFile, yesterdayFile]), {
// Ignore last modified time because it will show Today/Yesterday
// instead
// of the actual date.
// instead of the actual date.
ignoreLastModifiedTime: true,
});
// Check headings in list view mode.
const todayListItem =
await remoteCall.waitForElement(appId, 'li[group-heading="Today"]');
await remoteCall.waitForElementsCount(appId, ['.group-heading'], 2);
const groupHeadings = await remoteCall.callRemoteTestUtil(
'deepQueryAllElements', appId, ['.group-heading']);
chrome.test.assertEq(2, groupHeadings.length);
const fileItems = await remoteCall.callRemoteTestUtil(
'deepQueryAllElements', appId, ['.group-heading + .table-row']);
chrome.test.assertEq(2, fileItems.length);

chrome.test.assertEq('Today', groupHeadings[0].text);
chrome.test.assertEq(
todayFile.nameText, todayListItem.attributes['file-name']);
const yesterdayListItem =
await remoteCall.waitForElement(appId, 'li[group-heading="Yesterday"]');
todayFile.nameText, fileItems[0].attributes['file-name']);
chrome.test.assertEq('Yesterday', groupHeadings[1].text);
chrome.test.assertEq(
yesterdayFile.nameText, yesterdayListItem.attributes['file-name']);
yesterdayFile.nameText, fileItems[1].attributes['file-name']);

// Switch to grid view.
await remoteCall.waitAndClickElement(appId, '#view-button');
Expand Down

0 comments on commit dc8ff69

Please sign in to comment.