Open Bug 1349747 Opened 7 years ago Updated 2 years ago

The search box in about:preferences should show search suggestions

Categories

(Firefox :: Settings UI, enhancement)

Product:
Firefox
Component:
Settings UI
Version:
52 Branch
Type:
enhancement

Tracking

()

People

(Reporter: jaws, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Page-Shot-2017-3-22 5 1 Type.png
289.04 KB, image/png
Details
Bug 1349747 - Search Suggestions Implementation through trie tree, datalists.
59 bytes, text/x-review-board-request
Details 
As seen in the attached spec.

As the user types, we should show search suggestions in the autocomplete popup of the search box.
Depends on: 1350243
Attachment #8850725 - Attachment is obsolete: true
Attachment #8850727 - Attachment is obsolete: true
Attachment #8850728 - Attachment is obsolete: true
Attachment #8850726 - Flags: feedback?(jaws)
Comment on attachment 8850726 [details]
Bug 1349747 - Search Suggestions Implementation through trie tree, datalists.

https://reviewboard.mozilla.org/r/123258/#review131126

::: browser/components/preferences/in-content/findInPage.js:27
(Diff revision 6)
>      this.searchResultsCategory = document.getElementById("category-search-results");
>  
>      this.searchInput = document.getElementById("searchInput");
>      this.searchInput.hidden = !Services.prefs.getBoolPref("browser.preferences.search");
> +		document.getElementById("searchSuggestions").hidden = this.searchInput.hidden;
> -    if (!this.searchInput.hidden) {
> +		    if (!this.searchInput.hidden) {

spaces instead of tabs. This should fail eslint http://eslint.org/docs/rules/no-mixed-spaces-and-tabs

::: browser/components/preferences/in-content/findInPage.js:37
(Diff revision 6)
> +  /**
> +   * Repositions the search-input-hbox to be aligned with the Page Header
> +   */
> +  stylingSearchBar() {
> +    // Change this function to be called when about:preferences is made

This should all be part of a separate bug.

::: browser/components/preferences/in-content/findInPage.js:64
(Diff revision 6)
> +    let nodeObject = this.searchInput
> +    let regExp = new RegExp("(?:^|\\s)" + className + "(?!\\S)", "gi");
> +    if (classAction && !nodeObject.className.match(regExp)) {

You need to use the element.classList API instead of running a regex on the className.

::: browser/components/preferences/in-content/findInPage.js:92
(Diff revision 6)
> +        // Jared does not like this might have to implement it different way. Ask for help.
> +        let enterButton = document.createEvent("KeyboardEvent");
> +        enterButton.initKeyEvent("keypress", true, true, window, false, false, false, false, 13, 0);
> +
> +        this.searchInput.value = "testing search";
> +        this.searchInput.dispatchEvent(enterButton);
> +        this.searchInput.value = "";
> +        this.searchInput.dispatchEvent(enterButton);

Nice comment ;)

Although I'm not sure what specifically you are talking about me not liking here? Though I don't know why you need to create the KeyboardEvent and dispatch it? 

It looks like you need to find a better way to initialize search? What have you thought of for this?

::: browser/components/preferences/in-content/findInPage.js:103
(Diff revision 6)
> +			// Remove later Testing for beta presentation
> +      this.trie.addRelatedWords("tab", "firefox");

These should all be removed from the code.

::: browser/components/preferences/in-content/findInPage.js:122
(Diff revision 6)
> +      this.trie.addRelatedWords("spencer", "10 minutes");
> +      this.trie.addRelatedWords("spencer", "time is up");
> +      // Till here
> +
> +    } else if (event.type === "input") {
> +      let query = event.target.value.trim().toLowerCase()

semicolon

::: browser/components/preferences/in-content/findInPage.js:126
(Diff revision 6)
> +      // Stopping the infinite loop search for choosing a Datalist options value
> +      if (this.chooseDataListOption != query) {

What do you mean by infinite loop search? Sounds bad :)

::: browser/components/preferences/in-content/findInPage.js:128
(Diff revision 6)
> +        let datalistOptions = document.getElementById("searchSuggestions").children;
> +
> +        let word = "";
> +        for (let optionObj of datalistOptions) {
> +          word = optionObj.value;
> +          if (word === query) {

So is this looking at each item in the datalist to see if a value matches what was searched for? Why do we need to do this? Autocomplete?

::: browser/components/preferences/in-content/findInPage.js:135
(Diff revision 6)
> +            // Jared mentioned this is bad to do, think of other ways to do the same thing
> +            let enterButton = document.createEvent("KeyboardEvent");
> +            enterButton.initKeyEvent("keypress", true, true, window, false, false, false, false, 13, 0);

Yeah, I still don't understand why this is necessary and there is no comment here explaining it.

::: browser/components/preferences/in-content/findInPage.js:148
(Diff revision 6)
> +        }
> +      }
> +      this.chooseDataListOption = query;
> +      this.searchSuggestions(this.trie.autoComplete(String(query)));
> +    } else if (event.type === "keypress") {
> +      if (this.timer != null) {

nit, you can just do `if (this.timer)` instead of checking if it is not null.

::: browser/components/preferences/in-content/findInPage.js:155
(Diff revision 6)
> +        // Resetting the auto timmer search
> +        this.timer = new DeferredTask(function* () {
> +          gSearchResultsPane.searchFunction(event);
> +        }, 600);
> +        this.timer.arm();

This might as well be a setTimeout since you're arming it right away. Then you would want to call clearTimeout if another keypress comes in.

::: browser/components/preferences/in-content/findInPage.js:162
(Diff revision 6)
> +          gSearchResultsPane.searchFunction(event);
> +        }, 600);
> +        this.timer.arm();
> +      }
> +    } else if (event.type === "mousedown") {
> +      // Computing if close button was clicked

No need to do manual pixel calculations, you should be able to check event.target

::: browser/components/preferences/in-content/findInPage.js:173
(Diff revision 6)
> +      // Jared does not like this method ask help to change it
> +      let enterButton = document.createEvent("KeyboardEvent");
> +      enterButton.initKeyEvent("keypress", true, true, window, false, false, false, false, 13, 0);
> +      this.searchInput.dispatchEvent(enterButton);

You shouldn't need to dispatch a keypress event to clear the search. Since you're running code here, why can't you just call in to a function from here to clear the search?

::: browser/components/preferences/in-content/findInPage.js
(Diff revision 6)
>     * Will attempt to initialize all uninitialized categories
>     */
>    initializeCategories() {
>      //  Initializing all the JS for all the tabs
>      if (!this.categoriesInitialized) {
> -      this.categoriesInitialized = true;

Please undo this change.

::: browser/components/preferences/in-content/findInPage.js:278
(Diff revision 6)
>     *    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
> +   *    Concatination of textNodes"s text content

Please undo this change.

::: browser/components/preferences/in-content/findInPage.js:290
(Diff revision 6)
>     * @returns boolean
>     *      Returns true when atleast one instance of search phrase is found, otherwise false
>     */
>    highlightMatches(textNodes, nodeSizes, textSearch, searchPhrase) {
> +	if (this.initialSearch) {
> +      this.addToTries(textSearch.replace(/[.,\/#!$%\^&\*;:{}=\-_`~()]/g, "").removeStopWords().split(" "));

What is this doing?

::: browser/components/preferences/in-content/findInPage.js:474
(Diff revision 6)
>  
> -      //  Searching some elements, such as xul:button, have a 'label' attribute that contains the user-visible text.
> -      if (nodeObject.getAttribute("label")) {
> -        labelResult = this.stringMatchesFilters(nodeObject.getAttribute("label"), searchPhrase);
> +      //  Searching some elements, such as xul:button, have a "label" attribute that contains the user-visible text.
> +	  let labelContent = nodeObject.getAttribute("label");
> +      if (labelContent) {
> +				if (this.initialSearch) {
> +          this.addToTries(labelContent.replace(/[.,\/#!$%\^&\*;:{}=\-_`~()]/g, "").removeStopWords().split(" "));

Why does this need to happen in three places?

::: browser/components/preferences/in-content/findInPage.js:560
(Diff revision 6)
> +   *
> +   * @param Array of Strings listSuggestions
> +   *        List of autocompleted & related words
> +   */
> +	searchSuggestions(listSuggestions) {
> +    let object = document.getElementById("searchSuggestions");

s/object/dataList

::: browser/components/preferences/in-content/findInPage.js:562
(Diff revision 6)
> +    while (object.firstChild) {
> +      object.firstChild.remove();

dataList.innerHTML = "";

::: browser/components/preferences/in-content/findInPage.js:568
(Diff revision 6)
> +      option.setAttribute("value", word);
> +      option.setAttribute("data-value", word);

Why does this need to be set twice?

::: browser/components/preferences/in-content/tries.js:8
(Diff revision 6)
> +Trie.prototype.insert = function(word) {
> +	let node = this;

Please replace all tabs with spaces, and use 2-space indentation.

::: browser/components/preferences/in-content/tries.js:126
(Diff revision 6)
> +// wrote it with the help of http://stevehanov.ca/blog/index.php?id=114
> +Trie.prototype.levenshteinDistance = function(word, maxCost) {

Remove the Levenshtein functions and use NLP.jsm instead.

::: browser/components/preferences/in-content/tries.js:167
(Diff revision 6)
> +// Source : https://github.com/mirzaasif/JS-StopWord/blob/master/StopWord.js
> +var stopWords = "a,able,about,above,abst,accordance,according,accordingly,across,act,actually,added,adj,\

This giant string is not good for memory usage since strings in JavaScript are immutable and this string may live in memory for a long time.

Also, this is not localizer friendly. And I doubt many of these actually appear in the preferences.

It seems you may want to have a shorter list that is defined within a .properties file, and that only includes words found within the preferences that should be ignored.

::: browser/themes/shared/incontentprefs/preferences.inc.css:651
(Diff revision 6)
> +  /* backround-image: search-icon image, seach-close-button image
> +  *  http://stackoverflow.com/questions/6258521/clear-icon-inside-input-text
> +  */
> +  background-image: url(chrome://browser/skin/search-indicator-magnifying-glass.svg),url(chrome://global/skin/icons/searchfield-cancel.svg);
> +  border: 1px solid #999;
> +  padding: 4px 20px 4px 20px;     /* Use the same right padding (18) in jQ! */

What is jQ?

::: toolkit/themes/shared/in-content/common.inc.css:96
(Diff revision 6)
>    padding-top: 40px;
>    padding-inline-end: 44px; /* compensate the 4px margin of child elements */
>    padding-bottom: 48px;
>    padding-inline-start: 48px;
>    overflow: auto;
> +  position: relative; /* Search Bar alignment with header in about:preferences */

This should be moved to a separate bug.
Attachment #8850726 - Flags: review?(jaws) → review-
Comment on attachment 8850726 [details]
Bug 1349747 - Search Suggestions Implementation through trie tree, datalists.

https://reviewboard.mozilla.org/r/123258/#review131512

::: browser/components/preferences/in-content/tries.js:1
(Diff revision 6)
> +var Trie = function() {

This all needs documentation - extensive documentation.

::: toolkit/components/satchel/nsFormFillController.cpp:861
(Diff revision 6)
>  {
>    if (!mLastListener) {
>      return;
>    }
>  
> -  if (XRE_IsContentProcess()) {
> +  if (true) {

This is now fixed on Nightly, so this hack should be reverted.
Attachment #8850726 - Flags: review?(mconley) → review-
Comment on attachment 8850726 [details]
Bug 1349747 - Search Suggestions Implementation through trie tree, datalists.

https://reviewboard.mozilla.org/r/123258/#review132054

::: browser/components/preferences/in-content/findInPage.js:53
(Diff revisions 6 - 7)
>     * @param String className
>     *        Providing class name to bind appropriate CSS styling
>     */
>    classManipulation(classAction, className) {
>      let nodeObject = this.searchInput
> -    let regExp = new RegExp("(?:^|\\s)" + className + "(?!\\S)", "gi");
> +    

nit, please remove trailing whitespace here and other places. if you look at the patch on mozreview the trailing whitespace will be a red block.

::: browser/components/preferences/in-content/findInPage.js:56
(Diff revisions 6 - 7)
> -      let addName = nodeObject.className ? " " + className : className;
> -      nodeObject.className += addName;
> +      // let addName = nodeObject.className ? " " + className : className;
> +      // nodeObject.className += addName;

nit, please remove dead code and logging statements, here and elsewhere

::: browser/components/preferences/in-content/findInPage.js:60
(Diff revisions 6 - 7)
>        // Adding the class name
> -      let addName = nodeObject.className ? " " + className : className;
> -      nodeObject.className += addName;
> +      // let addName = nodeObject.className ? " " + className : className;
> +      // nodeObject.className += addName;
> +      nodeObject.classList.add(className);
>        if (className === "closeButton") {
>          document.getElementsByClassName(className)[0].addEventListener("mousedown", this);

in every case of this function, `document.getElementsByClassName(className)[0]` is called.

Please move this to the start of the function, and switch it with:
`document.querySelector(`.${className}`);

::: browser/components/preferences/in-content/findInPage.js:82
(Diff revisions 6 - 7)
>    handleEvent(event) {
> +    console.log("type",event.type);
>      if (event.type === "focus") {
>        this.initializeCategories();
>        if (!this.categoriesInitialized) {
> -        // Jared does not like this might have to implement it different way. Ask for help.
> +        // Initial search to load tries tree

We can't check this in without finding a better solution to initialize the tries trees. Why does text need to be put in the search input before calling searchFunction?

You should just be able to call searchFunction directly with a string, like searchFunction("").

Also, is there a less-expensive way to initialize the trie tree than performing a search (which will compare each string)?

::: browser/components/preferences/in-content/findInPage.js:96
(Diff revisions 6 - 7)
> -      let query = event.target.value.trim().toLowerCase()
> +      let query = event.target.value.trim().toLowerCase();
>        // Adding a class to show the close button
>        this.classManipulation(query, "closeButton");
> -
> -      // Stopping the infinite loop search for choosing a Datalist options value
> -      if (this.chooseDataListOption != query) {
> +      let overlayClose = document.getElementById("close-button-overlay");
> +      overlayClose.hidden = false;
> +      overlayClose.addEventListener("click",this);

nit, space after comma. I'm surprised this didn't fail eslint for you.

::: browser/components/preferences/in-content/findInPage.js:119
(Diff revisions 6 - 7)
> -      this.chooseDataListOption = query;
> +      // }
> +      // this.chooseDataListOption = query;
>        this.searchSuggestions(this.trie.autoComplete(String(query)));
>      } else if (event.type === "keypress") {
> -      if (this.timer != null) {
> -        this.timer.disarm();
> +      if (this.timer) {
> +        clearTimeout(this.timer);

Please remove the import of DeferredTask.jsm as well.

::: browser/components/preferences/in-content/findInPage.js:153
(Diff revisions 6 - 7)
> +      // above or below of the element
>        let heightOffSet = rect.height * 3;
>  
> -			if (child.className.indexOf("reverse") != -1) {
> -				child.className = "search-bubble-inner";
> -        parent.style.cssText = "top:" + (parseFloat(parent.style.top.slice(0, -2)) + heightOffSet).toString() + "px; left:" + parent.style.left.toString() + "; z-index:2;";
> +			if (child.classList.contains("reverse")) {
> +				child.classList.remove("reverse");
> +        parent.style.setProperty("bottom", "-31px");

Any CSS property that is hardcoded like this should be moved to a CSS file, and then it can be toggled by toggling a CSS class.

::: browser/components/preferences/in-content/findInPage.js:251
(Diff revisions 6 - 7)
>     * @returns boolean
>     *      Returns true when atleast one instance of search phrase is found, otherwise false
>     */
>    highlightMatches(textNodes, nodeSizes, textSearch, searchPhrase) {
>  	if (this.initialSearch) {
> -      this.addToTries(textSearch.replace(/[.,\/#!$%\^&\*;:{}=\-_`~()]/g, "").removeStopWords().split(" "));
> +      this.addToTries(textSearch);

indentation is busted here because the lines surrounding this are using tab characters.

::: browser/components/preferences/in-content/findInPage.js:550
(Diff revisions 6 - 7)
>     */
> -	addToTries(listOfWords) {
> +	addToTries(phrase) {
> +    // .replace is removing all special characters
> +    // then removing the stop words
> +    // then splitting it into list of words
> +    let listOfWords = phrase.replace(/[.,\/#!$%\^&\*;:{}=\-_`~()]/g, "").removeStopWords().split(" ");

This list is hardcoded in your JS file. Why can't it be hardcoded with all the junk already removed and in an array to begin with?

Also, please clean up that list as was requested in a previous review.

::: browser/components/preferences/in-content/findInPage.js:552
(Diff revisions 6 - 7)
> +      // Might need a better way to check if it is not just numbers
> +      if (isNaN(listOfWords[i])) {

Does something break if numbers get added?
Attachment #8850726 - Flags: review?(jaws) → review-
Comment on attachment 8850726 [details]
Bug 1349747 - Search Suggestions Implementation through trie tree, datalists.

Clearing review since the patch commit message doesn't include r?jaws or r?mconley. Please contact me if you wanted a review.
Attachment #8850726 - Flags: review?(mconley)
Attachment #8850726 - Flags: review?(jaws)
Blocks: 1357285
Whiteboard: [photon-preference]
Target Milestone: --- → Firefox 55
QA Contact: hani.yacoub
Flags: qe-verify+
Hi Francis,

Looks like this is within the intelligent search scope, which mean we won't release this at release 57, right? So we could remove the [photon-preference] from the whiteboard? If so, let's do it.
Flags: needinfo?(frlee)
indeed, this is smart search which is out of release 57 scope, remove whiteboard tag.
Flags: qe-verify+
Flags: needinfo?(frlee)
QA Contact: hani.yacoub
Whiteboard: [photon-preference]
Target Milestone: Firefox 55 → ---
Thanks, Francis.

And let's also remove the bug dependency fromo Project Photon now.
No longer blocks: 1357285
Hey Manotej - I'm going to unassign you from this one. A full-timer will drive this one through to a resolution. Thanks for your work!
Assignee: manotejmeka → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: