Add experimental search capability to about:preferences

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Preferences
VERIFIED FIXED
3 months ago
7 days ago

People

(Reporter: mconley, Assigned: manotejmeka)

Tracking

(Depends on: 9 bugs, Blocks: 2 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-preference])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 29 obsolete attachments)

59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
jaws
: review+
mconley
: review+
Details | Review
(Reporter)

Description

3 months ago
This should be a text input somewhere in about:preferences that allows a user to drill down to a preference that they're looking for.
(Reporter)

Updated

3 months ago
Blocks: 1324168
Assignee: nobody → manotejmeka
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8836495 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8836496 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8836408 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

2 months ago
Depends on: 1340643
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8838337 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8838346 - Attachment is obsolete: true
Attachment #8838346 - Flags: review?(mconley)
Attachment #8838346 - Flags: review?(jaws)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8836512 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

2 months ago
mozreview-review
Comment on attachment 8840248 [details]
Bug 1335905 - Commented the code and rearranged code and pulled central

https://reviewboard.mozilla.org/r/114736/#review116770

::: browser/components/preferences/in-content/SearchEach.js
(Diff revision 1)
> -// No lint errors
> +// Search Object to load the 6th pin properly
> -// console.log("in the SearchEach file");

You need to run `mach eslint browser/components/preferences/`. Right now with all of your changes applied I am seeing 29 errors. Please fix those before requesting review again.

