1353862 - Performing a search then clicking on a category should remove the 'Search Results' category and clear the search

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Set browser.preferences.search to true in about:config
Open about:preferences
Type in "Reports" in the search box on the top right
Wait a second for the search results to appear
Click on a category on the left

Expected results:
The search should be cleared and the 'Search Results' category should hide.

Actual results:
The search query is still visible and the 'Search Results' category stays visible.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey ava1on, let jaws or myself know if you have any questions on how to approach this (iFerguson or manotejmeka might also have tips).

Comment 2

[Ziyan Long]

Attachment: Bug 1353862 - Performing a search then clicking on a category should remove the 'Search Results' category and clear the search.

Review commit: https://reviewboard.mozilla.org/r/131428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131428/

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8859401 [details]
Bug 1353862 - Performing a search then clicking on a category should remove the 'Search Results' category and clear the search.

https://reviewboard.mozilla.org/r/131428/#review134110

::: browser/components/preferences/in-content/preferences.js:142
(Diff revision 1)
>  function onHashChange() {
>    gotoPref();
>  }
>  
>  function gotoPref(aCategory) {
> +  if (aCategory == "paneGeneral" || aCategory == "paneApplications" || aCategory == "paneSync" || aCategory == "panePrivacy" || aCategory == "paneAdvanced") {

What is the opposite of this? In other words, when would we *not* want to run this block of code?

::: browser/components/preferences/in-content/preferences.js:145
(Diff revision 1)
> +    searchInput.value = "";
> +    searchInput.doCommand()

Please find a way to clear the search without calling doCommand.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8859401 [details]
Bug 1353862 - Performing a search then clicking on a category should remove the 'Search Results' category and clear the search.

https://reviewboard.mozilla.org/r/131428/#review134586

Clearing review until next patch.

Comment 5

[Ziyan Long]

Comment on attachment 8859401 [details]
Bug 1353862 - Performing a search then clicking on a category should remove the 'Search Results' category and clear the search.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131428/diff/1-2/

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8859401 [details]
Bug 1353862 - Performing a search then clicking on a category should remove the 'Search Results' category and clear the search.

https://reviewboard.mozilla.org/r/131428/#review136980

::: browser/components/preferences/in-content/preferences.js:91
(Diff revisions 1 - 2)
> -    helpButton.setAttribute("href", getHelpLinkURL(category.getAttribute("helpTopic")));
> -  }
> -
>    // Wait until initialization of all preferences are complete before
>    // notifying observers that the UI is now ready.
> -  Services.obs.notifyObservers(window, "advanced-pane-loaded");
> +  Services.obs.notifyObservers(window, "advanced-pane-loaded", null);

The rebase here is adding back changes that were previously removed (the null argument shouldn't be here anymore and the subcategory support is now missing).

::: browser/components/preferences/in-content/preferences.js:137
(Diff revisions 1 - 2)
> -    searchInput.doCommand()
> -  }
>    let categories = document.getElementById("categories");
> +  if (aCategory != "paneSearchResults" && aCategory != undefined && gSearchResultsPane.searchInput.value) {
> +    gSearchResultsPane.searchInput.value = "";
> +    gSearchResultsPane.searchFunction("");

searchFunction is designed to take an event as its first argument, so passing it a string will cause it to throw an exception when it tries to access event.target.value.

I'm not sure we should have to call searchFunction. Is it not enough to just do the normal show/hide that is performed by `search()` (not to be confused with `searchFunction()`)?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8859401 [details]
Bug 1353862 - Performing a search then clicking on a category should remove the 'Search Results' category and clear the search.

https://reviewboard.mozilla.org/r/131428/#review137480

Comment 8

[Evan Tseng [:evanxd]]

Hi Ziyan,

Are you still active to work on this?

Comment 9

[Ziyan Long]

(In reply to Evan Tseng [:evanxd] from comment #8)
> Hi Ziyan,
> 
> Are you still active to work on this?

It works. My bug is actually base on https://bugzilla.mozilla.org/show_bug.cgi?id=1349747. Thats why I call gSearchResultsPane in that way.
I am busy now. Probably I will not have time in this month.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Hey avalon - now that the MSU course is over, I'm going to unassign you from this one. A full-timer will drive this one through to a resolution. Thanks for your work!

Comment 11

[Evan Tseng [:evanxd]]

Attachment: Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category,

Review commit: https://reviewboard.mozilla.org/r/135426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135426/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8863632 [details]
Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category,

https://reviewboard.mozilla.org/r/135426/#review138500

::: browser/components/preferences/in-content/preferences.js:148
(Diff revision 1)
>  function onHashChange() {
>    gotoPref();
>  }
>  
>  function gotoPref(aCategory) {
> +  if (aCategory != undefined && aCategory != "paneSearchResults") {

Why do we need to check that aCategory is not undefined here? Which case is this handling?

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:178
(Diff revision 1)
>  });
> +
> +add_task(function*() {
> +  yield openPreferencesViaOpenPreferencesAPI("paneGeneral", {leaveOpen: true});
> +  let searchInput = gBrowser.contentDocument.getElementById("searchInput");
> +  searchInput.doCommand();

Do you know what this first doCommand call is supposed to accomplish? I realize that the other tests are doing this but it should not be necessary.

On "focus" we do initialize all the categories. Can you try replacing searchInput.doCommand() with searchInput.focus()?

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:180
(Diff revision 1)
> +  searchInput.doCommand();
> +  yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
> +
> +  yield openPreferencesViaOpenPreferencesAPI("privacy", {leaveOpen: true});
> +  let searchResultsCategory = gBrowser.contentDocument.getElementById("category-search-results");

This test isn't testing what this bug is about. By removing the tab, we are indirectly clearing the search results. 

This test should be opening the preferences, performing a search, verifying that the search results category is visible and selected as well as a selection created. The test should then click on another category and verify that the search results category is unselected, the searchInput is empty, and that there are no selections.

Comment 13

[Evan Tseng [:evanxd]]

Comment on attachment 8863632 [details]
Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135426/diff/1-2/

Comment 14

[Evan Tseng [:evanxd]]

Comment on attachment 8863632 [details]
Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category,

https://reviewboard.mozilla.org/r/135426/#review138500

> Why do we need to check that aCategory is not undefined here? Which case is this handling?

We don't need to check the undefined condition here. Remove it.

> Do you know what this first doCommand call is supposed to accomplish? I realize that the other tests are doing this but it should not be necessary.
> 
> On "focus" we do initialize all the categories. Can you try replacing searchInput.doCommand() with searchInput.focus()?

Sure, let's do it.

> This test isn't testing what this bug is about. By removing the tab, we are indirectly clearing the search results. 
> 
> This test should be opening the preferences, performing a search, verifying that the search results category is visible and selected as well as a selection created. The test should then click on another category and verify that the search results category is unselected, the searchInput is empty, and that there are no selections.

Sure, let's do it.

Comment 15

[Evan Tseng [:evanxd]]

Comment on attachment 8863632 [details]
Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135426/diff/2-3/

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8863632 [details]
Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category,

https://reviewboard.mozilla.org/r/135426/#review140592

Looks good, thank you for cleaning up the other parts of the test while you were there.

Comment 17

[Evan Tseng [:evanxd]]

Comment on attachment 8863632 [details]
Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135426/diff/3-4/

Comment 18

[Evan Tseng [:evanxd]]

Thanks for reviewing, Jared.

Then I found a bug in the patch, already discussed and update the patch for it with Jared in person.

The patch is tested locally, and it is good. Let's land the patch after the treeherder task[1] is good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=101dfe8ece47

Comment 19

[Evan Tseng [:evanxd]]

The treeherder try is good. Let's land the patch.

Comment 20

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 50e66cd3b469 -d 75a14f02f6f7: rebasing 394920:50e66cd3b469 "Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category, r=jaws" (tip)
merging browser/components/preferences/in-content/preferences.js
merging browser/components/preferences/in-content/tests/browser_search_within_preferences.js
warning: conflicts while merging browser/components/preferences/in-content/tests/browser_search_within_preferences.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 21

[Evan Tseng [:evanxd]]

Comment on attachment 8863632 [details]
Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135426/diff/4-5/

Comment 22

[Evan Tseng [:evanxd]]

Comment on attachment 8863632 [details]
Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135426/diff/5-6/

Comment 23

[Evan Tseng [:evanxd]]

Already finished the rebasing work. Will do `check-needed` again after the try[1] is good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42c2e8730fe4

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ae51eab2015
Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category, r=jaws

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ae51eab2015

Comment 26

[Hani Yacoub, Desktop QA]

Build ID: 20170514030207
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.