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)
Firefox
Search
Tracking
()
People
(Reporter: alice0775, Assigned: Gavin)
References
Details
(Keywords: regression)
Attachments
(1 file)
1.48 KB,
patch
|
MattN
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
[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.
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 1•10 years ago
|
||
Bug 1007979 destroys Firefox.
Comment 2•10 years ago
|
||
Yes. This won't be enough for a dot release but, if low risk, we could take a patch for 33.1
Reporter | ||
Comment 3•10 years ago
|
||
screen capture: http://youtu.be/uPoPb2tZEWk
Comment 4•10 years ago
|
||
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
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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.
status-firefox33:
affected → ---
status-firefox34:
affected → ---
status-firefox35:
affected → ---
status-firefox36:
affected → ---
status-firefox-esr31:
unaffected → ---
Assignee | ||
Updated•10 years ago
|
Points: --- → 3
Flags: qe-verify+
Version: 34 Branch → Trunk
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
I pushed this to fx-team without a test: https://hg.mozilla.org/integration/fx-team/rev/46db8dc8ce2c
Target Milestone: --- → Firefox 36
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/46db8dc8ce2c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Iteration: --- → 36.3
Updated•10 years ago
|
QA Contact: petruta.rasa
Comment 12•10 years ago
|
||
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 14•9 years ago
|
||
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 15•9 years ago
|
||
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?
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8514372 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Updated•9 years ago
|
Comment 17•9 years ago
|
||
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.
Description
•