Closed Bug 1365847 Opened 2 years ago Closed 2 years ago

Search results are not correct for the "perform" keyword

Categories

(Firefox :: Preferences, defect, P1)

55 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox56 --- verified

People

(Reporter: evanxd, Assigned: evanxd)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

Search results are not correct for the "perform" (or "system") keyword.

STR:
1. Just search the "perform" keyword.

Actual:
Shows the "Performance" and "Nightly Updates" sections.

Expected:
Should only show the "Performance" section.
Flags: qe-verify+
Depends on: 1352481
Implemented the workable patch. But need to refine it and add tests.
Attachment #8871635 - Flags: review?(mconley)
Hi Mike,

Could you help review the patch?

Thanks.
Rebased.
Duplicate of this bug: 1368947
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

https://reviewboard.mozilla.org/r/143144/#review149258

Thanks for the patch. Is UX aware of this choice? This means that, for example, it's not possible to show results for the different states under History under Privacy and Security, for example. Are they okay with this?

::: commit-message-6eb85:1
(Diff revision 8)
> +Bug 1365847 - Do not search for unselected desk items.

Nit: "desk" -> "deck". Or, even better, "<xul:deck>".

::: browser/components/preferences/in-content-new/findInPage.js:330
(Diff revision 8)
>        }
>  
>        matchesFound = matchesFound || complexTextNodesResult || labelResult || valueResult || keywordsResult;
>      }
>  
> +    if (nodeObject.tagName != "deck") {

I think I'd probably prefer to have these conditions flipped, and do:

```js

if (nodeObject.tagName == "deck") {
  // ...
} else {
  // ...
}

```

::: browser/components/preferences/in-content-new/findInPage.js:343
(Diff revision 8)
> +      let index = nodeObject.getAttribute("selectedIndex");
> +      if (index != null) {

You can use:

`nodeObject.selectedIndex;`

Instead. It'll be -1 if nothing happens to be selected somehow.

::: browser/components/preferences/in-content-new/findInPage.js:347
(Diff revision 8)
> +          let result = this.searchWithinNode(deckSelectedNodeObject, searchPhrase);
> +          // Creating tooltips for menulist element
> +          if (result && nodeObject.tagName === "menulist") {
> +            this.listSearchTooltips.push(nodeObject);
> +          }
> +          matchesFound = matchesFound || result;

We're repeating ourselves here. Is there no way to extract this block out to a separate function so that both conditions can call it?

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_2.js:2
(Diff revision 8)
> +/*
> +* This file contains tests for the Preferences search bar.

Please add:

```
"use strict";
```

at the top of this file.

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_2.js:5
(Diff revision 8)
> +/*
> +* This file contains tests for the Preferences search bar.
> +*/
> +
> +// Enabling Searching functionatily. Will display search bar form this testcase forward.

Might as well put this in a /* */ comment block to match the other function, I guess.

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_2.js:10
(Diff revision 8)
> +/**
> + * Test for "perform" case. When we search "perform", it should show the "performanceGroup".
> + */

Instead of describing the individual case, we should perhaps describe the general problem: only search selected decks.
Attachment #8871635 - Flags: review?(mconley) → review-
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

https://reviewboard.mozilla.org/r/143144/#review149258

> We're repeating ourselves here. Is there no way to extract this block out to a separate function so that both conditions can call it?

Sure, let's do it.
Hi Mike,

I updated the patch for your comments.

Could you take a look again?

Thanks.
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

https://reviewboard.mozilla.org/r/143144/#review149806

::: commit-message-6eb85:1
(Diff revision 10)
> +Bug 1365847 - Do not search for unselected <xul:deck> items.

From my original review:

"Is UX aware of this choice? This means that, for example, it's not possible to show results for the different states under History under Privacy and Security, for example. Are they okay with this?"

We should make sure that UX is on board with this before landing.

::: browser/components/preferences/in-content-new/findInPage.js:339
(Diff revision 10)
> +    }
> +    return matchesFound;
> +  },
> +
> +  /**
> +   * Search visible child node from a parent node.

This comment (and the name of the function) makes it seem like the child node itself is visible, when it really might not be (which we check in line 351).

Perhaps we should name this `searchChildNodeIfVisible` and update the comment to say:

```js
/**
 * Search for a phrase within a child node if it
 * is visible.
 *
 * ...
 */
