1263588 - Duplicated auto-complete items when using the same value (taken once from the form history and once from the data list)

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P2)

Description

[Adrian Florinescu [:aflorinescu]]

48.0a1 20160410030224 Mozilla/5.0 (X11; Linux i686; rv:48.0) Gecko/20100101 Firefox/48.0 /Ubuntu 14.04

FF duplicates the auto-complete items by populating with the same value taken once from the form history and once from the data list. 

[Affected versions]:
-Firefox Nightly 48
-Firefox Aurora 47
-Firefox Beta 46
-Firefox Release 45

[Affected platforms]:
All.


[Steps to reproduce]:
1. Use http://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_datalist with the default option values.
2. Auto-complete and submit "Firefox" value.
3. Auto-complete using "Fir". 


[Expected result]:
The auto-complete should show:
    Firefox


[Actual result]:
The auto-complete result will show 2 Firefox values:
    Firefox
    -------
    Firefox

[Regression range]:
- This is not a recent regression. Reproducible on Firefox 16.0 Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/20100101 Firefox/16.0.


[Additional notes]:
Same behavior E10s on/off.
Not reproducible on Edge or Chrome.

Comment 1

[Matthew N. [:MattN]]

This will require debugging the code at https://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormAutoComplete.js?rev=ee0cb75bcbf9&mark=144,220,294-295#138 (note that you need to handle cached and non-cached code paths) and updating or adding to the test at /toolkit/components/satchel/test/test_form_autocomplete_with_list.html

Comment 2

[mgsudhanva]

hello , i would like to work on this bug .

Comment 3

[mgsudhanva]

Attachment: Proposed patch for this bug

Comment 4

[mgsudhanva]

Comment on attachment 8780827 [details] [diff] [review]
Proposed patch for this bug

