Closed Bug 1083990 Opened 10 years ago Closed 10 years ago

Deletion of SearchBar history by DEL key appears to delete another entry and extra empty item appears

Categories

(Firefox :: Search, defect)

defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.3
Tracking Status
firefox34 - wontfix
firefox35 - verified
firefox36 - verified

People

(Reporter: alice0775, Assigned: Gavin)

References

Details

(Keywords: regression)

Attachments

(1 file)

[Tracking Requested - why for this release]: Regression by Bug 1007979 , and data loss and layout error


Steps To Reproduce:
1. Focus SearchBar Ctrl+k, Clear the textbox if text exists
2. Press UP arrow key to display Search History drop down 
3. Select a entry and press Del key

Actual Results;
Selected entry is deleted as expected. However,the another history entry is also deleted. And extra empty item appears

Expected  Results;
Only selected entry should be deleted. Dropdown should render properly.

Regression window(fx)
Good:
https://hg.mozilla.org/integration/fx-team/rev/94e41f59be31
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140729110404
Bad:
https://hg.mozilla.org/integration/fx-team/rev/54d57bd38f51
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140729113008
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=94e41f59be31&tochange=54d57bd38f51

Regressed by:
	54d57bd38f51	Matthew Noorenberghe — Bug 1007979 - Refactor nsSearchSuggestions into a reusable JSM. r=adw Original JSM by mconnor.
Bug 1007979 destroys Firefox.
Yes. This won't be enough for a dot release but, if low risk, we could take a patch for 33.1
screen capture:  http://youtu.be/uPoPb2tZEWk
Thanks for the report.

I can't reproduce any dataloss although it may be perceived as such. If I invalidate the form history last result by doing another search, the other entry that appeared to be deleted is still there. This is a UI bug only AFAICT.
Flags: firefox-backlog+
Keywords: dataloss
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Deletion of SearchBar history by del key deletes another history entry too. And extra empty item appears → Deletion of SearchBar history by DEL key appears to delete another entry and extra empty item appears
This is certainly noticeable but doesn't look to be major breakage. 

Gavin - Do you want to address this in 34 or later?
Flags: needinfo?(gavin.sharp)
Attached patch patchSplinter Review
I debugged the issue, and the problem seems to be:
- nsSearchSuggestions' onResultsReady passes the same array object to the final FormAutoCompleteResult it creates and returns
- when that FormAutoCompleteResult's removeValueAt() method is called in response to deleting the entry, it removes the value twice ("values" and "labels" point to the same array object here: http://hg.mozilla.org/mozilla-central/annotate/675913ddbb55/toolkit/components/satchel/nsFormAutoCompleteResult.jsm?mark=177#l177)

This was only "caused by" bug 1007979 in the specific STR here because it made us take the onResultsReady path for the formhistory-only case, where we used to just return the underlying result directly:

https://hg.mozilla.org/mozilla-central/rev/54d57bd38f51#l3.178

In Firefox 32 (i.e. prior to bug 1007979), I can reproduce a similar issue with slightly modified STR:
1) Add 3 form history results by searching with the search bar (e.g. "test", "test2", "test3")
2) Perform a search that produces both form history and search suggestions (e.g. "test"): https://cloudup.com/coH0upLQHfq
3) Select the 2nd form history result ("test3" in that screenshot) and delete it

Expected: 2nd form history result disappears
Actual: two form history results disappear, one suggestion moves into the non-suggestions area (https://cloudup.com/cDDnz9TON5t , https://cloudup.com/c6aZxeKSpLX)
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Flags: needinfo?(gavin.sharp)
Attachment #8514372 - Flags: review?(MattN+bmo)
Let's not track this specifically, but given the simplicity of the patch I will request uplifts once reviewed.

Need to think about a test too.
Points: --- → 3
Flags: qe-verify+
Version: 34 Branch → Trunk
Comment on attachment 8514372 [details] [diff] [review]
patch

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

A test would be good since it seems like something easy to regress. I'll leave that decision up to you.
Attachment #8514372 - Flags: review?(MattN+bmo) → review+
I pushed this to fx-team without a test:
https://hg.mozilla.org/integration/fx-team/rev/46db8dc8ce2c
Target Milestone: --- → Firefox 36
https://hg.mozilla.org/mozilla-central/rev/46db8dc8ce2c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Iteration: --- → 36.3
QA Contact: petruta.rasa
Since Firefox 4, the result following the deleted one from the suggestions list disappeared too.
The suggestions and search history results are correctly deleted now from search bar drop down.

Verified as fixed using Nightly 2014-11-17 under Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OSX 10.9.5.
Status: RESOLVED → VERIFIED
Comment on attachment 8514372 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1007979
[User impact if declined]: removing history suggestions from the search panel leaves the panel in an inconsistent state. Given that we are attracting attention on the search UI with bug 1088660, I think we should uplift this simple patch wherever we can.
[Describe test coverage new/current, TBPL]: on central for more than a week, and verified by QA.
[Risks and why]: low.
[String/UUID change made/needed]: none.
Attachment #8514372 - Flags: approval-mozilla-beta?
Attachment #8514372 - Flags: approval-mozilla-aurora?
Comment on attachment 8514372 [details] [diff] [review]
patch

Clearing the beta request as Florian told me that he nommed this to ensure that it gets into 35.
Attachment #8514372 - Flags: approval-mozilla-beta?
Attachment #8514372 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed using Firefox 35 beta 1 (20141201162954) under Win 7 64-bit, Ubuntu 14.04 and Mac OSX 10.9.5.
You need to log in before you can comment on or make changes to this bug.