Skip to content

Commit

Permalink
Reland "[Files]: Merging search container and search controller."
Browse files Browse the repository at this point in the history 
With Search V1 removed we have a change to merge search container and
search controller. The latter handles two cases. One: changes in search
(either query or options) to request the directory model to perform
a file search. Two: changes in the current directory, to request that
the current search is terminated. These two functions are now migrated
to search container.

Note that due to a merge conflict (TS conversion of metrics) this is a
manually created re-land. This version is equivalent to patch 13 of
https://chromium-review.googlesource.com/c/chromium/src/+/4875757, which
is the reverted version.

Bug: b:300012155
Change-Id: Id6d00a6ea4e4c869d5f6b95a0a5debfb98470b5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4929177
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Bo Majewski <majewski@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1211816}
  • Loading branch information
Bo Majewski authored and Chromium LUCI CQ committed Oct 18, 2023
1 parent eeac87f commit ad449b4
Show file tree
Hide file tree
Showing 11 changed files with 239 additions and 217 deletions.
1 change: 1 addition & 0 deletions chrome/browser/ash/file_manager/file_manager_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1564,6 +1564,7 @@ WRAPPED_INSTANTIATE_TEST_SUITE_P(
TestCase("searchImageByContent")
.EnableLocalImageSearch()
.EnableSearchV2(),
TestCase("changingDirectoryClosesSearch").EnableSearchV2(),
TestCase("searchQueryLaunchParam")));