>--- nsFormAutoComplete.js	2016-08-13 17:16:34.920054550 +0530
>+++ test.js	2016-08-13 17:19:12.372380034 +0530
>@@ -151,6 +151,7 @@
>                                         aPreviousResult,
>                                         aDatalistResult,
>                                         aListener) {
>+        this.log("auto completed");
>        
>         function sortBytotalScore (a, b) {
>             return b.totalScore - a.totalScore;
>@@ -278,6 +279,7 @@
>             }
>         } else {
>             this.log("Creating new autocomplete search result.");
>+
>             // Start with an empty list.
>             let result = aDatalistResult ?
>                 new FormAutoCompleteResult(FormHistory, [], aInputName, aUntrimmedSearchString, null) :
>@@ -293,6 +295,25 @@
> 
>                 if (aDatalistResult && aDatalistResult.matchCount > 0) {
>                     result = this.mergeResults(result, aDatalistResult);
>+                    //remove the repeated value in _labels , which will remove the repeated option when a single match is found 
>+                    var result_lab = result['_labels'];
>+                        for (var t = 0; t<result_lab.length;t++){
>+                            for (var k = t+1;k<result_lab.length;k++){
>+                                if(k!=t && result_lab[t]==result_lab[k]){
>+                                    result['_labels'].splice(k,1);
>+                                }
>+                            }
>+                         }
>+
>+                    // remove the repeate values in _values , which will remove the repeated option from the first search itself
>+                    var result_lab1 = result['_values'];
>+                        for (var t = 0; t<result_lab1.length;t++){
>+                            for (var k = t+1;k<result_lab1.length;k++){
>+                                if(k!=t && result_lab1[t]==result_lab1[k]){
>+                                    result['_values'].splice(k,1);
>+                                }
>+                            }
>+                        }
>                 }
> 
>                 if (aListener) {
>@@ -305,6 +326,7 @@
>     },
> 
>     mergeResults(historyResult, datalistResult) {
>+        this.log('mergeResults');
>         let values = datalistResult.wrappedJSObject._values;
>         let labels = datalistResult.wrappedJSObject._labels;
>         let comments = new Array(values.length).fill("");

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8780827 [details] [diff] [review]
Proposed patch for this bug

Review of attachment 8780827 [details] [diff] [review]:
-----------------------------------------------------------------

Hello, can you submit future patches with mozreview or provide 8 lines of context please?

https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html

Note that this file is getting refactored in bug 1294502.

Please add a test like I mentioned in comment 1 and also make sure you are fixing the cache where we re-use the result (e.g. one more letter is typed at the end).

::: nsFormAutoComplete.js
@@ +307,5 @@
> +
> +                    // remove the repeate values in _values , which will remove the repeated option from the first search itself
> +                    var result_lab1 = result['_values'];
> +                        for (var t = 0; t<result_lab1.length;t++){
> +                            for (var k = t+1;k<result_lab1.length;k++){

Do we need 2 sets of nested loops? Why can't we do this in one loop? I would recommend looking into using a Set object https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

@@ +326,4 @@
>      },
>  
>      mergeResults(historyResult, datalistResult) {
> +        this.log('mergeResults');

Have you considered doing the deduping in mergeResults?

Comment 6

[mgsudhanva]

Attachment: Bug 1263588 - Duplicated auto-complete items when using the same value (taken once from the form history and once from the data list)

Removed the loops and used deduping to removed the repeated values . Have changed the format of patch , used mercurial . About the tests , i am new to it and is there any documentation about how to write the test.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8782392 [details] [diff] [review]
Bug 1263588 - Duplicated auto-complete items when using the same value (taken once from the form history and once from the data list)

Review of attachment 8782392 [details] [diff] [review]:
-----------------------------------------------------------------

Please provide more lines of context in your patches or use mozreview.

Can you please update existing tests to work with this? You can run them with ./mach test toolkit/components/satchel/

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +294,5 @@
>                  if (aDatalistResult && aDatalistResult.matchCount > 0) {
>                      result = this.mergeResults(result, aDatalistResult);
> +                    //Dedupe the results : label and values
> +                    var result_labels = result['_labels'];
> +                    var result_values = result['_values'];

Please use double-quotes for most mozilla-central JS

@@ +296,5 @@
> +                    //Dedupe the results : label and values
> +                    var result_labels = result['_labels'];
> +                    var result_values = result['_values'];
> +                    result['_labels'] = uniq(result_labels);
> +                    result['_values'] = uniq(result_values);

There's not enough context in this patch for me to know if the separator between the datalist and history results will continue to work with this patch.

Comment 8

[mgsudhanva]

(In reply to Matthew N. [:MattN] from comment #7)
> Comment on attachment 8782392 [details] [diff] [review]
> Bug 1263588 - Duplicated auto-complete items when using the same value
> (taken once from the form history and once from the data list)
> 
> Review of attachment 8782392 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please provide more lines of context in your patches or use mozreview.
> 
> Can you please update existing tests to work with this? You can run them
> with ./mach test toolkit/components/satchel/
> 
> ::: toolkit/components/satchel/nsFormAutoComplete.js
> @@ +294,5 @@
> >                  if (aDatalistResult && aDatalistResult.matchCount > 0) {
> >                      result = this.mergeResults(result, aDatalistResult);
> > +                    //Dedupe the results : label and values
> > +                    var result_labels = result['_labels'];
> > +                    var result_values = result['_values'];
> 
> Please use double-quotes for most mozilla-central JS
> 
> @@ +296,5 @@
> > +                    //Dedupe the results : label and values
> > +                    var result_labels = result['_labels'];
> > +                    var result_values = result['_values'];
> > +                    result['_labels'] = uniq(result_labels);
> > +                    result['_values'] = uniq(result_values);
> 
> There's not enough context in this patch for me to know if the separator
> between the datalist and history results will continue to work with this
> patch.

I will provide more lines of context from the next patch . What is that i have to include more , if you could provide me the details i will add . The line separator is working , i have looked into it . And regarding adding test, i have never written  a test before and i dont't know whats happening in that test file , so can you help me in that .

Comment 9

[Monika Maheshwari [:MonikaMaheshwari]]

Hey, anyone working on this? I would like to work on this issue.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I think this should be implemented in mergeResults https://searchfox.org/mozilla-central/rev/45e308665eb9fc52fd21e2d4b3b959f3cec13ef1/toolkit/components/satchel/FormAutoComplete.jsm#514
A Set can be used, but you must pay attention to retain the order and association of results, each result has a value, a label and a comment. So one can't just build Sets from all the arrays and call it a day, that'd lose association. Comments are empty, so they don't matter.
What matters in practice is values, so one can build a Set of all the values, and then go by index through the concatenated value array, if the current value is in the Set remove it and continue, if it's not in the Set, then it was already removed and it's a duplicate. That index should then be spliced away from all the arrays.
The most complex part is probably understanding and updating test_form_autocomplete_with_list.html

Comment 11

[gloriaeskor]

I would like to work on this

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Bugs are assigned by the system when the first patch is attached, feel free to start working on it and ask questions if you get stuck.
Note this is NOT a good-first-bug, if this is the first time you work on the Firefox codebase, I'd suggest to pick a good-first-bug from https://codetribute.mozilla.org/projects/ff

Comment 13

[gloriaeskor]

Alright, Thank you marco

Comment 14

[Issam Mani [:issammani]]

Attachment: Bug 1263588 - Deduplicate autocomplete suggestions. r=sgalich

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abb937122136
Deduplicate autocomplete suggestions. r=sgalich

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/abb937122136