Closed Bug 1353862 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Version:
55 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: jaws, Assigned: evanxd)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference] )

Attachments

(1 file, 1 obsolete file)

Bug 1353862 - Remove the Search Results category, search input string, and highlights after performing a search then clicking on a category,
59 bytes, text/x-review-board-request
jaws
: review+
Details 
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.
Assignee: nobody → lzylong
Hey ava1on, let jaws or myself know if you have any questions on how to approach this (iFerguson or manotejmeka might also have tips).
Status: NEW → ASSIGNED
Blocks: 1357285
Whiteboard: [photon-preference]
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.
Attachment #8859401 - Flags: review?(jaws) → review-
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.
Attachment #8859401 - Flags: review?(mconley)
Target Milestone: --- → Firefox 55
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()`)?
Attachment #8859401 - Flags: review?(jaws) → review-
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
Attachment #8859401 - Flags: review?(mconley)
QA Contact: hani.yacoub
Flags: qe-verify+
Hi Ziyan,

Are you still active to work on this?
Flags: needinfo?(lzylong)
(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.
Flags: needinfo?(lzylong)
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!
Assignee: lzylong → nobody
Status: ASSIGNED → NEW
Assignee: nobody → evan
Status: NEW → ASSIGNED
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.
Attachment #8863632 - Flags: review?(jaws) → review-
Priority: -- → P1
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.
Attachment #8859401 - Attachment is obsolete: true
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.
Attachment #8863632 - Flags: review?(jaws) → review+
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
The treeherder try is good. Let's land the patch.
Keywords: checkin-needed
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)
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
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+ → in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: