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

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Andriy Syrovenko]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Marco,

Should we be tracking this for FF 20?

-Mike

Comment 2

[Marco Bonardo [:mak]]

We would have tracked it as a blocker in case.  It's too late for this kind of changes in 20.

Comment 3

[Loic]

Another report: http://forums.mozillazine.org/viewtopic.php?f=9&t=2686847

Comment 4

[Eddie]

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.

Comment 6

[Gauthier Van Vreckem]

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.

Comment 7

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

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.

Comment 8

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

matching the code as per the comment.
Now when the selection is at the end of the list, selects the previous sibling.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 744111 [details] [diff] [review]
patch v0.2

Redirecting to Mano, who knows more about this code.

Comment 10

[Asaf Romano (gone)]

Wow, so, listbox.selectItems is just a plan js field. :-/

I prefer cloning the array (through Array.slice) in this case.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 744111 [details] [diff] [review]
patch v0.2

The other change seems fine.

Comment 12

[Girish Sharma [:Optimizer]]

(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.

Comment 13

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3

Using Array.slice().

Comment 14

[Girish Sharma [:Optimizer]]

Comment on attachment 745898 [details] [diff] [review]
patch v0.3

really want it to not miss this train.

Comment 15

[Dimitar Pankov]

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

Comment 16

[Eddie]

Bug confirmed as still existing in FF 21.0

Comment 17

[Girish Sharma [:Optimizer]]

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.

Comment 18

[Asaf Romano (gone)]

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).

Comment 19

[Girish Sharma [:Optimizer]]

Attachment: patch v0.4

Carry forward r+.
Added the comment explaining everything.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/311949ad294f

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/311949ad294f

Comment 22

[Girish Sharma [:Optimizer]]

Do we need to uplift this ?

Comment 23

[Marco Bonardo [:mak]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/536e5dd30998

Comment 26

[tech]

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.

Comment 27

[Marco Bonardo [:mak]]

(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.

Comment 28

[Marty Schrader]

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?

Comment 29

[Marco Bonardo [:mak]]

(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.

Comment 30

[Loic]

(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.

Comment 31

[Marty Schrader]

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.

Comment 32

[tech]

(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 ?

Comment 33

[Mike Conley (:mconley) (:⚙️)]

(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. :)

Comment 34

[Girish Sharma [:Optimizer]]

... or.. just wait 10 more days when your 22 becomes 23.

Comment 35

[Ioana (away)]

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?

Comment 36

[Girish Sharma [:Optimizer]]

(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  ???

Comment 37

[Loic]

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

Comment 38

[Ioana (away)]

(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.

Comment 39

[Girish Sharma [:Optimizer]]

o.O I myself am on Windows 7 and I can see this in Fx 22.

Comment 40

[Girish Sharma [:Optimizer]]

I meant I can this bug present in Fx 22.

Comment 41

[Ioana (away)]

(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/

Comment 42

[Girish Sharma [:Optimizer]]

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 ?

Comment 43

[u279076]

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

Comment 44

[Andriy Syrovenko]

(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.

Comment 45

[u279076]

Thanks Andriy!