WRAPPED_INSTANTIATE_TEST_SUITE_P(
Expand Down
59 changes: 39 additions & 20 deletions ui/file_manager/file_manager/containers/search_container.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@ import {CrInputElement} from 'chrome://resources/cr_elements/cr_input/cr_input.j

import {queryRequiredElement} from '../common/js/dom_utils.js';
import {recordUserAction} from '../common/js/metrics.js';
import {str} from '../common/js/util.js';
import {str, strf} from '../common/js/util.js';
import {VolumeManagerCommon} from '../common/js/volume_manager_types.js';
import {CurrentDirectory, PropStatus, SearchData, SearchLocation, SearchOptions, SearchRecency, State} from '../externs/ts/state.js';
import {VolumeManager} from '../externs/volume_manager.js';
import {PathComponent} from '../foreground/js/path_component.js';
import {A11yAnnounce} from '../foreground/js/ui/a11y_announce.js';
import {changeDirectory} from '../state/ducks/current_directory.js';
import {clearSearch, getDefaultSearchOptions, updateSearch} from '../state/ducks/search.js';
import {clearSearch, getDefaultSearchOptions, isSearchEmpty, updateSearch} from '../state/ducks/search.js';
import type {FileKey} from '../state/file_key.js';
import {getStore, type Store} from '../state/store.js';
import {type BreadcrumbClickedEvent, XfBreadcrumb} from '../widgets/xf_breadcrumb.js';
Expand Down Expand Up @@ -190,6 +191,9 @@ export class SearchContainer extends EventTarget {
private pathComponents_: FileKey[] = [];
// Volume manager, used by us to resolve paths of selected entries.
private volumeManager_: VolumeManager;
// The accessibility interface that is used to announce the outcomes of file
// searches.
private a11y_: A11yAnnounce;


/**
Expand All @@ -204,7 +208,8 @@ export class SearchContainer extends EventTarget {
*/
constructor(
volumeManager: VolumeManager, searchWrapper: HTMLElement,
optionsContainer: HTMLElement, pathContainer: HTMLElement) {
optionsContainer: HTMLElement, pathContainer: HTMLElement,
a11y: A11yAnnounce) {
super();
this.volumeManager_ = volumeManager;
// The "box" around the search button, query input, and clear button.
Expand All @@ -231,6 +236,7 @@ export class SearchContainer extends EventTarget {

this.optionsContainer_ = optionsContainer;
this.pathContainer_ = pathContainer;
this.a11y_ = a11y;
this.store_ = getStore();
this.store_.subscribe(this);

Expand Down Expand Up @@ -264,21 +270,12 @@ export class SearchContainer extends EventTarget {
}
}

/**
* Sets the new query. This method does not post events, even if the query
* changed as a result.
*/
setQuery(query: string) {
this.inputElement_.value = query;
this.openSearch();
}

/**
* Returns the user entered search query. This method trims white spaces from
* the left side of the query.
*/
getQuery(): string {
return this.inputElement_.value.trimLeft();
return this.inputElement_.value.trimStart();
}

/**
Expand All @@ -301,19 +298,33 @@ export class SearchContainer extends EventTarget {
return;
}
// Cache the last received search state for future comparisons.
this.searchState_ = search;
if (!search) {
const lastSearch = this.searchState_;
this.searchState_ = state.search;

if (lastSearch?.query && search && search.query === undefined) {
this.a11y_.speakA11yMessage(str('SEARCH_A11Y_CLEAR_SEARCH'));
}
if (!search || isSearchEmpty(search)) {
this.closeSearch();
return;
}

const query = search.query;
if (query !== undefined && query !== this.getQuery()) {
this.setQuery(query);
this.inputElement_.value = query;
this.openSearch();
}
if (search.status === PropStatus.STARTED && query) {
this.showOptionsElement_(state);
this.showBreadcrumbElement_();
}
if (search.status === PropStatus.SUCCESS && query) {
const content = state.currentDirectory?.content;
const count = content ? content.keys.length : 0;
const messageId =
count === 0 ? 'SEARCH_A11Y_NO_RESULT' : 'SEARCH_A11Y_RESULT';
this.a11y_.speakA11yMessage(strf(messageId, query));
}
}

/**
Expand Down Expand Up @@ -576,6 +587,14 @@ export class SearchContainer extends EventTarget {
});
}

/**
* Returns whether the search container is open. In the open state the user
* may enter a search query, interact with options, etc.
*/
isOpen() {
return this.inputState_ === SearchInputState.OPEN;
}

/**
* Starts the process of opening the search widget. We use CSS transitions to
* open the widget and thus the widget it not fully opened until the CSS
Expand All @@ -585,6 +604,7 @@ export class SearchContainer extends EventTarget {
// Do not initiate open transition if we are not closed. This would leave us
// in the OPENING state, without ever getting to OPEN state.
if (this.inputState_ === SearchInputState.CLOSED) {
this.inputState_ = SearchInputState.OPEN;
this.inputElement_.addEventListener('transitionend', () => {
this.searchWrapper_.removeAttribute('collapsed');
}, {once: true, passive: true, capture: true});
Expand All @@ -595,7 +615,6 @@ export class SearchContainer extends EventTarget {
this.searchBox_.classList.add('has-cursor', 'has-text');
this.searchButton_.tabIndex = -1;
this.updateClearButton_(this.getQuery());
this.inputState_ = SearchInputState.OPEN;
}
}

Expand All @@ -608,6 +627,7 @@ export class SearchContainer extends EventTarget {
// Do not initiate close transition if we are not open. This would leave us
// in the CLOSING state, without ever getting to CLOSED state.
if (this.inputState_ === SearchInputState.OPEN) {
this.inputState_ = SearchInputState.CLOSED;
this.inputElement_.addEventListener('transitionend', () => {
this.searchWrapper_.setAttribute('collapsed', '');
}, {once: true, passive: true, capture: true});
Expand All @@ -622,7 +642,6 @@ export class SearchContainer extends EventTarget {
this.searchBox_.classList.remove('has-cursor', 'has-text');
this.searchButton_.tabIndex = 0;
this.currentOptions_ = getDefaultSearchOptions();
this.inputState_ = SearchInputState.CLOSED;
}
}

Expand All @@ -642,8 +661,8 @@ export class SearchContainer extends EventTarget {
this.updateClearButton_(query);
this.store_.dispatch(updateSearch({
query: query,
status: undefined, // do not change
options: undefined, // do not change
status: undefined, // do not change
options: this.currentOptions_,
}));
}
}
126 changes: 100 additions & 26 deletions ui/file_manager/file_manager/containers/search_container_unittest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,57 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import {loadTimeData} from 'chrome://resources/ash/common/load_time_data.m.js';
import {CrInputElement} from 'chrome://resources/cr_elements/cr_input/cr_input.js';
import {getTrustedHTML} from 'chrome://resources/js/static_types.js';
import {assertEquals, assertFalse, assertTrue} from 'chrome://webui-test/chromeos/chai_assert.js';

import {EntryLocation} from '../externs/entry_location.js';
import {State} from '../externs/ts/state.js';
import {PropStatus, State} from '../externs/ts/state.js';
import {VolumeManager} from '../externs/volume_manager.js';
import {A11yAnnounce} from '../foreground/js/ui/a11y_announce.js';
import {clearSearch, getDefaultSearchOptions, updateSearch} from '../state/ducks/search.js';
import {waitDeepEquals} from '../state/for_tests.js';
import {getEmptyState, getStore, type Store} from '../state/store.js';

import {SearchContainer} from './search_container.js';

class TestA11yAnnouncer extends A11yAnnounce {
messages: string[] = [];

override speakA11yMessage(message: string) {
this.messages.push(message);
}
}

let searchWrapper: HTMLElement|undefined;
let store: Store|undefined;
let searchContainer: SearchContainer|undefined;
const a11y: TestA11yAnnouncer = new TestA11yAnnouncer();

function setupStore(): Store {
const store = getStore();
/**
* Creates a store if necessary. Initializes it to an empty state.
*/
function setupStore(): void {
if (store === undefined) {
store = getStore();
}
store.init(getEmptyState());
return store;
}

/**
* Creates a search container if necessary.
*/
function setupSearchContainer(): void {
if (searchContainer === undefined) {
const volumeManager: VolumeManager = {
getLocationInfo: (_entry: Entry): EntryLocation => {
return new EntryLocation();
},
} as unknown as VolumeManager;
searchContainer = new SearchContainer(
volumeManager, document.querySelector('#search-wrapper') as HTMLElement,
document.querySelector('#options-container') as HTMLElement,
document.querySelector('#path-container') as HTMLElement, a11y);
}
}

/**
Expand All @@ -44,35 +74,27 @@ export function setUp() {
<div id="path-container"></div>
</div>
</div>`;
searchWrapper = document.querySelector('#search-wrapper') as HTMLElement;

store = setupStore();
const volumeManager: VolumeManager = {
getLocationInfo: (_entry: Entry): EntryLocation => {
return new EntryLocation();
},
} as unknown as VolumeManager;

searchContainer = new SearchContainer(
volumeManager, searchWrapper,
document.querySelector('#options-container') as HTMLElement,
document.querySelector('#path-container') as HTMLElement);

setupStore();
setupSearchContainer();
}

/**
* Resets flags state.
*/
export function tearDown() {
loadTimeData.resetForTesting();
// Clears accessibility message from the previous test.
a11y.messages = [];
}

/**
* Checks that manually entering a query (simulated here by setting value and
* posint an input event) correctly propagates the state to the store.
*/
export async function testQueryUpdated() {
// Manually open the search container; without this the container is in the
// closed state and does not clean up query or option values on close.
searchContainer!.openSearch();

// Test 1: Enter a query.
const input = searchWrapper!.querySelector('cr-input') as CrInputElement;
const input = document.querySelector('cr-input') as CrInputElement;
input.value = 'hello';
input.dispatchEvent(new Event('input', {
bubbles: true,
Expand All @@ -81,7 +103,7 @@ export async function testQueryUpdated() {
const want1 = {
query: 'hello',
status: undefined,
options: undefined,
options: getDefaultSearchOptions(),
};
await waitDeepEquals(store!, want1, (state: State) => {
return state.search;
Expand All @@ -99,4 +121,56 @@ export async function testQueryUpdated() {
});
}

// TODO(b:241868453): Add test for V2
/**
* Checks that store changes correctly result in opening and closing of the
* search box.
*/
export async function testOpenAndClose() {
assertFalse(searchContainer!.isOpen());
store!.dispatch(updateSearch({
query: 'hello',
status: undefined,
options: getDefaultSearchOptions(),
}));
assertTrue(searchContainer!.isOpen());
store!.dispatch(clearSearch());
assertFalse(searchContainer!.isOpen());
// No results appeared so expect just one message about search being closed.
assertEquals(1, a11y.messages.length);
assertEquals(
'Search text cleared, showing all files and folders.', a11y.messages[0]);
}

export async function testNoResultFoundAnnouncement() {
// Start a file search with the query 'hello'.
store!.dispatch(updateSearch({
query: 'hello',
status: undefined,
options: getDefaultSearchOptions(),
}));
// Wait for the store to update its state.
const want1 = {
query: 'hello',
status: undefined,
options: getDefaultSearchOptions(),
};
await waitDeepEquals(store!, want1, (state: State) => {
return state.search;
});
// Fake a successful search.
store!.dispatch(updateSearch({
query: 'hello',
status: PropStatus.SUCCESS,
options: getDefaultSearchOptions(),
}));
const want2 = {
query: 'hello',
status: PropStatus.SUCCESS,
options: getDefaultSearchOptions(),
};
await waitDeepEquals(store!, want2, (state: State) => {
return state.search;
});
assertEquals(1, a11y.messages.length);
assertEquals('There are no results for hello.', a11y.messages[0]);
}
14 changes: 0 additions & 14 deletions ui/file_manager/file_manager/foreground/js/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ group("js_modules") {
":path_component",
":providers_model",
":scan_controller",
":search_controller",
":selection_menu_controller",
":sort_menu_controller",
":spinner_controller",
Expand Down Expand Up @@ -130,7 +129,6 @@ js_type_check("closure_compile_jsmodules") {
":path_component",
":providers_model",
":scan_controller",
":search_controller",
":selection_menu_controller",
":sort_menu_controller",
":spinner_controller",
Expand Down Expand Up @@ -493,7 +491,6 @@ js_library("file_manager") {
":navigation_uma",
":providers_model",
":scan_controller",
":search_controller",
":selection_menu_controller",
":sort_menu_controller",
":spinner_controller",
Expand Down Expand Up @@ -852,17 +849,6 @@ js_library("scan_controller") {
]
}

js_library("search_controller") {
deps = [
":directory_model",
"ui:file_manager_ui",
"//ui/file_manager/file_manager/common/js:util",
"//ui/file_manager/file_manager/common/js:volume_manager_types",
"//ui/file_manager/file_manager/externs:entry_location",
"//ui/file_manager/file_manager/externs:volume_manager",
]
}

js_library("selection_menu_controller") {
deps = [
"ui:menu",
Expand Down

0 comments on commit ad449b4

Please sign in to comment.