From 250da1ddc9aaa9bedf495cbcca1e95134d0e17a7 Mon Sep 17 00:00:00 2001 From: Drew Willcoxon Date: Wed, 10 Jul 2019 15:55:30 +0000 Subject: [PATCH] Bug 1563812 - Quantumbar: Fix keyboard navigation after stale results have been removed. r=dao We need to update indices after removing stale rows. Differential Revision: https://phabricator.services.mozilla.com/D37142 --HG-- extra : moz-landing-system : lando --- browser/components/urlbar/UrlbarView.jsm | 1 + .../browser/browser_selectStaleResults.js | 175 +++++++++++++++++- 2 files changed, 173 insertions(+), 3 deletions(-) diff --git a/browser/components/urlbar/UrlbarView.jsm b/browser/components/urlbar/UrlbarView.jsm index aaf472d98e4a..264b8e8f6b01 100644 --- a/browser/components/urlbar/UrlbarView.jsm +++ b/browser/components/urlbar/UrlbarView.jsm @@ -719,6 +719,7 @@ class UrlbarView { } row = next; } + this._updateIndices(); } _startRemoveStaleRowsTimer() { diff --git a/browser/components/urlbar/tests/browser/browser_selectStaleResults.js b/browser/components/urlbar/tests/browser/browser_selectStaleResults.js index 7e99954d2745..bc76d55d5859 100644 --- a/browser/components/urlbar/tests/browser/browser_selectStaleResults.js +++ b/browser/components/urlbar/tests/browser/browser_selectStaleResults.js @@ -2,12 +2,14 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -// This test makes sure that when the view shows a mix of stale and fresh -// results, arrowing down and up through the results works correctly. +// This test makes sure that arrowing down and up through the view's results +// works correctly with regard to stale results. "use strict"; -add_task(async function test() { +// This tests the case where queryContext.results.length < the number of rows in +// the view, i.e., the view contains stale rows. +add_task(async function viewContainsStaleRows() { await PlacesUtils.history.clear(); await PlacesUtils.bookmarks.eraseEverything(); @@ -87,6 +89,8 @@ add_task(async function test() { // the one-off search buttons at that point. for (let i = 1; i < maxResults; i++) { Assert.equal(UrlbarTestUtils.getSelectedIndex(window), i); + let result = await UrlbarTestUtils.getDetailsOfResultAt(window, i); + Assert.equal(result.element.row.result.uiIndex, i); EventUtils.synthesizeKey("KEY_ArrowDown"); } @@ -104,3 +108,168 @@ add_task(async function test() { EventUtils.synthesizeKey("KEY_Escape") ); }); + +// This tests the case where, before the search finishes, stale results have +// been removed and replaced with non-stale results. +add_task(async function staleReplacedWithFresh() { + // For this test, we need one set of results that's added quickly and another + // set that's added after a delay. We do an initial search and wait for both + // sets to be added. Then we do another search, but this time only wait for + // the fast results to be added, and then we arrow down to stop the search + // before the delayed results are added. The order in which things should + // happen after the second search goes like this: + // + // (1) second search + // (2) fast results are added + // (3) remove-stale-rows timer fires and removes stale rows (the rows from + // the delayed set of results from the first search) + // (4) we arrow down to stop the search + // + // We use history for the fast results and a slow search engine for the + // delayed results. + // + // NB: If this test ends up failing, it may be because the remove-stale-rows + // timer fires before the history results are added. i.e., steps 2 and 3 + // above happen out of order. If that happens, we'll need a way to force the + // timer not to fire until we want it to. + + await PlacesUtils.history.clear(); + await PlacesUtils.bookmarks.eraseEverything(); + + // Enable search suggestions, and add an engine that returns suggestions on a + // delay. + await SpecialPowers.pushPrefEnv({ + set: [["browser.urlbar.suggest.searches", true]], + }); + let engine = await SearchTestUtils.promiseNewSearchEngine( + getRootDirectory(gTestPath) + "searchSuggestionEngineSlow.xml" + ); + let oldDefaultEngine = await Services.search.getDefault(); + await Services.search.moveEngine(engine, 0); + await Services.search.setDefault(engine); + + let maxResults = UrlbarPrefs.get("maxRichResults"); + + // Add enough visits to pages with "test" in the title to fill up the entire + // view. + for (let i = 0; i < maxResults; i++) { + await PlacesTestUtils.addVisits({ + uri: "http://example.com/" + i, + title: "test" + i, + }); + } + + gURLBar.focus(); + + // Search for "tes" and wait for the search to finish. + await promiseAutocompleteResultPopup("tes", window, true); + + // Sanity check the results. They should be: + // + // tes -- Search with searchSuggestionEngineSlow [heuristic] + // tesfoo [search suggestion] + // tesbar [search suggestion] + // test9 [history] + // test8 [history] + // test7 [history] + // test6 [history] + // test5 [history] + // test4 [history] + // test3 [history] + let count = UrlbarTestUtils.getResultCount(window); + Assert.equal(count, maxResults); + let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 0); + Assert.ok(result.heuristic); + result = await UrlbarTestUtils.getDetailsOfResultAt(window, 1); + Assert.ok(result.searchParams); + Assert.equal(result.searchParams.suggestion, "tesfoo"); + result = await UrlbarTestUtils.getDetailsOfResultAt(window, 2); + Assert.ok(result.searchParams); + Assert.equal(result.searchParams.suggestion, "tesbar"); + for (let i = 3; i < maxResults; i++) { + result = await UrlbarTestUtils.getDetailsOfResultAt(window, i); + Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.URL); + Assert.equal(result.title, "test" + (maxResults - i + 2)); + } + + // Below we'll do a search for "test" *but* not wait for the two search + // suggestion results to be added. We'll only wait for the history results to + // be added. To determine when the history results are added, use a mutation + // listener on the node containing the rows, and wait until the title of the + // next-to-last row is "test2". At that point, the results should be: + // + // test -- Search with searchSuggestionEngineSlow + // test9 + // test8 + // test7 + // test6 + // test5 + // test4 + // test3 + // test2 + // test1 + let mutationPromise = new Promise(resolve => { + let observer = new MutationObserver(mutations => { + let row = gURLBar.view._rows.children[maxResults - 2]; + if (row && row._elements.get("title").textContent == "test2") { + observer.disconnect(); + resolve(); + } + }); + observer.observe(gURLBar.view._rows, { + subtree: true, + characterData: true, + childList: true, + attributes: true, + }); + }); + + // Now type a "t" so that we search for "test", but only wait for history + // results to be added, as described above. + EventUtils.synthesizeKey("t"); + await mutationPromise; + + // Now arrow down. The search, which is still ongoing, will now stop and the + // view won't be updated anymore. + EventUtils.synthesizeKey("KEY_ArrowDown"); + + // Wait for the search to stop. + await gURLBar.lastQueryContextPromise; + + // Sanity check the results. They should be as described above. + count = UrlbarTestUtils.getResultCount(window); + Assert.equal(count, maxResults); + result = await UrlbarTestUtils.getDetailsOfResultAt(window, 0); + Assert.ok(result.heuristic); + Assert.equal(result.element.row.result.uiIndex, 0); + for (let i = 1; i < maxResults; i++) { + result = await UrlbarTestUtils.getDetailsOfResultAt(window, i); + Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.URL); + Assert.equal(result.title, "test" + (maxResults - i)); + Assert.equal(result.element.row.result.uiIndex, i); + } + + // Arrow down through all the results. After arrowing down from "test3", we + // should continue on to "test2". We should *not* enter the one-off search + // buttons at that point. + for (let i = 1; i < maxResults; i++) { + Assert.equal(UrlbarTestUtils.getSelectedIndex(window), i); + EventUtils.synthesizeKey("KEY_ArrowDown"); + } + + // Now the first one-off should be selected. + Assert.equal(UrlbarTestUtils.getSelectedIndex(window), -1); + Assert.equal(gURLBar.view.oneOffSearchButtons.selectedButtonIndex, 0); + + // Arrow back up through all the results. + for (let i = maxResults - 1; i >= 0; i--) { + EventUtils.synthesizeKey("KEY_ArrowUp"); + Assert.equal(UrlbarTestUtils.getSelectedIndex(window), i); + } + + await UrlbarTestUtils.promisePopupClose(window, () => + EventUtils.synthesizeKey("KEY_Escape") + ); + await SpecialPowers.popPrefEnv(); + await Services.search.setDefault(oldDefaultEngine); +});