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

NEW
Unassigned

Status

()

Toolkit
Form Manager
P2
normal
2 years ago
2 years ago

People

(Reporter: adrian_sv, Unassigned, Mentored)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox48 affected)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Has STR: --- → yes
Summary: Duplicated auto-complete items when having the same value taken once from the form history and once from the data list → Duplicated auto-complete items when using the same value (taken once from the form history and once from the data list)
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
Mentor: MattN+bmo
Priority: -- → P2
Whiteboard: [lang=js]

Comment 2

2 years ago
hello , i would like to work on this bug .

Comment 3

2 years ago
Created attachment 8780827 [details] [diff] [review]
Proposed patch for this bug

Comment 4

2 years ago
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("");
Attachment #8780827 - Flags: review?(MattN+bmo)
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?
Attachment #8780827 - Flags: review?(MattN+bmo) → feedback+

Comment 6

2 years ago
Created 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)

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.
Attachment #8780827 - Attachment is obsolete: true
Attachment #8782392 - Flags: review?(MattN+bmo)
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.
Attachment #8782392 - Flags: review?(MattN+bmo) → feedback+

Comment 8

2 years ago
(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 .
You need to log in before you can comment on or make changes to this bug.