From ca6bd61910347a8d6b53299b2a621c312aed7129 Mon Sep 17 00:00:00 2001 From: tsergeant Date: Thu, 6 Jul 2017 01:20:58 -0700 Subject: [PATCH] MD Bookmarks: Prevent flash of folder contents when changing search term In MD Bookmarks, changing search term would clear the results of the current search, then fire off a new search request, which would bring in new results <1 second later. This caused a brief flash of folder contents for whatever folder was currently selected. This CL fixes that issue by holding on to the old search results until the new search results are available. BUG=737076 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2972963003 Cr-Commit-Position: refs/heads/master@{#484494} --- .../resources/md_bookmarks/reducers.js | 7 ++-- .../browser/resources/md_bookmarks/types.js | 12 ++++++- chrome/browser/resources/md_bookmarks/util.js | 10 +++--- .../data/webui/md_bookmarks/reducers_test.js | 33 +++++++++++++++++-- 4 files changed, 52 insertions(+), 10 deletions(-) diff --git a/chrome/browser/resources/md_bookmarks/reducers.js b/chrome/browser/resources/md_bookmarks/reducers.js index 8337e5767457d..7154b83d59b3b 100644 --- a/chrome/browser/resources/md_bookmarks/reducers.js +++ b/chrome/browser/resources/md_bookmarks/reducers.js @@ -118,7 +118,7 @@ cr.define('bookmarks', function() { return { term: action.term, inProgress: true, - results: [], + results: search.results, }; }; @@ -139,7 +139,7 @@ cr.define('bookmarks', function() { return { term: '', inProgress: false, - results: [], + results: null, }; }; @@ -149,6 +149,9 @@ cr.define('bookmarks', function() { * @return {SearchState} */ SearchState.removeDeletedResults = function(search, deletedIds) { + if (!search.results) + return search; + var newResults = []; search.results.forEach(function(id) { if (!deletedIds.has(id)) diff --git a/chrome/browser/resources/md_bookmarks/types.js b/chrome/browser/resources/md_bookmarks/types.js index cf857680ae282..6361f9dc7c678 100644 --- a/chrome/browser/resources/md_bookmarks/types.js +++ b/chrome/browser/resources/md_bookmarks/types.js @@ -37,10 +37,20 @@ var NodeMap; var SelectionState; /** + * Note: + * - If |results| is null, it means no search results have been returned. This + * is different to |results| being [], which means the last search returned 0 + * results. + * - |term| is the last search that was performed by the user, and |results| are + * the last results that were returned from the backend. We don't clear + * |results| on incremental searches, meaning that |results| can be 'stale' + * data from a previous search term (while |inProgress| is true). If you need + * to know the exact search term used to generate |results|, you'll need to + * add a new field to the state to track it (eg, SearchState.resultsTerm). * @typedef {{ * term: string, * inProgress: boolean, - * results: !Array, + * results: ?Array, * }} */ var SearchState; diff --git a/chrome/browser/resources/md_bookmarks/util.js b/chrome/browser/resources/md_bookmarks/util.js index 73b156a2a0d6c..3760bdea171d3 100644 --- a/chrome/browser/resources/md_bookmarks/util.js +++ b/chrome/browser/resources/md_bookmarks/util.js @@ -14,10 +14,10 @@ cr.define('bookmarks.util', function() { * @return {!Array} */ function getDisplayedList(state) { - if (!isShowingSearch(state)) - return assert(state.nodes[state.selectedFolder].children); + if (isShowingSearch(state)) + return assert(state.search.results); - return state.search.results; + return assert(state.nodes[state.selectedFolder].children); } /** @@ -77,7 +77,7 @@ cr.define('bookmarks.util', function() { search: { term: '', inProgress: false, - results: [], + results: null, }, selection: { items: new Set(), @@ -91,7 +91,7 @@ cr.define('bookmarks.util', function() { * @return {boolean} */ function isShowingSearch(state) { - return !!state.search.term && !state.search.inProgress; + return state.search.results != null; } /** diff --git a/chrome/test/data/webui/md_bookmarks/reducers_test.js b/chrome/test/data/webui/md_bookmarks/reducers_test.js index 15fae4d3f5485..e8cfc37ee6c39 100644 --- a/chrome/test/data/webui/md_bookmarks/reducers_test.js +++ b/chrome/test/data/webui/md_bookmarks/reducers_test.js @@ -436,7 +436,7 @@ suite('search state', function() { assertFalse(bookmarks.util.isShowingSearch(clearedState)); assertDeepEquals(['3'], bookmarks.util.getDisplayedList(clearedState)); assertEquals('', clearedState.search.term); - assertDeepEquals([], clearedState.search.results); + assertDeepEquals(null, clearedState.search.results); // Case 2: Clear search by selecting a new folder. action = bookmarks.actions.selectFolder('1'); @@ -446,7 +446,36 @@ suite('search state', function() { assertFalse(bookmarks.util.isShowingSearch(selectedState)); assertDeepEquals(['2'], bookmarks.util.getDisplayedList(selectedState)); assertEquals('', selectedState.search.term); - assertDeepEquals([], selectedState.search.results); + assertDeepEquals(null, selectedState.search.results); + }); + + test('results do not clear while performing a second search', function() { + action = bookmarks.actions.setSearchTerm('te'); + state = bookmarks.reduceAction(state, action); + + assertFalse(bookmarks.util.isShowingSearch(state)); + + action = bookmarks.actions.setSearchResults(['2', '3']); + state = bookmarks.reduceAction(state, action); + + assertFalse(state.search.inProgress); + assertTrue(bookmarks.util.isShowingSearch(state)); + + // Continuing the search should not clear the previous results, which should + // continue to show until the new results arrive. + action = bookmarks.actions.setSearchTerm('test'); + state = bookmarks.reduceAction(state, action); + + assertTrue(state.search.inProgress); + assertTrue(bookmarks.util.isShowingSearch(state)); + assertDeepEquals(['2', '3'], bookmarks.util.getDisplayedList(state)); + + action = bookmarks.actions.setSearchResults(['3']); + state = bookmarks.reduceAction(state, action); + + assertFalse(state.search.inProgress); + assertTrue(bookmarks.util.isShowingSearch(state)); + assertDeepEquals(['3'], bookmarks.util.getDisplayedList(state)); }); test('removes deleted nodes', function() {