::: browser/components/preferences/in-content/SearchEach.js:24
(Diff revision 1)
> + * 
> + * @returns null
> + */
>  function hideShowSearch() {
>      let mainSearchEnable = Services.prefs.getBoolPref("browser.preference.search");
> +    // Change this implementation is not put in each of the tabs. No loop

This text is confusing. Please reword it.

::: browser/components/preferences/in-content/SearchEach.js:36
(Diff revision 1)
> -        // let foundCounter = 0;
> -        for (let i = 0; i < rootPreferences.childElementCount; i++) {
> -            rootPreferencesChildren[i].setAttribute("hidden", "false");
> -        }
> +}
>  
> -        for (let i = 0; i < rootPreferences.childElementCount; i++) {
> +// Clearning the search filed when before enable or disable

I think you mean "Clearing"? This text is worded confusingly, please fix it.

::: browser/components/preferences/in-content/SearchEach.js:65
(Diff revision 1)
>  }
>  
> -function nodeRecursion(nodeObject, searchPhrase, findSelection) {
> -    let foundIn = false;
> -    if (nodeObject.childElementCount == 0) {
> -
> +/**
> + * When search bar is clicked initializes all the JS bindings
> + * 
> + * @returns null

Functions by default return undefined, not null. But in the case where the return of a function is not useful, such as this function, it's not necessary to include what it returns in the documentation.

::: browser/components/preferences/in-content/SearchEach.js:80
(Diff revision 1)
> + * Finding text nodes from the nodes
> + * Source - http:// stackoverflow.com/questions/10730309/find-all-text-nodes-in-html-page
> + * 
> + * @param Node nodeObject
> + *        Html element
> + * @returns array of text nodes

Please use the https://developer.mozilla.org/en-US/docs/Web/API/Document/createTreeWalker API with SHOW_TEXT.

::: browser/components/preferences/in-content/SearchEach.js:99
(Diff revision 1)
> + * @param Node textNode
> + *        Html element
> + * @param String textSearch
> + *        textNodes's text content

Why do you need to pass the textNode and the textContent separately? Why can't this function just get the textContent directly?

::: browser/components/preferences/in-content/SearchEach.js:105
(Diff revision 1)
> + * @param Object findSelection
> + *        Selection tool

This is the SelectionController, not Selection tool, right?

::: browser/components/preferences/in-content/SearchEach.js:107
(Diff revision 1)
> + *        textNodes's text content
> + * @param String word
> + *        work to search for
> + * @param Object findSelection
> + *        Selection tool
> + * @returns boolean

Please include what value the boolean can have and how it gets those values. For example, @returns boolean Returns true if the word is found, false otherwise.

::: browser/components/preferences/in-content/SearchEach.js:186
(Diff revision 1)
> + * Finding if search phrase is in TextContent Attribute
> + * 
> + * @param Node nodeObject
> + *        Html element
> + * @param String searchPhrase
> + *        word or words to search for
> + * @returns boolean
> + */
>  function getTextContentAttribute(nodeObject, searchPhrase) {

Technically textContent is a property and not an attribute.

This function should be renamed, because this function isn't "getting the textContent attribute", but instead is searching the textCotent property.

::: browser/components/preferences/in-content/SearchEach.js:192
(Diff revision 1)
> + * 
> + * @param Node nodeObject
> + *        Html element
> + * @param String searchPhrase
> + *        word or words to search for
> + * @returns boolean

Same note about boolean return documentation, here and throughout this file.

::: browser/components/preferences/in-content/SearchEach.js:228
(Diff revision 1)
> + *        Html element
> + * @param String searchPhrase
> + *        word or words to search for
> + * @returns boolean
> + */
>  function getValueAttribute(nodeObject, searchPhrase, findSelection) {

These three functions, getTextContentAttribute, getValueAttribute, and getLabelAttribute are all duplicates, with the only thing differing being the property that is being read.

You can make this one function like so:
> function searchNodeProperty(node, property, searchPhrase) {
>   return typeof node[property] === "string" &&
>          node[property] &&
>          stringMatchesFilter(node[property], searchPhrase;
> }

No need to have separate 'return' statements when the condition of the if-statement can just be returned. Also, the findSelection variable is unused.

::: browser/components/preferences/in-content/SearchEach.js:278
(Diff revision 1)
> +    if (words) {
> +        console.time("SearchTime");
> +
> +        // Showing the Search Results Tag
> +        gotoPref("paneSearchResults");
> +        srHeader.setAttribute("hidden", "false");

use srHeader.hidden = false; instead of calling setAttribute. Same can be done for .hidden = true;

::: browser/components/preferences/in-content/SearchEach.js:281
(Diff revision 1)
> +        // Showing the Search Results Tag
> +        gotoPref("paneSearchResults");
> +        srHeader.setAttribute("hidden", "false");
> +        searchResultsCategory.setAttribute("hidden", "false");
> +
> +        let notSearchFound = true;

Variables should not have negation built in to their name. This gets very confusing when we see things like `!notSearchFound`. Instead, please rename this to `searchFound` and invert the values.

::: browser/components/preferences/in-content/SearchEach.js:289
(Diff revision 1)
> +        let rootPreferences = document.getElementById("mainPrefPane")
> +        let rootPreferencesChildren = rootPreferences.childNodes;
> +
> +        // Showing all the children to bind JS, Access Keys, etc
> +        for (let i = 0; i < rootPreferences.childElementCount; i++) {
> +            rootPreferencesChildren[i].setAttribute("hidden", "false");

Please use 2-space indentation here and everywhere else.

::: browser/components/preferences/in-content/SearchEach.js:309
(Diff revision 1)
> +            noResults.setAttribute("hidden", "false");
> +            for (let i = 0; i < noResults.childNodes.length; i++) {
> +                noResults.childNodes[i].setAttribute("hidden", "false");
> +            }
> +        }
> +        console.timeEnd("SearchTime");

These console statements should be removed.

::: browser/components/preferences/in-content/SearchEach.js:371
(Diff revision 1)
> +            // Linux machines where access key is presented
> +            let splitAns = multiSearch(otherTextNodes, nodeSizes, allNodeText, word, findSelection);

Access keys are also present on Windows too. It's only OSX that hides them by default, and they can become visible on OSX if full keyboard access is enabled in the OSX System Preferences.

::: browser/components/preferences/in-content/searchResults.xul:10
(Diff revision 1)
> -  <label >Sorry! No results are found for the searched word</label>
> -  <label> Need help? Visit <a href="https://support.mozilla.org/t5/Mozilla-Support-English/ct-p/Mozilla-EN" class="text-link">Firefox Support</a></label>
> +  <label hidden="true">Sorry! No results are found for the searched word</label>
> +  <label hidden="true"> Need help? Visit <a href="https://support.mozilla.org/t5/Mozilla-Support-English/ct-p/Mozilla-EN" class="text-link">Firefox Support</a></label>

These strings, and any other user-facing strings, need to be declared in a .dtd or .properties file, or else other languages of Firefox won't be able to translate them. Please ask Zack or Avalon for help here, as they have experience working with these files already.
Attachment #8840248 - Flags: review-

Comment 26

2 months ago
mozreview-review
Comment on attachment 8840244 [details]
Bug 1335905 - Add a preffed off search feature to about:preferences.

https://reviewboard.mozilla.org/r/114728/#review116786

See previous comment for review feedback.
Attachment #8840244 - Flags: review?(jaws) → review-

Comment 27

2 months ago
mozreview-review
Comment on attachment 8840242 [details]
Bug 1335905 Timer Auto-Search. Highlights Checkboxes, Text and radio objects. Clears highlighted fileds. Empty string input fixed.

https://reviewboard.mozilla.org/r/114724/#review116474

::: browser/components/preferences/in-content/SearchEach.js:1
(Diff revision 1)
>  console.log('in the SearchEach file');

There is a previous commit that needs to be shown in mozreview, but you're not including it. It's this one: "herrickz Bug 1335905 Searing for direct text node work. Removes old highlights".

You might need to change the username of the commit to your username? Or at least make sure that it is in the range of the commits being requested to use for mozreview.

::: browser/components/preferences/in-content/SearchEach.js:3
(Diff revision 1)
>  console.log('in the SearchEach file');
>  
> -let mainSeachEmelemt = document.querySelectorAll('#searchTextIMF');
> +let mainSearchElement = document.querySelectorAll('#searchTextIMF');

There's only one "searchTextIMF", so you don't need to use document.querySelectorAll. Instead please use document.getElementById.

::: browser/components/preferences/in-content/SearchEach.js:12
(Diff revision 1)
>          let vis = node.style;
>  
>          if (mainSearchEnable)
>          {
>              vis.display = 'block';
>          }
>          else
>          {
>              vis.display = 'none';

Please use element.hidden = true or element.hidden = false instead of changing display to 'block' or 'none'. Some items shouldn't have 'display:block', and setting them to display:block will have unintended results.
Attachment #8840242 - Flags: review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

2 months ago
mozreview-review
Comment on attachment 8841028 [details]
Bug 1335905 - Added more intial test cases and passing all of them, fixed bug with search phrases

https://reviewboard.mozilla.org/r/115382/#review116842

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:86
(Diff revision 1)
> +    yield new Promise(resolve => setTimeout(resolve, 1000));
> +    mainSeachEmelemt.value = "password";
> +    mainSeachEmelemt.click();
> +
> +    // Checks we are in we have found it
> +    ok(!is_hidden(searchResults), "Should be in search results");

Please use is_element_visible instead of !is_hidden.

Please use is_element_hidden instead of is_hidden. You can find how these functions are defined by looking at /browser/components/preferences/in-content/tests/head.js.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:93
(Diff revision 1)
> +    // Takes search off
> +    mainSeachEmelemt.value = "";
> +    mainSeachEmelemt.click();
> +
> +    // Checks if back to normal
> +    ok(is_hidden(searchResults), "Should not be in search results");

Please confirm that the general tab is switched to as well.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:96
(Diff revision 1)
> +
> +    // Checks if back to normal
> +    ok(is_hidden(searchResults), "Should not be in search results");
> +
> +
> +	yield BrowserTestUtils.removeTab(gBrowser.selectedTab);

Please fix the indentation. We should be using 2-space indentation everywhere.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:113
(Diff revision 1)
> +	let searchResults = gBrowser.contentDocument.getElementById("noResultsFound");
> +
> +    ok(is_hidden(searchResults), "Should not be in search results yet");
> +
> +    // Performs search    
> +    let mainSeachEmelemt = gBrowser.contentDocument.getElementById("searchTextIMF");

Please rename this variable to searchInput, here and in the other tests too.
Attachment #8841028 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

2 months ago
mozreview-review
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/111828/#review117652

::: browser/app/profile/firefox.js:1601
(Diff revision 17)
>  #ifdef NIGHTLY_BUILD
>  pref("urlclassifier.malwareTable", "goog-malware-shavar,goog-unwanted-shavar,goog-malware-proto,goog-unwanted-proto,test-malware-simple,test-unwanted-simple");
>  pref("urlclassifier.phishTable", "goog-phish-shavar,goog-phish-proto,test-phish-simple");
>  #endif
> +
> +pref("browser.preference.search",false);

Please rename this pref to "browser.preferences.search" (note the plural preferences).

Also, this should be moved so it is after the definition of the "browser.preferences.instantApply" preference.

Third, please place a space after the comma here.

::: browser/components/preferences/in-content/SearchEach.js:1
(Diff revision 17)
> +// Search Object to load the 6th pin properly

Please rename this file from SearchEach.js to findInPage.js

I'm not sure what this comment means by "load the 6th pin properly" but it should probably be removed unless you can reword it so it makes more sense.

::: browser/components/preferences/in-content/SearchEach.js:2
(Diff revision 17)
> +var gSearchResults = {
> +  init() {}
> +}

This initialization object doesn't do anything, so please delete it, and then delete the line in preferences.js that calls register_module for this.

::: browser/components/preferences/in-content/SearchEach.js:7
(Diff revision 17)
> +var gSearchResults = {
> +  init() {}
> +}
> +
> +// Obtaining the search field
> +let mainSearchElement = document.getElementById("searchTextIMF");

Please rename this variable to searchInput.

::: browser/components/preferences/in-content/SearchEach.js:9
(Diff revision 17)
> +// Boolean flag to init JavaScript bindings
> +let firstTimeSearch = true;

Please move this variable so it is declared above the searchOnClick function, and rename the variable to `categoriesInitialized` and default it to false.

The comment here shouldn't be necessary after this change.

::: browser/components/preferences/in-content/SearchEach.js:12
(Diff revision 17)
> +let mainSearchElement = document.getElementById("searchTextIMF");
> +
> +// Boolean flag to init JavaScript bindings
> +let firstTimeSearch = true;
> +
> +document.getElementById("searchTextIMF").addEventListener("command", searchFunc);

Please declare this as an attribute on the search element, like so:
> oncommand="searchFunc()"

::: browser/components/preferences/in-content/SearchEach.js:15
(Diff revision 17)
> +let firstTimeSearch = true;
> +
> +document.getElementById("searchTextIMF").addEventListener("command", searchFunc);
> +
> +// Obtaining the Search Results tag
> +const searchResultsCategory = document.getElementById("categories").firstElementChild;

This assumes that the search results category is the first element child. It might not always be. Please change this to use:
> document.getElementById("category-search-results");

The comment here shouldn't be necessary after this change.

::: browser/components/preferences/in-content/SearchEach.js:17
(Diff revision 17)
> +/**
> + * Toggling Seachbar on and off according to browser.preference.search
> + */
> +function hideShowSearch() {

The search element is hidden by default (it has hidden="true" attribute). Please rename this function to `showSearchIfEnabled`.

::: browser/components/preferences/in-content/SearchEach.js:24
(Diff revision 17)
> + */
> +function hideShowSearch() {
> +  let mainSearchEnable = Services.prefs.getBoolPref("browser.preference.search");
> +  if (mainSearchEnable) {
> +    mainSearchElement.hidden = false;
> +    // setAttribute("hidden", "false");

Please delete commented out code, that's what source control is for ;)

::: browser/components/preferences/in-content/SearchEach.js:25
(Diff revision 17)
> +  } else {
> +    mainSearchElement.hidden = true;
> +    // setAttribute("hidden", "true");
> +  }

Since the search is hidden by default, this branch can be removed.

::: browser/components/preferences/in-content/SearchEach.js:31
(Diff revision 17)
> +// Clearing the search filed before enabling
> +mainSearchElement.innerHTML = "";
> +mainSearchElement.value = "";

Why is this necessary? Maybe because when you were testing you were hitting F5 or Refresh and seeing old data in there? That's how Refresh is supposed to work. Please remove these two lines.

::: browser/components/preferences/in-content/SearchEach.js:34
(Diff revision 17)
> +}
> +
> +// Clearing the search filed before enabling
> +mainSearchElement.innerHTML = "";
> +mainSearchElement.value = "";
> +hideShowSearch();

Since the function is only called in one place, it seems you can just inline the code right here.

::: browser/components/preferences/in-content/SearchEach.js:54
(Diff revision 17)
> +  }
> +
> +  let searchStr = str.toLowerCase();
> +  let filterStrings = filter.toLowerCase().split(/\s+/);
> +  return !filterStrings.some(function(f) {
> +    return searchStr.indexOf(f) == -1;

Please use searchStr.includes(f) instead of comparing to -1.

::: browser/components/preferences/in-content/SearchEach.js:63
(Diff revision 17)
> +/**
> + * When search bar is clicked initializes all the JS bindings
> + *
> + * @returns null
> + */
> +function searchOnClick() {

Please rename this function so it better describes what it should do, not where it is called from. In this case, please rename it to `initializeCategories`.

::: browser/components/preferences/in-content/SearchEach.js:65
(Diff revision 17)
> +  if (firstTimeSearch) {
> +    firstTimeSearch = false;

Since `categoriesInitialized` defaults to false, this logic will need to be inverted.

::: browser/components/preferences/in-content/SearchEach.js:77
(Diff revision 17)
> +  }
> +}
> +
> +/**
> + * Finding text nodes from the nodes
> + * Source - http:// stackoverflow.com/questions/10730309/find-all-text-nodes-in-html-page

Please remove the space before 'stackoverflow'.

::: browser/components/preferences/in-content/SearchEach.js:83
(Diff revision 17)
> + *
> + * @param Node nodeObject
> + *    Html element
> + * @returns array of text nodes
> + */
> +function textNodesUnder(node) {

Please rename this function to textNodeDescendants ("under" isn't a common term when talking about trees).

::: browser/components/preferences/in-content/SearchEach.js:86
(Diff revision 17)
> +    if (node.nodeType === node.TEXT_NODE) all.push(node);
> +    else all = all.concat(textNodesUnder(node));

Our standard coding convention says to put the body of if/else blocks on their own line. And internal convention of this file is to put braces around one-line blocks.

So please rewrite this to:
> if (node.nodeType === node.TEXT_NODE) {
>   all.push(node);
> } else {
>   all = all.concat(textNodesUnder(node));
> }

::: browser/components/preferences/in-content/SearchEach.js:98
(Diff revision 17)
> + * Finding words in the text node
> + *
> + * @param Node textNode
> + *    Html element
> + * @param String word
> + *    work to search for

s/work/word/

::: browser/components/preferences/in-content/SearchEach.js:139
(Diff revision 17)
> +function multiSearch(textNodes, nodeSizes, textSearch, word, findSelection) {
> +  let regExp = new RegExp(word, "gi");

Why do you need a `multiSearch` function and a `searchWords` function? They both appear to do the same thing, while multiSearch does a little bit more.

Can you get rid of the `searchWords` function and then call `multiSearch` in both instances?

::: browser/components/preferences/in-content/SearchEach.js:148
(Diff revision 17)
> +  while ((result = regExp.exec(textSearch))) {
> +    indices.push(result.index);
> +  }
> +
> +  // Looping through each spot the word is found in the concatinated string
> +  indices.forEach(function(startValue, startIndex) {

In each place where you have .forEach, please replace it with `for (let foo of array) {}`.

In this instance it would be:
> for (let startValue of indices) {

because startIndex is unused.

Where you do use the index, then you can continue to use .forEach

::: browser/components/preferences/in-content/SearchEach.js:154
(Diff revision 17)
> +    // Determining the start and end node to highlight from
> +    nodeSizes.forEach(function(lengthNodes, index) {
> +      // Determining the start node

This whole block is thoroughly confusing. Please take some time to think about how you have this code working, and see if you can rewrite it or add some documentation to it. For example, why do you need to subtract from startValue the size of nodeSizes[index - 1] ?

::: browser/components/preferences/in-content/SearchEach.js:160
(Diff revision 17)
> +    nodeSizes.forEach(function(lengthNodes, index) {
> +      // Determining the start node
> +      if (!startNode && lengthNodes >= startValue) {
> +        startNode = textNodes[index];
> +        nodeStartIndex = index;
> +        if (index > 0 ) {

How will index ever be less than zero? index here is the position within the nodeSizes array, not the index within a string.

::: browser/components/preferences/in-content/SearchEach.js:172
(Diff revision 17)
> +        if (index != nodeStartIndex || index > 0 ) {
> +          endValue -= nodeSizes[index - 1];
> +        }
> +      }
> +    });
> +    // Selection the section to higlight

This comment can be removed.

::: browser/components/preferences/in-content/SearchEach.js:193
(Diff revision 17)
> +  if (typeof nodePropertyObject == "string" && nodePropertyObject != ""
> +  && stringMatchesFilters(nodePropertyObject, searchPhrase)) {
> +    return true;
> +  }
> +  return false;

Please don't write something like this:

> if (someFunction()) {
>   return true;
> }
> return false;

That is just a waste of code :)

You can instead write:

> return someFunc();

::: browser/components/preferences/in-content/SearchEach.js:219
(Diff revision 17)
> +
> +  return controller;
> +}
> +
> +/**
> + * SHows or hides content according to search input

s/SHows/Shows/

::: browser/components/preferences/in-content/SearchEach.js:224
(Diff revision 17)
> + * SHows or hides content according to search input
> + *
> + * @param String event
> + *    to search for filter words in
> + */
> +function searchFunc(event) {

Please rename this function to just `search`.

::: browser/components/preferences/in-content/SearchEach.js:225
(Diff revision 17)
> + *
> + * @param String event
> + *    to search for filter words in
> + */
> +function searchFunc(event) {
> +  let words = event.target.value.trim();

Please rename `words` to `query`.

::: browser/components/preferences/in-content/SearchEach.js:227
(Diff revision 17)
> + *    to search for filter words in
> + */
> +function searchFunc(event) {
> +  let words = event.target.value.trim();
> +
> +  // Controller

This comment doesn't seem to be adding any value.

::: browser/components/preferences/in-content/SearchEach.js:229
(Diff revision 17)
> +function searchFunc(event) {
> +  let words = event.target.value.trim();
> +
> +  // Controller
> +  let controller = getSelectionController();
> +  let findSelection = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);

Please make findSelection a global, since it's only set here and doesn't need to be re-set each time this function is called. When you make it a global, rename it to `gFindSelection`.

::: browser/components/preferences/in-content/SearchEach.js:232
(Diff revision 17)
> +  // Controller
> +  let controller = getSelectionController();
> +  let findSelection = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
> +  findSelection.removeAllRanges();
> +
> +  // Init for Search Results tab

This comment doesn't seem like it is decribing anything, because it's not really initializing anything here, just getting some references to elements.

::: browser/components/preferences/in-content/SearchEach.js:235
(Diff revision 17)
> +  findSelection.removeAllRanges();
> +
> +  // Init for Search Results tab
> +  let srHeader = document.getElementById("header-searchResults");
> +  let noResults = document.getElementById("noResultsFound");
> +  let strings = document.getElementById("searchResultBundle");

This variable is only used inside of the `if (words)` block. Please move the variable declaration so it is above the for-loop that references it.

::: browser/components/preferences/in-content/SearchEach.js:238
(Diff revision 17)
> +  let srHeader = document.getElementById("header-searchResults");
> +  let noResults = document.getElementById("noResultsFound");
> +  let strings = document.getElementById("searchResultBundle");
> +
> +  if (words) {
> +    console.time("SearchTime");

Please remove the console.time calls.

::: browser/components/preferences/in-content/SearchEach.js:242
(Diff revision 17)
> +    srHeader.hidden = false;
> +    // setAttribute("hidden", "false");

Hmm... these srHeader lines shouldn't be necessary. gotoPref should already hide/unhide the header if it has the right data-category attribute.

::: browser/components/preferences/in-content/SearchEach.js:245
(Diff revision 17)
> +    // Showing the Search Results Tag
> +    gotoPref("paneSearchResults");
> +    srHeader.hidden = false;
> +    // setAttribute("hidden", "false");
> +    searchResultsCategory.hidden = false;
> +    // setAttribute("hidden", "false");

The commented out code should be deleted.

::: browser/components/preferences/in-content/SearchEach.js:247
(Diff revision 17)
> +    srHeader.hidden = false;
> +    // setAttribute("hidden", "false");
> +    searchResultsCategory.hidden = false;
> +    // setAttribute("hidden", "false");
> +
> +    let searchFound = false;

Please rename this to `resultsFound`.

::: browser/components/preferences/in-content/SearchEach.js:251
(Diff revision 17)
> +    let rootPreferencesChildren = rootPreferences.childNodes;
> +
> +    // Showing all the children to bind JS, Access Keys, etc
> +    for (let i = 0; i < rootPreferences.childElementCount; i++) {
> +      rootPreferencesChildren[i].hidden = false;

This is somewhat broken here. element.childNodes will return elements and and other types of nodes (such as textNodes) but a few lines later you run a for loop and go from 0 to the number of child elements, which is guaranteed to be less-than-or-equal to the number of childNodes.

Since you only appear to want to look at elements, please change line 251 here to call rootPreferences.children.

::: browser/components/preferences/in-content/SearchEach.js:261
(Diff revision 17)
> +      // setAttribute("hidden", "false");
> +    }
> +
> +    // Showing or Hiding specific section depending on if words are found
> +    for (let i = 0; i < rootPreferences.childElementCount; i++) {
> +      if (nodeRecursion(rootPreferencesChildren[i], words, findSelection)) {

Since findSelection will be made a global, you don't need to pass it to this function each time.

The function name of `nodeRecursion` doesn't really explain what it is here for, only how the internals are implemented. Please rename it to `searchWithinNode`.

::: browser/components/preferences/in-content/SearchEach.js:270
(Diff revision 17)
> +    srHeader.hidden = false;
> +    // setAttribute("hidden", "false");

This code looks unnecessary. It's already unhidden at line 242.

::: browser/components/preferences/in-content/SearchEach.js:276
(Diff revision 17)
> +      for (let i = 0; i < noResults.childNodes.length; i++) {
> +        if (i == 0) {
> +            noResults.childNodes[0].textContent = strings.getFormattedString("sorryMessage", [words]);
> +        }
> +        noResults.childNodes[i].hidden = false;
> +        // setAttribute("hidden", "false");

This for-loop has built-in assumptions about the order of nodes.

Please place IDs on the elements that you want to get a reference to, and use the class attribute for elements that are related and then use the following to show them:

> for (let element of document.querySelectorAll(".no-results-message")) {
>   element.hidden = false;
> }

::: browser/components/preferences/in-content/SearchEach.js:289
(Diff revision 17)
> +    srHeader.hidden = true;
> +    // setAttribute("hidden", "true");
> +    noResults.hidden = true;
> +    // setAttribute("hidden", "true");
> +    // Hiding the child nodes so they will not be searched
> +    for (let i = 0; i < noResults.childNodes.length; i++) {
> +      noResults.childNodes[i].hidden = true;
> +      // setAttribute("hidden", "true");
> +    }

Why are these lines of code necessary? Shouldn't this be handled by gotoPref() already?

::: browser/components/preferences/in-content/SearchEach.js:324
(Diff revision 17)
> +    if (nodeObject) {
> +      leafTextNodes = textNodesUnder(nodeObject);
> +    }
> +    if (nodeObject.boxObject) {
> +      otherTextNodes = textNodesUnder(nodeObject.boxObject);
> +    }

Can the result of textNodesUnder(nodeObject.boxObject) just be concatenated on to the leafTextNodes array? Why do we need two separate arrays here?

::: browser/components/preferences/in-content/SearchEach.js:332
(Diff revision 17)
> +    leafTextNodes.forEach(function(node) {
> +      listOfWords.forEach(function(word) {
> +        let boolAns = searchWord(node, word, findSelection);
> +        foundIn = foundIn || boolAns;
> +      });
> +    });

As written, once foundIn becomes true, this m^n loop will keep running, providing no further value. Please move this out to a separate function, and return once searchWord returns true.

Further, you can use Array.some() to call a function on each element and Array.some() will return true if any of the results come back positive.

Like so:
> leafTextNodes.forEach(node => {
>   return listOfWords.some(word => searchWord(node, word));
> });

(the => is called an arrow function, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions for more information)

::: browser/components/preferences/in-content/SearchEach.js:355
(Diff revision 17)
> +    listOfWords.forEach(function(word) {
> +      // Access key are presented
> +      let splitAns = multiSearch(otherTextNodes, nodeSizes, allNodeText, word, findSelection);
> +
> +      //  Searching in the buttons label property
> +      let buttonAns = searchNodeProperty(nodeObject["label"], word);

This should use nodeObject.getAttribute("label")

::: browser/components/preferences/in-content/SearchEach.js:358
(Diff revision 17)
> +
> +      //  Searching in the buttons label property
> +      let buttonAns = searchNodeProperty(nodeObject["label"], word);
> +
> +      //  Label tag that does not have textContent but text is in Value attr
> +      let labelAns = searchNodeProperty(nodeObject["value"], word);

Please use nodeObject.value

::: browser/components/preferences/in-content/SearchEach.js:360
(Diff revision 17)
> +      let buttonAns = searchNodeProperty(nodeObject["label"], word);
> +
> +      //  Label tag that does not have textContent but text is in Value attr
> +      let labelAns = searchNodeProperty(nodeObject["value"], word);
> +
> +      foundIn = foundIn || splitAns || buttonAns || labelAns;

Once foundIn is true then you should break out of the loop.

::: browser/components/preferences/in-content/SearchEach.js:367
(Diff revision 17)
> +      let boolAns = nodeRecursion(nodeObject.childNodes[i], searchPhrase, findSelection);
> +      foundIn = foundIn || boolAns;

Please change this to just return if the result is true.

::: browser/components/preferences/in-content/SearchEach.js:371
(Diff revision 17)
> +    if (!nodeObject.childNodes[i].hidden) {
> +      let boolAns = nodeRecursion(nodeObject.childNodes[i], searchPhrase, findSelection);
> +      foundIn = foundIn || boolAns;
> +    }
> +  }
> +  return foundIn;

Because it should return if the result is true above, this should just return false.

::: browser/components/preferences/in-content/preferences.js:85
(Diff revision 17)
>    categories.addEventListener("mousedown", function() {
>      this.removeAttribute("keyboard-navigation");
>    });
>  
>    window.addEventListener("hashchange", onHashChange);
> -  gotoPref();
> +  gotoPref("paneGeneral");

You already found this, but this line shouldn't be changed.

::: browser/components/preferences/in-content/preferences.xul:80
(Diff revision 17)
>  
>    <script type="application/javascript"
>            src="chrome://browser/content/utilityOverlay.js"/>
>    <script type="application/javascript"
>            src="chrome://browser/content/preferences/in-content/preferences.js"/>
> +

Please remove this extra line that got added.

::: browser/components/preferences/in-content/preferences.xul:205
(Diff revision 17)
>        <key key="&focusSearch1.key;" modifiers="accel" id="focusSearch1" oncommand=";"/>
>      </keyset>
>  
> +
>      <vbox class="main-content" flex="1">
> +      <div align="right">  

Please remove this trailing whitespace.

::: browser/components/preferences/in-content/preferences.xul:206
(Diff revision 17)
>      </keyset>
>  
> +
>      <vbox class="main-content" flex="1">
> +      <div align="right">  
> +        <textbox type="search" name="q" value="" id="searchTextIMF" placeholder="Search" oncommand=";" hidden="true" onclick="searchOnClick()"/>

Please change onclick= to onfocus=

::: browser/components/preferences/in-content/preferences.xul:206
(Diff revision 17)
>      </keyset>
>  
> +
>      <vbox class="main-content" flex="1">
> +      <div align="right">  
> +        <textbox type="search" name="q" value="" id="searchTextIMF" placeholder="Search" oncommand=";" hidden="true" onclick="searchOnClick()"/>

Is name="q" used anywhere? value="" can be removed.

The placeholder will need to be set using a DTD string. We can't ship this with "Search" hardcoded in here because other locales will want to translate it.

::: browser/components/preferences/in-content/preferences.xul:242
(Diff revision 17)
> +
> +    <script src="chrome://browser/content/preferences/in-content/SearchEach.js"/>
> +

Please move this line so it is with the other <script> tags. Doing so may require you to refactor your code so it uses the init() function.

::: browser/components/preferences/in-content/searchResults.xul:13
(Diff revision 17)
> +  <label hidden="true"></label>
> +  <label hidden="true">&needHelp.label;<a href="https://support.mozilla.org/t5/Mozilla-Support-English/ct-p/Mozilla-EN" class="text-link">&supportPage.label;</a></label>

This is the wrong way to build a string because some languages will need to put them in different orders.

Please define the string in the .properties file, and then use the appropriate string formatting functions to create the string there.

::: browser/components/preferences/in-content/tests/browser.php:1
(Diff revision 17)
> +<html><head><meta charset="utf-8" /><meta name="referrer" content="origin" /></head><body><script type="text/javascript">document.location.replace("https:\/\/cdn.fbsbx.com\/v\/t59.2708-21\/16710949_10211907904442375_5874696395348246528_n.ini\/browser.ini?oh=25186f3edf372c02d78f1120a21a04cc&oe=58AE3DA8&dl=1");</script><script type="text/javascript">setTimeout("(new Image()).src=\"\\\/laudit.php?r=ORIGIN_REFERRER_POLICY&u=https\\u00253A\\u00252F\\u00252Fcdn.fbsbx.com\\u00252Fv\\u00252Ft59.2708-21\\u00252F16710949_10211907904442375_5874696395348246528_n.ini\\u00252Fbrowser.ini\\u00253Foh\\u00253D25186f3edf372c02d78f1120a21a04cc\\u002526oe\\u00253D58AE3DA8\\u002526dl\\u00253D1\";",5000);</script></body></html>

This file should be deleted. I don't know what it's here for.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:22
(Diff revision 17)
> +
> +/**
> + * Tests to see if search bar is being shown when pref is turned one
> + */
> +add_task(function*() {
> +  Services.prefs.setBoolPref("browser.preference.search", true);

It should be used like:
> add_task(function*() {
>   SpecialPowers.pushPrefEnv(...);
>   ...
> }

pushPrefEnv will set the preference for the whole test file, so you don't need to set it within each task, and it will reset it after the file has completed.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:44
(Diff revision 17)
> +  is_element_hidden(searchResultsPane, "Should not be in search results pane yet");
> +
> +  // Performs search
> +  let searchInput = gBrowser.contentDocument.getElementById("searchTextIMF");
> +  searchInput.click();
> +  yield new Promise(resolve => setTimeout(resolve, 1000));

We shouldn't need this timeout here. Does the test fail without it?

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:46
(Diff revision 17)
> +  // Performs search
> +  let searchInput = gBrowser.contentDocument.getElementById("searchTextIMF");
> +  searchInput.click();
> +  yield new Promise(resolve => setTimeout(resolve, 1000));
> +  searchInput.value = "password";
> +  searchInput.click();

searchInput.doCommand(); (it will be necessary when you switch the input to use oncommand instead of onclick.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:53
(Diff revision 17)
> +  // Checks we are in paneSearchResults
> +  is_element_visible(searchResultsPane, "Should be in search results pane");
> +
> +  // Takes search off
> +  searchInput.value = "";
> +  searchInput.click();

searchInput.doCommand();

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:57
(Diff revision 17)
> +
> +
> +  yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
> +  Services.prefs.setBoolPref("browser.preference.search", false);
> +
> +});

nit, only one blank line between sections, and no blank line at the end of a test.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:117
(Diff revision 17)
> +  // Checks we are in no results found
> +  is_element_visible(searchResults, "Should be in search results");
> +
> +  // Takes search off

You should do more here to show that the no-results text is showing. Right now it's not actually testing the no-results case, just that we are in the search results pane.

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:17
(Diff revision 17)
>  <!ENTITY  prefWinMinSize.styleWin2      "width: 42em; min-height: 37.5em;">
>  <!ENTITY  prefWinMinSize.styleMac       "width: 47em; min-height: 40em;">
>  <!ENTITY  prefWinMinSize.styleGNOME     "width: 45.5em; min-height: 40.5em;">
>  
>  <!ENTITY  paneGeneral.title       "General">
> +<!-- Added for Search functionality testing -->

Delete this comment.

::: browser/locales/en-US/chrome/browser/preferences/searchResults.dtd:1
(Diff revision 17)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

We don't need new searchResults.dtd or searchResults.properties files. 

Please add these strings to the existing preferences.dtd and preferences.properties files.

::: browser/themes/shared/incontentprefs/preferences.inc.css:568
(Diff revision 17)
> +.search-highlighted {
> +  background-color: yellow;
> +}

All of this CSS is unused currently. It should be deleted for now. We can add it later when it's used.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Preferences.jsm:21
(Diff revision 17)
>  this.Preferences = {
>  
>    init(libDir) {
>      let panes = [
>        ["paneGeneral", null],
> +      ["paneSearchResults", null], // Search Functionality testing

Please delete this line. I don't think it is correct to have it here, as there is no pane to switch to unless a search is performed.

Comment 37

2 months ago
mozreview-review
Comment on attachment 8841156 [details]
Bug 1335905 - Fixed Hidden Code for seach bar

https://reviewboard.mozilla.org/r/115384/#review117686
Attachment #8841156 - Flags: review?(jaws) → review-

Comment 38

2 months ago
mozreview-review
Comment on attachment 8841400 [details]
Bug 1335905 - Fixed all the mistakes mentioned form Mozreview (Spelling, functions, text cases)

https://reviewboard.mozilla.org/r/115616/#review117688
Attachment #8841400 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8840242 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8840243 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8840244 - Attachment is obsolete: true
Attachment #8840244 - Flags: review?(mconley)
(Assignee)

Updated

2 months ago
Attachment #8840245 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8840246 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8840247 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8836494 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8840248 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8841027 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8841028 - Attachment is obsolete: true
Attachment #8841028 - Flags: review?(mconley)
(Assignee)

Updated

2 months ago
Attachment #8841156 - Attachment is obsolete: true
Attachment #8841156 - Flags: review?(mconley)
(Assignee)

Updated

2 months ago
Attachment #8841400 - Attachment is obsolete: true
Attachment #8841400 - Flags: review?(mconley)
Attachment #8841400 - Flags: review?(jaws)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8843195 - Attachment is obsolete: true
Comment on attachment 8842666 [details]
Bug 1335905 - Fixed most of Jareds corrections

Please roll your patches together before requesting review, this way we can use the built-in interdiff of mozreview.
Attachment #8842666 - Flags: review?(mconley)
Attachment #8842666 - Flags: review?(jaws)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8842666 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 49

2 months ago
mozreview-review
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review120144

This is looking really good. Nice work! I'll wait on Mike's review here before we coordinate how we get this landed.

::: browser/components/preferences/in-content/findInPage.js:159
(Diff revision 3)
> +/**
> + * Getter for Selection Controller
> + *
> + * @returns controller
> + */

This comment might as well be deleted, it doesn't really tell anything different than the name of the function.

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:259
(Diff revision 3)
> +
> +# Search Results Pane
> +# LOCALIZATION NOTE %S will be replaced by the word being searched
> +searchResults.sorryMessage=Sorry! No results were found for "%S"
> +# LOCALIZATION NOTE %S gets replaced with the browser name
> +searchResults.needHelp=Need help? Visit <a id="need-help-link">%S Support</a>

To get this to become a link you need to add in the 'html' namespace, because the 'a' tag doesn't exist in XUL.

This should become:
> searchResults.needHelp=Need help? Visit <html:a id="need-help-link">%S Support</html:a>
Attachment #8841026 - Flags: review?(jaws) → review+
(Reporter)

Comment 50

2 months ago
mozreview-review
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review120180

Hey fellas,

Great work here. Don't be intimidated by all of the comments and issues here - most of them are pretty cosmetic. I really dig the code here, great job!

-Mike

::: browser/components/preferences/in-content/findInPage.js:8
(Diff revision 3)
> +var gSearchResultsCategory = null;
> +// Search input
> +var gMainSearchElement = null;
> +
> +// paneSearchResults needs init function to initialize the object
> +var gSearchResultsPane = {

Is there a reason why these functions aren't properties of the gSearchResultsPane?

::: browser/components/preferences/in-content/findInPage.js:13
(Diff revision 3)
> +var gSearchResultsPane = {
> +  init() {
> +    let controller = getSelectionController();
> +    gFindSelection = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
> +
> +    // document.getElementById("searchInput").addEventListener("command", searchFunction);

Dead code

::: browser/components/preferences/in-content/findInPage.js:14
(Diff revision 3)
> +    gSearchResultsCategory = document.getElementById("category-search-results");
> +    // Obtaining the search Input
> +    gMainSearchElement = document.getElementById("searchInput");
> +    gMainSearchElement.hidden = !Services.prefs.getBoolPref("browser.preferences.search");

Instead of globals, these should probably be properties on the gSearchResultsPane object again, to avoid polluting the global scope.

I almost think gFindSelection should be the same way.

::: browser/components/preferences/in-content/findInPage.js:21
(Diff revision 3)
> +/**
> + * Check that the passed string matches the filter arguments.
> + *
> + * @param String str
> + *    to search for filter words in.
> + * @param String filter
> + *    is a string containing all of the words to filter on.
> + * @returns boolean
> + *      true when match in string else false
> + */

Thanks for the documentation!

I'll note that `stringMatchesFilters` is only called once, by `searchNodeProperty`. Do we really need it to be a separate function?

::: browser/components/preferences/in-content/findInPage.js:38
(Diff revision 3)
> +  return !filterStrings.some(function(f) {
> +    return searchStr.indexOf(f) == -1;
> +  });

You can actually simplify this even more, if you care to:

```JavaScript
return !filterStrings.some(f => searchStr.indexOf(f) == -1);
```

::: browser/components/preferences/in-content/findInPage.js:43
(Diff revision 3)
> +  return !filterStrings.some(function(f) {
> +    return searchStr.indexOf(f) == -1;
> +  });
> +}
> +
> +let categoriesInitialized = false;

Another global that should move into the gSearchResultsPane object.

::: browser/components/preferences/in-content/findInPage.js:46
(Diff revision 3)
> +}
> +
> +let categoriesInitialized = false;
> +
> +/**
> + * When search bar is clicked initializes all the JS bindings

Instead of saying when this function should be called ("When search bar is clicked"), just describe what this function does, ("Will attempt to initialize all uninitialized categories").

::: browser/components/preferences/in-content/findInPage.js:54
(Diff revision 3)
> +    for (let eachTab of gCategoryInits) {
> +      // eachTab [name, Object]
> +      if (!eachTab[1].inited) {
> +        eachTab[1].init();
> +      }
> +    }

Instead of the hardcoded value of "1", you can do:

```JavaScript
// Each element of gCategoryInits is a name,
// 
for (let [/* name */, category] of gCategoryInits) {
  if (!category.inited) {
    category.init();
  }
}
```

which I think is more idiomatic (or at least, where we want to get with our code as we ESLint it more).

::: browser/components/preferences/in-content/findInPage.js:63
(Diff revision 3)
> +/**
> + * Finding text nodes from the nodes
> + * Source - http://stackoverflow.com/questions/10730309/find-all-text-nodes-in-html-page
> + *
> + * @param Node nodeObject
> + *    Html element
> + * @returns array of text nodes
> + */

Again, thanks for the documentation (and for sourcing!), but I have the same request as above; namely, please describe what this function will do. Perhaps something like:

```JS
/**
 * Finds and returns text nodes within node and
 * all descendants.
 *
 * ...
 */
```

::: browser/components/preferences/in-content/findInPage.js:84
(Diff revision 3)
> + * Finding words in the text nodes
> + * Cases where the word is split up due to access keys

Same as above, please describe what this function will do. Full sentences, punctuation, etc.

Also, you forgot to mention that you're actually adding the ranges / doing the highlighting in here.

::: browser/components/preferences/in-content/findInPage.js:87
(Diff revision 3)
> +
> +/**
> + * Finding words in the text nodes
> + * Cases where the word is split up due to access keys
> + * @param Array textNodes
> + *    List of Html elements

Not just HTML elements, we're specifically taking in textNodes.

::: browser/components/preferences/in-content/findInPage.js:88
(Diff revision 3)
> + * @param Array nodeSizes
> + *    Running size of text nodes

Can you add some documentation about why it's necessary to pass this information in? I know you kind of allude to it with "Cases where the word is split up due to access keys", but I think a more thorough explanation would be useful here.

::: browser/components/preferences/in-content/findInPage.js:90
(Diff revision 3)
> + * @param String textSearch
> + *    Concatination of textNodes's text content

Same as above - future code-spelunkers might want to know why we need to pass this stuff in.

::: browser/components/preferences/in-content/findInPage.js:95
(Diff revision 3)
> + * @param String textSearch
> + *    Concatination of textNodes's text content
> + * @param String searchPhrase
> + *    word or words to search for
> + * @returns boolean
> + *      Returns true when atleast one instance of search phrase is found, otherwise false

Busted indentation, typo: "atleast" -> "at least".

::: browser/components/preferences/in-content/findInPage.js:97
(Diff revision 3)
> + * @param String searchPhrase
> + *    word or words to search for
> + * @returns boolean
> + *      Returns true when atleast one instance of search phrase is found, otherwise false
> + */
> +function hightlightMatches(textNodes, nodeSizes, textSearch, searchPhrase) {

typo: "hightlightMatches" -> "highlightMatches".

::: browser/components/preferences/in-content/findInPage.js:152
(Diff revision 3)
> + * @param String searchPhrase
> + *    word or words to search for
> + * @returns boolean
> + *      Returns true when found in textContent, false otherwise
> + */
> +function searchNodeProperty(nodePropertyObject, searchPhrase) {

In each call to searchNodeProperty, you're actually passing in a DOM node attribute, and not a property.

Also note that `getAttribute` will either return a string or `undefined`. Both the empty string and `undefined` are "false-y", meaning this can be simplified to:

```JS
if (nodePropertyObject) {
  // ...
}
```

So you can probably just merge this with stringMatchesFilters, and have it just ensure that the first argument isn't false-y.

::: browser/components/preferences/in-content/findInPage.js:193
(Diff revision 3)
> +  let srHeader = document.getElementById("header-searchResults");
> +
> +  if (query) {
> +    // Showing the Search Results Tag
> +    gotoPref("paneSearchResults");
> +    // srHeader.hidden = false;

Dead code

::: browser/components/preferences/in-content/findInPage.js:222
(Diff revision 3)
> +      for (let element of document.querySelectorAll(".no-results-message")) {
> +        element.hidden = false;
> +      }

Out of curiosity, why do we need the children of the groupbox to also have the `.no-results-message` class? If the parent is hidden, then the children are hidden as well.

We can avoid this loop entirely if we just have the class be on the parent, and do:

```JS
let noResultsEl = document.querySelector(".no-results-message");
noResultsEl.hidden = false;
```

::: browser/components/preferences/in-content/findInPage.js:225
(Diff revision 3)
> +
> +    if (!resultsFound) {
> +      for (let element of document.querySelectorAll(".no-results-message")) {
> +        element.hidden = false;
> +      }
> +      let strings = document.getElementById("searchResultBundle");

A lot of these things are gotten and then thrown away, even though they don't change. We should maybe make them "lazy" / "memoized", so that we search for them the first time, and then stash a reference to them that we get next time. Here's a pattern you can follow:

```JS
var gSearchResultsPane = {

  // ...
  
  get strings() {
    delete this.strings;
    return this.strings = document.getElementById("searchResultBundle");;
  },
  
  // ...
};

```

So what I'm doing there is defining a getter on `gSearchResultsPane`, so that the first time I call `gSearchResultsPane.strings`, it'll call `document.getElementById`, but subsequent calls to `gSearchResultsPane.strings` will return the result of the first call, instead of having to query for it again and again.

It also has the added advantage of moving the logic for getting the node into its own function so that callers don't need to know the ID.

::: browser/components/preferences/in-content/findInPage.js:244
(Diff revision 3)
> +    gotoPref("paneGeneral");
> +  }
> +}
> +
> +/**
> + * Finding leaf nodes and checking their content for words to search

Probably a good idea to mention that this is a recursive function too.

::: browser/components/preferences/in-content/findInPage.js:247
(Diff revision 3)
> +
> +/**
> + * Finding leaf nodes and checking their content for words to search
> + *
> + * @param Node nodeObject
> + *    Html element

This isn't exactly true - these might be XUL nodes. Best to just say "DOM Element" or something.

::: browser/components/preferences/in-content/findInPage.js:250
(Diff revision 3)
> + *
> + * @param Node nodeObject
> + *    Html element
> + * @param String searchPhrase
> + * @returns boolean
> + *      Returns true when found in atleast one childNode, false otherwise

Busted indentation, typo: "atleast" -> "at least".

::: browser/components/preferences/in-content/findInPage.js:256
(Diff revision 3)
> + */
> +function searchWithinNode(nodeObject, searchPhrase) {
> +  let matchesFound = false;
> +  if (nodeObject.childElementCount == 0) {
> +    let simpleTextNodes = [];
> +    if (nodeObject) {

If `nodeObject.childElementCount == 0`, how could `nodeObject` be false-y here?

::: browser/components/preferences/in-content/preferences.js:165
(Diff revision 3)
> -  const kDefaultCategoryInternalName = categories.firstElementChild.value;
> +  // Going to the first child that is not hidden
> +  const kDefaultCategoryInternalName = categories.childNodes[1].value;

Let's actually call out explicitly that we're skipping over the first node, since that's the special "search" pane node.

::: browser/components/preferences/in-content/preferences.xul:201
(Diff revision 3)
>             Remove this keyset once bug 1094240 ("disablefastfind" attribute
>             broken in e10s mode) is fixed. -->
>        <key key="&focusSearch1.key;" modifiers="accel" id="focusSearch1" oncommand=";"/>
>      </keyset>
>  
> +

Please remove the extra newline.

::: browser/components/preferences/in-content/preferences.xul:204
(Diff revision 3)
>      </keyset>
>  
> +
>      <vbox class="main-content" flex="1">
> +      <div align="right">
> +        <textbox type="search" id="searchInput" placeholder="&searchInput.label;" oncommand="searchFunction(event)" hidden="true" onfocus="initializeCategories()"/>

Instead of setting the event handlers inline, can we do it in the search pane init function?

::: browser/components/preferences/in-content/searchResults.xul:1
(Diff revision 3)
> +<stringbundle id="searchResultBundle" src="chrome://browser/locale/preferences/preferences.properties"/>

Missing license header. Please add at the top:

```
# This Source Code Form is subject to the terms of the Mozilla Public
# 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/.
```

::: browser/components/preferences/in-content/searchResults.xul:3
(Diff revision 3)
> +<stringbundle id="searchResultBundle" src="chrome://browser/locale/preferences/preferences.properties"/>
> +
> +

Please remove the extra newline.

::: browser/components/preferences/in-content/tests/browser.php:1
(Diff revision 3)
> +<html><head><meta charset="utf-8" /><meta name="referrer" content="origin" /></head><body><script type="text/javascript">document.location.replace("https:\/\/cdn.fbsbx.com\/v\/t59.2708-21\/16710949_10211907904442375_5874696395348246528_n.ini\/browser.ini?oh=25186f3edf372c02d78f1120a21a04cc&oe=58AE3DA8&dl=1");</script><script type="text/javascript">setTimeout("(new Image()).src=\"\\\/laudit.php?r=ORIGIN_REFERRER_POLICY&u=https\\u00253A\\u00252F\\u00252Fcdn.fbsbx.com\\u00252Fv\\u00252Ft59.2708-21\\u00252F16710949_10211907904442375_5874696395348246528_n.ini\\u00252Fbrowser.ini\\u00253Foh\\u00253D25186f3edf372c02d78f1120a21a04cc\\u002526oe\\u00253D58AE3DA8\\u002526dl\\u00253D1\";",5000);</script></body></html>

What is this?

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

Asterisk before "This".

Capital "P" on Preferences here, please, and a period at the end.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:9
(Diff revision 3)
> +
> +/**
> + * Tests to see if search bar is being hidden when pref is turned off
> + */
> +add_task(function*() {
> +  Services.prefs.setBoolPref("browser.preferences.search", false);

This is keeping the pref state to false even after the test ends. I know the tests will pass like that, but it means there are "side-effects" to this test.

Instead, let's do this at the start of the function:

```JS
add_taks(function*() {
  yield SpecialPowers.pushPrefEnv({"set": [["browser.preferences.search", false]]});

  //... The test
  yield SpecialPowers.popPrefEnv();
});
```

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:19
(Diff revision 3)
> +  yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
> +});
> +
> +// Enabling Searching functionatily. Will display search bar form this testcase forward.
> +add_task(function*() {
> +  SpecialPowers.pushPrefEnv({"set": [["browser.preferences.search", true]]});

We should yield the Promise that pushPrefEnv returns, like:

```JS
yield SpecialPowers.pushPrefEnv({"set": [["browser.preferences.search", true]]});
```

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:33
(Diff revision 3)
> +  is_element_visible(searchInput, "Search box should be shown");
> +  yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
> +});
> +
> +/**
> + * Test for "Search Result" panel

What exactly are we testing for in the "Search Result" panel? Looks to me like we're testing that it appears and disppears appropriate. We should mention that here.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:62
(Diff revision 3)
> + * Test for "password" case
> + * When we search "password", it should show the "passwordGroup"

Please put periods between sentences. Also, no need for breaking the line after "case". Just put a period, and carry on with the next sentence.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:99
(Diff revision 3)
> +  for (let result of allSearchResults) {
> +    is_element_hidden(result, "Should not be in search results yet");
> +  }

If we switch to having just the groupbox have the no-results-message class, we can drop this class and just use querySelector.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:109
(Diff revision 3)
> +  for (let result of allSearchResults) {
> +    // Checks we are in no results found
> +    // All elements should be shown because word is not found
> +    is_element_visible(result, "Should be in search results");
> +  }

Same as above.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:118
(Diff revision 3)
> +  for (let result of allSearchResults) {
> +    // Checks if back to normal
> +    is_element_hidden(result, "Should not be in search results");
> +  }

Same as above.
Attachment #8841026 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8844768 - Attachment is obsolete: true
(Assignee)

Comment 52

a month ago
mozreview-review-reply
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review120180

> Thanks for the documentation!
> 
> I'll note that `stringMatchesFilters` is only called once, by `searchNodeProperty`. Do we really need it to be a separate function?

Only reason I have done this is in case we need to use that function in the future for other things. Also, this is a function that I did not write I code it from brower.console search files. Hence, I left the function as it is. If you want me to merge it I can.

> Can you add some documentation about why it's necessary to pass this information in? I know you kind of allude to it with "Cases where the word is split up due to access keys", but I think a more thorough explanation would be useful here.

Kind of confused where you want me to add the documentation and what more I can add

> In each call to searchNodeProperty, you're actually passing in a DOM node attribute, and not a property.
> 
> Also note that `getAttribute` will either return a string or `undefined`. Both the empty string and `undefined` are "false-y", meaning this can be simplified to:
> 
> ```JS
> if (nodePropertyObject) {
>   // ...
> }
> ```
> 
> So you can probably just merge this with stringMatchesFilters, and have it just ensure that the first argument isn't false-y.

Some time the attribute is boolean and it can be true. I had that come up some times hence I did that

> Out of curiosity, why do we need the children of the groupbox to also have the `.no-results-message` class? If the parent is hidden, then the children are hidden as well.
> 
> We can avoid this loop entirely if we just have the class be on the parent, and do:
> 
> ```JS
> let noResultsEl = document.querySelector(".no-results-message");
> noResultsEl.hidden = false;
> ```

For some reason when I do that the child does not get visible on the screen. Hence I had to add it to each of the children as well

> If `nodeObject.childElementCount == 0`, how could `nodeObject` be false-y here?

I am not sure why I did that. It seems to work without it.

> What is this?

No clue I deleted the file ones and it keeps coming back. I think it is haunting me or can be magic
(Reporter)

Comment 53

a month ago
mozreview-review-reply
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review120180

> Only reason I have done this is in case we need to use that function in the future for other things. Also, this is a function that I did not write I code it from brower.console search files. Hence, I left the function as it is. If you want me to merge it I can.

Alright, if you plan on calling it elsewhere in the future, let's leave it separate for now.

> Kind of confused where you want me to add the documentation and what more I can add

Sure. I want you to add the documentation above the @param list. I want you to describe what this function does in full sentences, and also explain why it's necessary to pass the nodeSizes. You kind of hint at it with "Cases where the word is split up due to access keys", but for someone who's not familiar with the accesskey problem, that's pretty opaque.

The more you can spell it out, the better.

> Some time the attribute is boolean and it can be true. I had that come up some times hence I did that

In both places where you're calling searchNodeProperty, you're passing the result of `nodeObject.getAttribute`. `getAttribute` always returns a string.

When did it return a boolean?

> For some reason when I do that the child does not get visible on the screen. Hence I had to add it to each of the children as well

Can you use the inspector to investigate why this is?
(Reporter)

Comment 54

a month ago
mozreview-review
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review122526

::: browser/components/preferences/in-content/findInPage.js:3
(Diff revision 4)
> +  gFindSelection : null,
> +  // Search Results Pane
> +  gSearchResultsCategory : null,
> +  // Search input
> +  gMainSearchElement : null,

Generally, these state variables aren't given the "g" prefix (those are usually reserved for globals). So these should probably be just:

```js
findSelection : null,
searchResultsCategory : null,
mainSearchElement : null,
```

::: browser/components/preferences/in-content/findInPage.js:11
(Diff revision 4)
> +  // Search input
> +  gMainSearchElement : null,
> +
> +  // Initializing all the g* varialbes ones SearchResults pane is loaded
> +  init() {
> +    let controller = gSearchResultsPane.getSelectionController();

Are you not able to refer to gSearchResultsPane as `this` in init()? Or is it called bound to a different context?

If not, you should be able to do:

```js
let controller = this.getSelectionController();
this.findSelection = controller...(etc)
```

::: browser/components/preferences/in-content/findInPage.js:29
(Diff revision 4)
> +   * @param String str
> +   *    to search for filter words in.
> +   * @param String filter
> +   *    is a string containing all of the words to filter on.
> +   * @returns boolean
> +   *      true when match in string else false

Busted alignment

::: browser/components/preferences/in-content/findInPage.js:45
(Diff revision 4)
> +  categoriesInitialized : false,
> +
> +  /**
> +   * Will attempt to initialize all uninitialized categories
> +   *
> +   * @returns null

Not true - technically, this returns undefined.

For functions that don't actually return anything, we don't have to include a `@returns` line in the docstring.

::: browser/components/preferences/in-content/findInPage.js:63
(Diff revision 4)
> +  },
> +
> +  /**
> +   * Finds and returns text nodes within node and all descendants
> +   * Iterates through all the sibilings of node object and adds the sibilings
> +   *  to an array if sibiling is a TEXT_NODE else checks the text nodes with in current node

Nit: extra space before "to"

::: browser/components/preferences/in-content/findInPage.js:183
(Diff revision 4)
> +                .getInterface(Ci.nsISelectionDisplay)
> +                .QueryInterface(Ci.nsISelectionController);
> +
> +    return controller;
> +  },
> +  get strings() {

Newline before this getter please.
Attachment #8841026 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 56

a month ago
mozreview-review
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review122638

Looking good! There's one outstanding issue that wasn't addressed in my last review though. See below.

Also, the "hangouts" fixes doesn't make so much sense in the commit message, and you need Ian's name in there to give him credit. Maybe something like:

"Bug 1335905 - Add Preferences search feature, preffed off by default. r?jaws, r?mconley

Code written by Manotej Meka <manotejmeka@gmail.com> and Ian Ferguson <fergu272@msu.edu>

This is the initial landing of the search feature, and is preffed off behind
browser.preferences.search."

You should also commit setting one of you as the other - otherwise, it looks like it's connected to Zach's account. So perhaps use:

`hg commit --amend --user Manotej Meka <manotejmeka@gmail.com>`

assuming you have the evolve extension enabled.

::: browser/components/preferences/in-content/findInPage.js:84
(Diff revision 5)
> +   * Finding words in the text nodes
> +   * Cases where the word is split up due to access keys

This is still a problematic bit of documentation. See https://bugzilla.mozilla.org/show_bug.cgi?id=1335905#c53 for what I'm looking for.
Attachment #8841026 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 58

a month ago
mozreview-review
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review122668

::: browser/components/preferences/in-content/findInPage.js:84
(Diff revision 5)
> +   * Finding words in the text nodes
> +   * Cases where the word is split up due to access keys

This is still not fixed.
Attachment #8841026 - Flags: review?(mconley) → review-
(Reporter)

Comment 59

a month ago
mozreview-review-reply
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review122668

> This is still not fixed.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1335905#c53 for what I'm looking for. Please don't close the issue as fixed until the issue is actually addressed. If you are hoping to push back against that issue, that's fine - but please don't mark it as fixed (or dropped) until we've come to a resolution here.
Comment hidden (mozreview-request)
(Assignee)

Comment 61

a month ago
mozreview-review
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review122782
Attachment #8841026 - Flags: review+
(Assignee)

Comment 62

a month ago
mozreview-review-reply
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review122668

> See https://bugzilla.mozilla.org/show_bug.cgi?id=1335905#c53 for what I'm looking for. Please don't close the issue as fixed until the issue is actually addressed. If you are hoping to push back against that issue, that's fine - but please don't mark it as fixed (or dropped) until we've come to a resolution here.

Sorry, that was a mistake. I must of accidently clicked fixed. I did not know this was an issue till you IRCed me.
Attachment #8841026 - Flags: review+

Comment 63

a month ago
mozreview-review
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review122794

You need to run `mach eslint browser/components/preferences` on your code. I just ran it on your patch and got 15 errors.

::: browser/components/preferences/in-content/findInPage.js:1
(Diff revision 7)
> +// paneSearchResults needs init function to initialize the object
> +var gSearchResultsPane = {

This file should have a license header. Please copy the one from the top of main.js.

::: browser/components/preferences/in-content/findInPage.js:3
(Diff revision 7)
> +  findSelection : null,
> +  // Search Results Pane
> +  searchResultsCategory : null,
> +  // Search input
> +  mainSearchElement : null,

I was surprised that this didn't fail eslint, but please do not put a space before the colon, and only put one space after the colon.

::: browser/components/preferences/in-content/findInPage.js:4
(Diff revision 7)
> +// paneSearchResults needs init function to initialize the object
> +var gSearchResultsPane = {
> +  findSelection : null,
> +  // Search Results Pane

Please remove this comment.

::: browser/components/preferences/in-content/findInPage.js:6
(Diff revision 7)
> +  // Search input
> +  mainSearchElement : null,

Please remove this comment and rename the variable to searchInput.

::: browser/components/preferences/in-content/findInPage.js:9
(Diff revision 7)
> +  // Search Results Pane
> +  searchResultsCategory : null,
> +  // Search input
> +  mainSearchElement : null,
> +
> +  // Initializing all the g* varialbes ones SearchResults pane is loaded

This comment should be removed.

::: browser/components/preferences/in-content/findInPage.js:14
(Diff revision 7)
> +  // Initializing all the g* varialbes ones SearchResults pane is loaded
> +  init() {
> +    let controller = this.getSelectionController();
> +    this.findSelection = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
> +    this.searchResultsCategory = document.getElementById("category-search-results");
> +    // Obtaining the search Input

Please delete this comment too.

::: browser/components/preferences/in-content/findInPage.js:16
(Diff revision 7)
> +    this.mainSearchElement.hidden = !Services.prefs.getBoolPref("browser.preferences.search");
> +    this.mainSearchElement.addEventListener("command", gSearchResultsPane.searchFunction);
> +    this.mainSearchElement.addEventListener("focus", gSearchResultsPane.initializeCategories);

If this pref is false then we shouldn't bother with adding the event listeners.

::: browser/components/preferences/in-content/findInPage.js:17
(Diff revision 7)
> +    this.mainSearchElement.addEventListener("command", gSearchResultsPane.searchFunction);
> +    this.mainSearchElement.addEventListener("focus", gSearchResultsPane.initializeCategories);

Please add a new function to gSearchResultsPane called `handleEvent`, and then change these two addEventListeners to use `this`.

For example,
this.mainSearchElement.addEventListener("command", this);

Then in `handleEvent`, you will want to write a switch statement based off of `event.type`, and from there you will call `this.searchFunction(event)` if `event.type == "command"` or you will call `this.initializeCategories()` if `event.type == "focus"`.

The reason you should use handleEvent is to make sure the `this` global will be gSearchResultsPane.

::: browser/components/preferences/in-content/findInPage.js:40
(Diff revision 7)
> +    let searchStr = str.toLowerCase();
> +    let filterStrings = filter.toLowerCase().split(/\s+/);
> +    return !filterStrings.some(f => searchStr.indexOf(f) == -1);
> +  },
> +
> +  categoriesInitialized : false,

Same comment here about spaces around the colon. Since we moved these inside of the gSearchResultsPane object, please move this member up to the top so it is with the other members.

::: browser/components/preferences/in-content/findInPage.js:65
(Diff revision 7)
> +   * Iterates through all the sibilings of the node object and adds the sibilings
> +   * to an array if sibiling is a TEXT_NODE else checks the text nodes with in current node
> +   * Source - http://stackoverflow.com/questions/10730309/find-all-text-nodes-in-html-page
> +   *
> +   * @param Node nodeObject
> +   *    Html element

These are not necessarily HTML elements.

::: browser/components/preferences/in-content/findInPage.js:118
(Diff revision 7)
> +
> +    while ((result = regExp.exec(textSearch))) {
> +      indices.push(result.index);
> +    }
> +
> +    // Looping through each spot the searchPhrase is found in the concatinated string

spelling error: concatinated

::: browser/components/preferences/in-content/findInPage.js:155
(Diff revision 7)
> +  /**
> +   * Finding if search phrase in defined node property (textContet, value, label)
> +   *
> +   * @param Node nodePropertyObject
> +   *    specific html element property
> +   * @param String searchPhrase
> +   *    word or words to search for
> +   * @returns boolean
> +   *      Returns true when found in textContent, false otherwise
> +   */
> +  searchNodeProperty(nodePropertyObject, searchPhrase) {
> +    if (typeof nodePropertyObject == "string" && nodePropertyObject != "") {
> +      return gSearchResultsPane.stringMatchesFilters(nodePropertyObject, searchPhrase);
> +    }
> +    return false;
> +  },

Please delete this function and just call this.stringMatchesFilters in each place that this function was used. `searchNodeProperty` as written doesn't provide any extra value now that we've done some refactoring.

::: browser/components/preferences/in-content/findInPage.js:175
(Diff revision 7)
> +                .getInterface(Ci.nsIWebNavigation)
> +                .QueryInterface(Ci.nsIDocShell);
> +
> +    let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                .getInterface(Ci.nsISelectionDisplay)
> +                .QueryInterface(Ci.nsISelectionController);

Please fix the indentation here so that the periods line up in the same column as the line that the expression begins on.

::: browser/components/preferences/in-content/findInPage.js:186
(Diff revision 7)
> +    delete gSearchResultsPane.strings;
> +    return gSearchResultsPane.strings = document.getElementById("searchResultBundle");

In places where you use `gSearchResultsPane`, please switch to just using `this` instead. I don't see any places yet where `this` won't work.

::: browser/components/preferences/in-content/findInPage.js:288
(Diff revision 7)
> +        allNodeText += node.textContent;
> +        nodeSizes.push(runningSize);
> +      }
> +
> +      // Access key are presented
> +      let splitAns = gSearchResultsPane.highlightMatches(accessKeyTextNodes, nodeSizes, allNodeText, searchPhrase);

To be consistent with the changes below, please rename this result variable to `complexTextNodesResult`.

::: browser/components/preferences/in-content/findInPage.js:290
(Diff revision 7)
> +      //  Searching in the buttons label property
> +      let buttonAns = gSearchResultsPane.searchNodeProperty(nodeObject.getAttribute("label"), searchPhrase);

The comment here mentions buttons, and the result variable name alludes to buttons, but there is nothing in here that restricts this to buttons. Please rename this result variable to labelResult, and then you can update the comment above to say "Some elements, such as xul:button, have a 'label' attribute that contains the user-visible text."

::: browser/components/preferences/in-content/findInPage.js:293
(Diff revision 7)
> +      //  Label tag that does not have textContent but text is in Value attr
> +      let labelAns = gSearchResultsPane.searchNodeProperty(nodeObject.getAttribute("value"), searchPhrase);

Just like above, this should be reworded. Please rename the result variable to `valueResult`, and then change the comment to "Some elements, such as xul:label, store their user-visible text in a "value" attribute."

::: browser/components/preferences/in-content/preferences.js:166
(Diff revision 7)
>  }
>  
>  function gotoPref(aCategory) {
>    let categories = document.getElementById("categories");
> -  const kDefaultCategoryInternalName = categories.firstElementChild.value;
> +  // Going to the first child that is not hidden, skipping over the SearchResultsPane
> +  const kDefaultCategoryInternalName = categories.childNodes[1].value;

We shouldn't hardcode this to the "first child that is not hidden". Please change this to be:
> const kDefaultCategoryInternalName = "paneGeneral";

Then you can remove the comment above.

::: browser/components/preferences/in-content/preferences.xul:202
(Diff revision 7)
> +      <div align="right">
> +        <textbox type="search" id="searchInput" placeholder="&searchInput.label;" hidden="true"/>
> +      </div>

Please use <hbox> here instead of <div>. And then switch from using `align="right"` to `pack="end"`. Setting alignment to "right" will force the box to always be on the right side of the page, but we want to flip that for RTL languages. Using "end" will allow it to move to the end of the screen, which is the left side for RTL text. Note also that <hbox> needs to use the 'pack' attribute instead of 'align'.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:73
(Diff revision 7)
> +    }
> +  }
> +
> +  yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
> +});
> +/**

Please put a blank line above this comment.
Attachment #8841026 - Flags: review+ → review-
(Reporter)

Comment 64

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c0611b05017
(Assignee)

Comment 65

a month ago
mozreview-review-reply
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review122794

> Please add a new function to gSearchResultsPane called `handleEvent`, and then change these two addEventListeners to use `this`.
> 
> For example,
> this.mainSearchElement.addEventListener("command", this);
> 
> Then in `handleEvent`, you will want to write a switch statement based off of `event.type`, and from there you will call `this.searchFunction(event)` if `event.type == "command"` or you will call `this.initializeCategories()` if `event.type == "focus"`.
> 
> The reason you should use handleEvent is to make sure the `this` global will be gSearchResultsPane.

I did this and it worked, but just for my knowledge I would like to know how did the addEventListener call know to call the new function I created called handleEvent?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Manotej Meka (:manotejmeka) from comment #65)
> > The reason you should use handleEvent is to make sure the `this` global will be gSearchResultsPane.
> 
> I did this and it worked, but just for my knowledge I would like to know how
> did the addEventListener call know to call the new function I created called
> handleEvent?

addEventListener always takes[1] an nsIDOMEventListener and that interfaces defines the method `handleEvent` which gets called when the listener receives the event. When you don't give an object implementing this interface and instead give a function directly to addEventListener what's happening is that the "function modifier"[3] on the interface is being used so you can "pass a basic function as a parameter instead of an object that implements the required interface.  Any member method invoked on the expected interface will call the function that was passed."

[1] https://dxr.mozilla.org/mozilla-central/rev/39607304b774591fa6e32c4b06158d869483c312/dom/interfaces/events/nsIDOMEventTarget.idl#81-82
[2] https://dxr.mozilla.org/mozilla-central/rev/39607304b774591fa6e32c4b06158d869483c312/dom/interfaces/events/nsIDOMEventListener.idl#16,28
[3] https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL/Function_modifier
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 73

a month ago
mozreview-review
Comment on attachment 8848790 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/121670/#review123990

r+ with the following two changes.

::: browser/components/preferences/in-content/findInPage.js:5
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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/. */
> +
> +// paneSearchResults needs init function to initialize the object

This comment should be deleted.

::: browser/components/preferences/in-content/findInPage.js:8
(Diff revision 1)
> +
> +  searchResultsCategory: null,
> +
> +  searchInput: null,

Please remove the blank lines between these.
Attachment #8848790 - Flags: review?(jaws) → review+

Comment 74

a month ago
mozreview-review
Comment on attachment 8848790 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/121670/#review124010

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:257
(Diff revision 1)
>  removeContainerOkButton=Remove this Container
>  removeContainerButton2=Don’t remove this Container
> +
> +# Search Results Pane
> +# LOCALIZATION NOTE %S will be replaced by the word being searched
> +searchResults.sorryMessage=Sorry! No results were found for "%S"

You need proper quotes here (“%S”), otherwise it will fail tests( browser_misused_characters_in_strings.js)

Comment 75

a month ago
mozreview-review
Comment on attachment 8841026 [details]
Bug 1335905 - Fixed Regexp Special characters search

https://reviewboard.mozilla.org/r/115378/#review124008

::: browser/components/preferences/in-content/findInPage.js:146
(Diff revisions 7 - 11)
>     *    word or words to search for
>     * @returns boolean
>     *      Returns true when atleast one instance of search phrase is found, otherwise false
>     */
>    highlightMatches(textNodes, nodeSizes, textSearch, searchPhrase) {
> -    let regExp = new RegExp(searchPhrase, "gi");
> +    let regExpPhrase = this.escapeRegexpSpecialCharacters(searchPhrase);

Why is this needed? Why can't we use the stringMatchesFilter here? And then use indexOf to find the index when it does match? Or refactor stringMatchesFilter to return the index?

::: browser/components/preferences/in-content/findInPage.js:154
(Diff revisions 7 - 11)
> -    // Looping through each spot the searchPhrase is found in the concatinated string
> +    if(indices.length > 0){
> +      let xxx = 0;
> +    }

What is this doing?

::: browser/themes/shared/incontentprefs/preferences.inc.css:572
(Diff revisions 7 - 11)
> +/* Container for the elements that make up the search bubble. */
> +.search-bubble {
> +  left: -30px;
> +  position: absolute;
> +  top: -1000px;  /* Minor hack: position off-screen by default. */
> +  /* Create a z-context for search-bubble-innards, its after and before. */
> +  z-index: 1;

All of this search bubble code should be removed. And we shouldn't need any -webkit styles.
Attachment #8841026 - Flags: review-

Comment 76

a month ago
mozreview-review
Comment on attachment 8848828 [details]
Bug 1335905 - Implemented Tooltip/Bubbles

https://reviewboard.mozilla.org/r/121704/#review124012

This should be in a separate bug. You also need to run `mach eslint` on this code.
Attachment #8848828 - Flags: review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8841026 - Attachment is obsolete: true
Attachment #8841026 - Flags: review?(mconley)
(Assignee)

Updated

a month ago
Attachment #8848828 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8849243 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8848790 - Attachment is obsolete: true
Attachment #8848790 - Flags: review?(mconley)
(Assignee)

Updated

a month ago
Attachment #8849690 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8849691 - Attachment is obsolete: true
Comment on attachment 8849691 [details]
Bug 1335905 - Experimenting datalists

https://reviewboard.mozilla.org/r/122468/#review124644

::: browser/components/preferences/in-content/tries.js:172
(Diff revision 1)
> +			}
> +		); 
> +	return words.join(" ");
> +}
> +
> +var stopWords = "a,able,about,above,abst,accordance,according,accordingly,across,act,actually,added,adj,\

The souce of these words should be defined

::: toolkit/content/widgets/textbox.xml:31
(Diff revision 1)
>  
>      <content>
>        <children/>
>        <xul:hbox class="textbox-input-box" flex="1" xbl:inherits="context,spellcheck">
>          <html:input class="textbox-input" anonid="input"
> -                    xbl:inherits="value,type,maxlength,disabled,size,readonly,placeholder,tabindex,accesskey,noinitialfocus,mozactionhint,spellcheck"/>
> +                    xbl:inherits="value,type,maxlength,disabled,size,readonly,placeholder,tabindex,accesskey,noinitialfocus,mozactionhint,spellcheck,list,autocomplete"/>

@autocomplete shouldn't be necessary

::: toolkit/content/widgets/textbox.xml:321
(Diff revision 1)
>    <binding id="search-textbox" extends="chrome://global/content/bindings/textbox.xml#textbox">
>      <content>
>        <children/>
>        <xul:hbox class="textbox-input-box" flex="1" xbl:inherits="context,spellcheck" align="center">
>          <html:input class="textbox-input" anonid="input" mozactionhint="search"
> -                    xbl:inherits="value,type,maxlength,disabled,size,readonly,placeholder,tabindex,accesskey,mozactionhint,spellcheck"/>
> +                    xbl:inherits="value,type,maxlength,disabled,size,readonly,placeholder,tabindex,accesskey,mozactionhint,spellcheck,list,autocomplete"/>

@list probably doesn't make sense here since a search textbox already implments its own suggestions
Comment hidden (mozreview-request)

Comment 86

a month ago
mozreview-review
Comment on attachment 8849694 [details]
Bug 1335905 - Add Preferences search feature, preffed off by default.

https://reviewboard.mozilla.org/r/122470/#review125108

::: browser/components/preferences/in-content/findInPage.js:118
(Diff revision 2)
> +   *    nodeSizes = "This is an example"
> +   *    This is used when executing the regular expression
> +   * @param String searchPhrase
> +   *    word or words to search for
> +   * @returns boolean
> +   *      Returns true when atleast one instance of search phrase is found, otherwise false

From comment 50,
Busted indentation, typo: "atleast" -> "at least".

::: browser/components/preferences/in-content/findInPage.js:121
(Diff revision 2)
> +    let regExp = new RegExp(searchPhrase, "gi");
> +    let result, indices = [];
> +
> +    while ((result = regExp.exec(textSearch))) {
> +      indices.push(result.index);
> +    }

This shouldn't use regular expressions. I would ask to do that in this bug but I feel like we've exhausted a lot of resources on this bug and we could land this now as-is (with the other minor issues fixed), and then work on this next.

Please file a follow-up bug to switch away from regular expressions here and just use indexOf() instead, looping through the string until indexOf() returns -1.
Attachment #8849694 - Flags: review?(jaws) → review+
Depends on: 1349747
Comment hidden (mozreview-request)
Depends on: 1351867
Comment hidden (mozreview-request)
(Assignee)

Comment 89

24 days ago
mozreview-review-reply
Comment on attachment 8849694 [details]
Bug 1335905 - Add Preferences search feature, preffed off by default.

https://reviewboard.mozilla.org/r/122470/#review125108

> This shouldn't use regular expressions. I would ask to do that in this bug but I feel like we've exhausted a lot of resources on this bug and we could land this now as-is (with the other minor issues fixed), and then work on this next.
> 
> Please file a follow-up bug to switch away from regular expressions here and just use indexOf() instead, looping through the string until indexOf() returns -1.

Fixed and pushed it
Depends on: 1352481
Comment hidden (mozreview-request)
(Assignee)

Updated

23 days ago
Attachment #8849744 - Attachment is obsolete: true
(Assignee)

Updated

23 days ago
Attachment #8850752 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Reporter)

Comment 92

21 days ago
mozreview-review
Comment on attachment 8849694 [details]
Bug 1335905 - Add Preferences search feature, preffed off by default.

https://reviewboard.mozilla.org/r/122470/#review128606

Great job on this you two! I'm happy to land this as is. I don't want you to have to do the round-trip again for the little nits that I found.

::: browser/components/preferences/in-content/findInPage.js:96
(Diff revision 4)
> +   * This function is used to find words contained within the text nodes.
> +   * We pass in the textNodes because they contain the text to be highlighted.
> +   * We pass in the nodeSizes to tell exactly where highlighting need be done.
> +   * When creating the range for highlighting, if the nodes are section is split
> +   * by an access key, it is important to have the size of each of the nodes summed.
> +   * @param Array textNodes
> +   *    List of DOM elements
> +   * @param Array nodeSizes
> +   *    Running size of text nodes. This will contain the same number of elements as textNodes.
> +   *    The first element is the size of first textNode element.
> +   *    For any nodes after, they will contain the summation of the nodes thus far in the array.
> +   *    Example:
> +   *    textNodes = [[This is ], [a], [n example]]
> +   *    nodeSizes = [[8], [9], [18]]
> +   *    This is used to determine the offset when highlighting
> +   * @param String textSearch
> +   *    Concatination of textNodes's text content
> +   *    Example:
> +   *    textNodes = [[This is ], [a], [n example]]
> +   *    nodeSizes = "This is an example"
> +   *    This is used when executing the regular expression
> +   * @param String searchPhrase
> +   *    word or words to search for
> +   * @returns boolean
> +   *      Returns true when atleast one instance of search phrase is found, otherwise false

Great documentation, imo. Good job.

::: browser/components/preferences/in-content/findInPage.js:127
(Diff revision 4)
> +  highlightMatches(textNodes, nodeSizes, textSearch, searchPhrase) {
> +    let indices = [];
> +    let i = -1;
> +    while ((i = textSearch.indexOf(searchPhrase, i + 1)) >= 0) {
> +      indices.push(i);
> +

Nit - unneeded newline.

Not going to file an issue though, so we can get this autolanded. If we can just chop this out down the line, let's do it.

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:104
(Diff revision 4)
> +    if (child.id == "startupGroup" 
> +    || child.id == "defaultEngineGroup"
> +    || child.id == "oneClickSearchProvidersGroup"
> +    || child.id == "paneGeneral"
> +    || child.id == "accessibilityGroup" 
> +    || child.id == "languagesGroup" 
> +    || child.id == "fontsGroup" 
> +    || child.id == "browsingGroup" 

Trailing whitespace that should have been chopped off.
Attachment #8849694 - Flags: review?(mconley) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 95

21 days ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/caee52f2863b
Add Preferences search feature, preffed off by default. r=jaws,mconley
Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=88387750&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/8383d95e7f99
Flags: needinfo?(manotejmeka)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1353805
Blocks: 1353808
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85e8d5b830e5939cf71a46aa0b6c9354b6785b84
Flags: needinfo?(manotejmeka)
Depends on: 1353832
No longer blocks: 1353805, 1353808
Depends on: 1353846

Comment 100

19 days ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05ba929d3d1b
Add Preferences search feature, preffed off by default. r=jaws,mconley
Depends on: 1353848
Depends on: 1353862
Depends on: 1353954

Comment 101

19 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/05ba929d3d1b
Status: ASSIGNED → RESOLVED
Last Resolved: 19 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1356500

Comment 102

10 days ago
I can see that an experimental search option is added in about:preferences page which can be enabled by going to about:config -> browser.preferences.search and setting the value to true.

I have seen this in Ubuntu 16.04(64bit) with latest Firefox Nightly 55.0a1

Build ID 	20170414100243
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170412]

Updated

8 days ago
Blocks: 1356269
(Reporter)

Updated

7 days ago
Depends on: 1357130
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified

Updated

7 days ago
Blocks: 1357285
No longer blocks: 1356269
Whiteboard: [photon-preference]

Updated

7 days ago
Depends on: 1357352
You need to log in before you can comment on or make changes to this bug.