Closed Bug 844606 Opened 11 years ago Closed 11 years ago

Clicking "Remove From History" in the downloads view removes only a single entry when several items are selected

Categories

(Firefox :: Downloads Panel, defect)

Product:
Firefox
Component:
Downloads Panel
Version:
20 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 24
Tracking Flags:
Tracking Status
firefox23 --- verified
firefox24 --- verified

People

(Reporter: andriys, Assigned: Optimizer)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

patch v0.4
2.84 KB, patch
Optimizer
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130220104816

Steps to reproduce:

1. Click "Downloads" from the Firefox menu.
2. In the Library window select several items from the Downloads list (using either Shift+Click or Ctrl+Click).
3. Right-Click on a selected item and choose "Remove From History" in the context menu.


Actual results:

Only the topmost selected item in the list gets removed.


Expected results:

All selected items in the list are removed.
Component: Untriaged → Bookmarks & History
Status: UNCONFIRMED → NEW
Component: Bookmarks & History → Downloads Panel
Ever confirmed: true
Marco,

Should we be tracking this for FF 20?

-Mike
We would have tracked it as a blocker in case.  It's too late for this kind of changes in 20.
Additional notes on bug:

Introduced in FF20, still present in 20.1

If multiple entries are selected from downloads list only the first item selected is deleted.

However if multiple items (in order) are selected from the bottom of the downloads list all the selected items are deleted.
Summary: Clicking "Remove From History" in the download panel removes only a single entry when several items are selected → Clicking "Remove From History" in the downloads view removes only a single entry when several items are selected
When using the old UI by setting browser.download.useToolkitUI to true
the delete behaves correctly.

The bug is only present on some download lists.
Doing a clear download solve it at least temporarily.
Attached patch patch v0.1 (obsolete) — — Splinter Review 
This patch:

1) While removing a single item, deselects the item first and then removes the item.
2) Selects the next entry only when there is one item in the selectedItems.
3) Cache the length of selectedItems and remove the first item, length number of times from the selectedItems. (Since the length of selectedItems will never be zero)

I switched to while from for .. of loop as for .. of loop caches the index to run on next. So when i=0, and I deselect the item i=0, the current i=1 becomes i=0 which is skipped by the for of loop as it runs on i=1 next.
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attachment #744110 - Flags: review?(mak77)
Attached patch patch v0.2 (obsolete) — — Splinter Review 
matching the code as per the comment.
Now when the selection is at the end of the list, selects the previous sibling.
Attachment #744110 - Attachment is obsolete: true
Attachment #744110 - Flags: review?(mak77)
Attachment #744111 - Flags: review?(mak77)
Attachment #744111 - Flags: review?(mak77) → review?(mconley)
Comment on attachment 744111 [details] [diff] [review]
patch v0.2

Redirecting to Mano, who knows more about this code.
Attachment #744111 - Flags: review?(mconley) → review?(mano)
Wow, so, listbox.selectItems is just a plan js field. :-/

I prefer cloning the array (through Array.slice) in this case.
Comment on attachment 744111 [details] [diff] [review]
patch v0.2