```

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_2.js:7
(Diff revision 10)
> +/**
> + * This file contains tests for the Preferences search bar.
> + */
> +
> +/**
> + * Enabling Searching functionatily. Will display search bar form this testcase forward.

Nit: typo "functionatily" -> "functionality"

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_2.js:14
(Diff revision 10)
> +add_task(async function() {
> +  await SpecialPowers.pushPrefEnv({"set": [["browser.preferences.search", true]]});
> +});
> +
> +/**
> + * Test for that only search the selected deck.

Nit: "Test for that only search the selected deck" -> "Test that we only search the selected child of a XUL deck".

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_2.js:19
(Diff revision 10)
> + * Test for that only search the selected deck.
> + * When we search "perform", it should show the "performanceGroup".
> + */
> +add_task(async function() {
> +  await openPreferencesViaOpenPreferencesAPI("paneGeneral", {leaveOpen: true});
> +

Probably a good idea to assert that the Performance group is actually the selectedIndex too before doing the search.

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_2.js:28
(Diff revision 10)
> +  searchInput.value = "perform";
> +  searchInput.doCommand();
> +
> +  let mainPrefTag = gBrowser.contentDocument.getElementById("mainPrefPane");
> +  for (let i = 0; i < mainPrefTag.childElementCount; i++) {
> +    let child = mainPrefTag.children[i]

Nit: missing semi-colon.
Attachment #8871635 - Flags: review?(mconley) → review+
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

https://reviewboard.mozilla.org/r/143144/#review149926

::: commit-message-6eb85:1
(Diff revision 10)
> +Bug 1365847 - Do not search for unselected <xul:deck> items.

Sure, let's get feebacks from UX.

::: browser/components/preferences/in-content-new/findInPage.js:339
(Diff revision 10)
> +    }
> +    return matchesFound;
> +  },
> +
> +  /**
> +   * Search visible child node from a parent node.

Thanks.
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

https://reviewboard.mozilla.org/r/143144/#review149806

> Probably a good idea to assert that the Performance group is actually the selectedIndex too before doing the search.

Sure, let's do it.
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

https://reviewboard.mozilla.org/r/143144/#review149806

> From my original review:
> 
> "Is UX aware of this choice? This means that, for example, it's not possible to show results for the different states under History under Privacy and Security, for example. Are they okay with this?"
> 
> We should make sure that UX is on board with this before landing.

Sure, let's do ux-review and get feedbacks from Tina.
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

Hi Tina,

Could you review this UX change?

The patch add the "searchkeywords" attribute in the "historyMode" menulist element to fix the below issue mentioned by Mike.

Thanks.

> "Is UX aware of this choice? This means that, for example, it's not possible to show results for the different states under History under Privacy and Security, for example. Are they okay with this?"
Attachment #8871635 - Flags: ui-review?(thsieh)
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

Hi Mike and Evan,
Thanks for requesting UX review! Evan shows the patch to me and I think it looks very good.

However, I believe that we still need the search function to help users find the keywords that hide under the dropdown button. Especially for cookies. Our user test results always showed that finding cookie options is the most difficult task for participants. Before we have resouce to implement the History redesign, I believe search is the tool to help users to find it.

Evan, I'm going to r+ this bug. Could you please file another bug for finding the content which is hiding under dropdown buttons?
Attachment #8871635 - Flags: ui-review?(thsieh) → ui-review+
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

https://reviewboard.mozilla.org/r/143144/#review150094

::: browser/components/preferences/in-content-new/findInPage.js:301
(Diff revision 15)
>        let valueResult = this.stringMatchesFilters(nodeObject.getAttribute("value"), searchPhrase);
>  
>        // Searching some elements, such as xul:button, buttons to open subdialogs.
>        let keywordsResult = this.stringMatchesFilters(nodeObject.getAttribute("searchkeywords"), searchPhrase);
>  
> -      // Creating tooltips for buttons
> +      // Creating tooltips for buttons and menulists.

We also need to add tooltips for menulists.

::: browser/components/preferences/in-content-new/findInPage.js:313
(Diff revision 15)
>        }
>  
>        matchesFound = matchesFound || complexTextNodesResult || labelResult || valueResult || keywordsResult;
>      }
>  
> +    // Should not search unselcted child nodes of a <xul:deck> element

Hi Mike,

Tina already ui-review+ the ui changes.

> "Is UX aware of this choice? This means that, for example, it's not possible to show results for the different states under History under Privacy and Security, for example. Are they okay with this?"

The solution is that don't search unselcted child nodes of a <xul:deck> element except the "historyPane" <xul:deck> element. What do you think?
Blocks: 1370495
> Evan, I'm going to r+ this bug. Could you please file another bug for
> finding the content which is hiding under dropdown buttons?

Hi Tina,

Ricky and I filed two bugs to address the issue.

Bug 1370495 - Add search keywords for history section.
Bug 1370491 - Show search tooltips on menuitem for history section.

Once the two bugs are fixed, we could find and highlight the content which is hiding under dropdown buttons.
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

https://reviewboard.mozilla.org/r/143144/#review150100

::: browser/components/preferences/in-content-new/findInPage.js:302
(Diff revision 16)
>  
>        // Searching some elements, such as xul:button, buttons to open subdialogs.
>        let keywordsResult = this.stringMatchesFilters(nodeObject.getAttribute("searchkeywords"), searchPhrase);
>  
> -      // Creating tooltips for buttons
> -      if (keywordsResult && nodeObject.tagName === "button") {
> +      // Creating tooltips for buttons and menulists.
> +      if (keywordsResult && (nodeObject.tagName === "button" || nodeObject.tagName == "menulist")) {

We also need to add highlight tooltips for menulists.

::: browser/components/preferences/in-content-new/findInPage.js:315
(Diff revision 16)
>        matchesFound = matchesFound || complexTextNodesResult || labelResult || valueResult || keywordsResult;
>      }
>  
> +    // Should not search unselected child nodes of a <xul:deck> element
> +    // except the "historyPane" <xul:deck> element.
> +    if (nodeObject.tagName == "deck" && nodeObject.id != "historyPane") {

Hi Mike,

Tina already ui-review+ the ui changes.

> "Is UX aware of this choice? This means that, for example, it's not possible to show results for the different states under History under Privacy and Security, for example. Are they okay with this?"

The solution is that don't search unselcted child nodes of a <xul:deck> element except the "historyPane" <xul:deck> element. What do you think?
Hi Mike,

I updated patch for searching the content of the history section. And we filed two follow-up bugs to fix the issue mentioned by Tina at Comment 27 (for finding the content which is hiding under dropdown buttons).

Could you take a look[1]? What the patch doing is not searching unselected child nodes of a <xul:deck> element except the "historyPane" <xul:deck> element.

If the patch is OK to you, let's land the patch. Thank you.

[1]: https://reviewboard.mozilla.org/r/143144/diff/15#index_header
Flags: needinfo?(mconley)
No longer blocks: 1370491
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

https://reviewboard.mozilla.org/r/143144/#review152024

Code looks great, so r=me again. I have some documentation tweaks to suggest though.

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_1.js:22
(Diff revisions 10 - 16)
>    await BrowserTestUtils.removeTab(gBrowser.selectedTab);
>    await SpecialPowers.popPrefEnv();
>  });
>  
>  /**
> - * Enabling Searching functionatily. Will display search bar form this testcase forward.
> + * Enabling Searching functionality. Will display search bar form this testcase forward.

One last typo: "form" -> "from".

Also, maybe "Searching" should have the "S" lowercased.

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_2.js:7
(Diff revisions 10 - 16)
>  /**
>   * This file contains tests for the Preferences search bar.
>   */
>  
>  /**
> - * Enabling Searching functionatily. Will display search bar form this testcase forward.
> + * Enabling Searching functionality. Will display search bar form this testcase forward.

Same suggestions as above.

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_2.js:42
(Diff revisions 10 - 16)
>      } else if (child.id) {
>        is_element_hidden(child, "Should not be in search results");
>      }
>    }
>  
> +  // Performs search.

Maybe this should say that this string exists in a hidden child of the deck, so we expect no search results. It might also be worth-while to ensure that this text actually exists in the hidden child of the deck, so that if it ever changes, we don't open ourselves up to missing regressions.

::: browser/components/preferences/in-content-new/tests/browser_search_within_preferences_2.js:48
(Diff revisions 10 - 16)
> +  searchInput.focus();
> +  searchInput.value = "is currently your default browser";
> +  searchInput.doCommand();
> +
> +  let noResultsEl = gBrowser.contentDocument.querySelector(".no-results-message");
> +  is_element_visible(noResultsEl, "Should be in search results");

"Should be in search results", I think this is a special case... perhaps this should be:

"Should be reporting no results".
Flags: needinfo?(mconley)
Comment on attachment 8871635 [details]
Bug 1365847 - Do not search for unselected <xul:deck> items.

https://reviewboard.mozilla.org/r/143144/#review152024

> Maybe this should say that this string exists in a hidden child of the deck, so we expect no search results. It might also be worth-while to ensure that this text actually exists in the hidden child of the deck, so that if it ever changes, we don't open ourselves up to missing regressions.

Sure, let's do it.
Updated patch and tests for review comment. Let's land the patch once the try[1] is good.

Thank you for reviewing, Mike.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8214f69e1460
The try is good. Let's land the patch.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d6fadd1aa4fc
Do not search for unselected <xul:deck> items. r=mconley
Keywords: checkin-needed
I think I fix the test failure. Let's wait for the try result[1].

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae4e64fab04c
Flags: needinfo?(evan)
The try[1] is OK on Windows 8 and other platforms now. Let's land the patch.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=532667cd5cac&selectedJob=107077816
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cfb7ce2c6942
Do not search for unselected <xul:deck> items. r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cfb7ce2c6942
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Build ID: 20170615030208
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Verified as fixed on Firefox Nightly 56.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.