The other change seems fine.
Attachment #744111 - Flags: review?(mano) → feedback+
(In reply to Mano from comment #10)
> Wow, so, listbox.selectItems is just a plan js field. :-/
> 
> I prefer cloning the array (through Array.slice) in this case.

Wouldn't that be really heavy in case when then number of items is huge, lets say 100 to 500 ?

Already iterating through each item is heavy and removing 10 items form the list takes a noticeable time (around 1 second). May be the whole deleting and adding mechanism could be refactored, but that would be out of scope for this bug.
Attached patch patch v0.3 (obsolete) — — Splinter Review 
Using Array.slice().
Attachment #744111 - Attachment is obsolete: true
Attachment #745898 - Flags: review?(mano)
Comment on attachment 745898 [details] [diff] [review]
patch v0.3

really want it to not miss this train.
Attachment #745898 - Flags: review?(mak77)
Sadly the problem is still present in FF21.
So I guess it missed that train.
...
and I'd like to extend a bit the scope of the problem:
pressing the "delete" button on the keyboard doesn't work also - despite the fact that there are multiple items selected only the first one is removed.
...
and even worse - it deselects the rest of the previously selected items.
...
and even more worse is the fact that pressing "delete" loses the focus (the next item is not selected) every other press. What I mean is that pressing delete once removes the selected single item and selects the next one which makes it easy to delete the next one too but sadly this only works for the first deletion. The second one gets deleted and no item in the list gets selected making it impossible to delete another item until the user explicitly selects one with the mouse or the keyboard.
I really hope this was not intentional and will be fixed very quickly
Bug confirmed as still existing in FF 21.0
Hi Eddie and Dimitar, no need to confirm. The fact that this bug is still open, means that it is not even solved on the latest Nightly builds. Once this bug is fixed in Nightly, we will try to get the fix ported to Aurora (and / or Beta) Channels, which means that it can only land as latest as Firefox 22 in the best case scenario.
Attachment #745898 - Flags: review?(mak77)
Comment on attachment 745898 [details] [diff] [review]
patch v0.3

Please add a comment explaining that the the reason for cloning the array (just that selectedItems is updated lively and that doCommand may alter the selection).
Attachment #745898 - Flags: review?(mano) → review+
Attached patch patch v0.4 — — Splinter Review 
Carry forward r+.
Added the comment explaining everything.
Attachment #745898 - Attachment is obsolete: true
Attachment #756391 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/311949ad294f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Do we need to uplift this ?
Comment on attachment 756391 [details] [diff] [review]
patch v0.4

The fix looks safe enough for uplifting and the ui behavior is improved (this was one of the top feedback from input), so I think it's ok for Aurora. On the other side, this is not critical enough to do an evaluation for Beta imo.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): downloads panel feature
User impact if declined: multiple selection in the Library misbehaves
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #756391 - Flags: approval-mozilla-aurora?
Attachment #756391 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
There's something strange about this bug : it appeared when the download manager was unified with the history manager (a thing that I still don't understand as the two tools haven't the same behaviour at all and shouldn't be linked) but if you select many items in history and delete them at once it works, and it doesn't work only for the downloads.
(In reply to tech from comment #26)
> There's something strange about this bug : it appeared when the download
> manager was unified with the history manager (a thing that I still don't
> understand as the two tools haven't the same behaviour at all and shouldn't
> be linked) but if you select many items in history and delete them at once
> it works, and it doesn't work only for the downloads.

nothing strange, those views use different code.
Tested with 22.0 (release channel) and the issue remains UNFIXED, boys and girls. Let's not be too quick to dismiss active bugs, eh?
(In reply to Marty Schrader from comment #28)
> Tested with 22.0 (release channel) and the issue remains UNFIXED, boys and
> girls. Let's not be too quick to dismiss active bugs, eh?

as indicated in the target milestone and tracking flags this bug is fixed from Firefox 23 on.
(In reply to Marty Schrader from comment #28)
> Tested with 22.0 (release channel) and the issue remains UNFIXED, boys and
> girls. Let's not be too quick to dismiss active bugs, eh?

It's fixed in FF23+, read the flags.
Ah, so one has to be subscribed to the devel build channel to see these fixes? Sorry about that, all. Will be more careful next time.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/536e5dd30998

Hello, as I don't want to wait for the FF 23 release dealing with this boring bug, I would like to correct the file by myself before, but I don't find the file allDownloadsViewOverlay.js anywhere in my system. Could someone indicate me where it is located ?
(In reply to tech from comment #32)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/536e5dd30998
> 
> Hello, as I don't want to wait for the FF 23 release dealing with this
> boring bug, I would like to correct the file by myself before, but I don't
> find the file allDownloadsViewOverlay.js anywhere in my system. Could
> someone indicate me where it is located ?

Instead of monkey-patching your release version of Fireox, I'd suggest just getting a copy of Firefox 23 (Beta) here - http://www.mozilla.org/en-US/firefox/beta/

You don't need to uninstall your release version of Firefox either - you can run either (but not simultaneously, unless you're using separate profiles).

And then once Firefox 23 is released, you can remove the beta, update your release version, and you should be good to go.

Or you could stay on Beta - we love Beta testers. :)
... or.. just wait 10 more days when your 22 becomes 23.
Keywords: verifyme
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0

I can't reproduce this issue at all on builds with the bug, so I can't verify it either.

Can someone who reproduces it verify it on Firefox 23 and 24 please?
Keywords: verifyme
(In reply to Ioana Budnar, QA [:ioana] from comment #35)
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
> 
> I can't reproduce this issue at all on builds with the bug, so I can't
> verify it either.
> 
> Can someone who reproduces it verify it on Firefox 23 and 24 please?

Are you saying that using Firefox 22, when you select multiple entries in the downloads view and either press Delete, or right click and choose "Remove from history", All the selected entries get removed  ???
No, I see the bug in FF22 (and it's normal because it's fixed in FF23+).

If I select 3 entries in the middle of the download list, only 1 is removed.
See screenshot with 3 entries selected, only 1 is deleted: http://i.imgur.com/yf3bzQA.jpg
(In reply to Girish Sharma [:Optimizer] from comment #36)
> Are you saying that using Firefox 22, when you select multiple entries in
> the downloads view and either press Delete, or right click and choose
> "Remove from history", All the selected entries get removed  ???

Yes. I selected multiple entries (Ctrl+Click/Shift+Click), then I right-clicked one of them and chose "Remove from history". All the selected entries were removed.

I tried this at least 10 times with different downloads, selecting different amounts of entries, selecting them with Shift some times and Ctrl other times. It worked every time.
o.O I myself am on Windows 7 and I can see this in Fx 22.
I meant I can this bug present in Fx 22.
(In reply to Girish Sharma [:Optimizer] from comment #40)
> I meant I can this bug present in Fx 22.

Can you please verify it's fixed on Firefox 23 and 24 then? :)
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/23.0b9-candidates/build1/
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/
I downloaded both Firefox beta and aurora and this issue it not visible there, while it is visible in Firefox 22.

But I have to ask. I fixed this issue, am I the right person to verify ?
Andriy, since you reported this issues can you please verify it's fixed? It should be fixed in the following builds:

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/23.0b9-candidates/build1/
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/

Thanks
Flags: needinfo?(andriys)
Whiteboard: [qa-]
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #43)
> Andriy, since you reported this issues can you please verify it's fixed? It
> should be fixed in the following builds:

The issue is fixed in both latest FF23 beta and aurora.
Flags: needinfo?(andriys)
Thanks Andriy!
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
status-firefox24: fixed → verified
You need to log in before you can comment on or make changes to this bug.