Closed Bug 851519 Opened 7 years ago Closed 7 years ago

Keyboard commands may apply to the wrong downloads panel entry

Categories

(Firefox :: Downloads Panel, defect)

20 Branch
All
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 23
Tracking Status
firefox20 + wontfix
firefox21 + wontfix
firefox22 --- verified
firefox23 --- verified

People

(Reporter: scoobidiver, Assigned: mak)

References

Details

(Keywords: dataloss, regression)

Attachments

(1 file, 2 obsolete files)

In the download panel, if you select a file entry and press the Del key, the bottom file entry is removed not the selected one.
It's very surprising and also inconsistent with the Library behavior.
We'll track (although we weren't able to repro on OS X), but this definitely wouldn't block the release.
Assignee: nobody → mak77
OS: All → Windows 7
Going to build on our second-to-last beta today, if there's not a simple, low-risk backout that can return us to a known-good state available for uplift in our last beta this will have to go unfixed for FF20.
(In reply to Scoobidiver from comment #0)
> In the download panel, if you select a file entry and press the Del key, the
> bottom file entry is removed not the selected one.
> It's very surprising and also inconsistent with the Library behavior.

What do you mean by the bottom entry and how did you select the entry, just with the mouse? mouse and keyword? was the selected entry highlighted or not?
Having better steps to reproduce would help reproducing the problem.
So far I cannot reproduce on Win7.
Flags: needinfo?(scoobidiver)
(In reply to Marco Bonardo [:mak] from comment #3)
> What do you mean by the bottom entry and how did you select the entry, just
> with the mouse? mouse and keyword? was the selected entry highlighted or not?
It's the entry of the bottom line. The entry is selected by hovering over it with the mouse (OK with arrows) and is highlighted in blue.
Flags: needinfo?(scoobidiver)
(In reply to Scoobidiver from comment #5)
> It's the entry of the bottom line. The entry is selected by hovering over it
> with the mouse (OK with arrows) and is highlighted in blue.

is this a real download, or the summary element ("N other downloads...") that usually opens the Library?
Flags: needinfo?(scoobidiver)
(In reply to Marco Bonardo [:mak] from comment #6)
> is this a real download, or the summary element ("N other downloads...")
> that usually opens the Library?
It's the third entry in Library/Download Panel that is removed. The downloaded file itself is not deleted of course.
Flags: needinfo?(scoobidiver)
I'm sorry, I may not have been clear, but you didn't report proper steps to reproduce the bug yet. I tried following the few information you posted, but I'm still unable to reproduce.

I need to know :
1. how many downloads are visible in the panel
2. what's the status of those downloads (in progress, failed, succeeded?)
2. what did you do with mouse and keyboard before pressing del
3. which of the downloads is selected when you press del
4. which entry is removed (you actually answered this question in comment 7)
I think you may have moved the mouse over Show All Downloads and pressing DEL removed the 3rd download? I could reproduce something similar now.
(In reply to Marco Bonardo [:mak] from comment #8)
> 1. how many downloads are visible in the panel
Three which is the maximum.
> 2. what's the status of those downloads (in progress, failed, succeeded?)
Succeeded.
> 2. what did you do with mouse and keyboard before pressing del
Hovering over one of the two top download entries. Nothing with keyboard.
> 3. which of the downloads is selected when you press del
The first or second one.
> 4. which entry is removed (you actually answered this question in comment 7)
The third one which is at the bottom.
I can reproduce this ever since the Downloads Panel was implemented in Bug 726444. 
Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20120418 Firefox/14.0a1
Build ID: 20120418030652

I've also made a screencast, all you have to do is focus the fist item in the panel with the mouse, and press the Del key. 
http://screencast.com/t/7iVPj1f9
thanks, quite strange, will try to investigate it asap.
Keywords: dataloss
So the problem is that we use the richlistbox with an hover behavior, the selectedItem is properly updated on opening the context menu, or selecting with the keyboard or clicking on the download (through the download binding).
But it's not properly updated when just hovering an entry and executing a command and we just apply the highlight through :hover.
Attached patch mouseover patch v1.0 (obsolete) — Splinter Review
This is the first possibility, a mouseover listener that updates the richlistbox selectedItem.  Off-hand doesn't look like we need a mouseout, the selection with the keyboard still works fine as well as any other mouseover on other items is handled.  And if the rlb is not focused we should not be caring.  But please let me know if I missed some case.
Attachment #734816 - Flags: review?(mano)
Attached patch keyfocusing patch (obsolete) — Splinter Review
just for completion, this is an alternative patch providing a different selectedItem through keyfocusing attribute and querySelector.  While it works in comparison with the mouseover patch it looks more hackish.
Summary: Pressing Del key deletes bottom file entry instead of selected file entry → Keyboard commands may apply to the wrong downloads panel entry
Comment on attachment 734816 [details] [diff] [review]
mouseover patch v1.0

So, what happens if one hovers an item, hover something outside the richlistbox, and then hit backspace?
Comment on attachment 734816 [details] [diff] [review]
mouseover patch v1.0

And if we go with this approach there's some code to be removed.
Attachment #734816 - Flags: review?(mano)
Comment on attachment 734816 [details] [diff] [review]
mouseover patch v1.0

Also, unlike the other patch, this one doesn't check the download state. Is it on purpose?
(In reply to Mano from comment #17)
> And if we go with this approach there's some code to be removed.

As discussed, there shouldn't be any more code to be removed, since the visual hover effect is just visual, doesn't have any functionality effect.

(In reply to Mano from comment #18)
> Also, unlike the other patch, this one doesn't check the download state. Is
> it on purpose?

Yes, the other patch was too restrictive, we don't visually show the hover effect on mouseover, but we allow acting on the same entry through the keyboard.

(In reply to Mano from comment #16)
> So, what happens if one hovers an item, hover something outside the
> richlistbox, and then hit backspace?

The last selected item will be acted upon.
This looks wrong sure, though considered this:
http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#325
we basically always ensure there is a selection in the RLB, if we keep this condition I can't do differently. I may reset the selectedIndex to 0, but would be even more surprising.
Or we should reconsider the whole selection behavior and remove the "has always selection" condition, that means user can't just click on the button to open the panel and press a button to act on the first entry.
Attached patch patch v1.1Splinter Review
This appears to work fine after various tests.
As I suspected some of the code was not happy to be without a selection, but wasn't too hard to fix it.
The mouseout is not clean as I'd like it, though it's the most reliable solution I found, also cause (I suspect due to the arrow panel animation) for a while the richlistbox causes mouseover and mouseout events on panel opening.  That is on the empty richlistbox, so I'm rather handling events on the richlistitems.
Attachment #734816 - Attachment is obsolete: true
Attachment #734818 - Attachment is obsolete: true
Attachment #735229 - Flags: review?(mano)
https://hg.mozilla.org/mozilla-central/rev/63c003375416
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 735229 [details] [diff] [review]
patch v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Downloads panel feature
User impact if declined: the keyboard commands on the downloads panel may act on the wrong download, that means the user may delete the wrong entry.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): limited impact to selection behavior, backout is feasible in case
String or IDL/UUID changes made by this patch: none
Attachment #735229 - Flags: approval-mozilla-aurora?
Is the plan to land this patch on beta as well?
Comment on attachment 735229 [details] [diff] [review]
patch v1.1

Approving on aurora , as this is a recent regression from the download panel feature in FX 20 and the fix has limited impact to selection behavior and a back-out is possible if needed . 

Will request some verification on this before we consider uplift on beta .
Attachment #735229 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Scoobidiver from comment #0)
> In the download panel, if you select a file entry and press the Del key, the
> bottom file entry is removed not the selected one.
> It's very surprising and also inconsistent with the Library behavior.

Scoobidiver,

Thanks for the report .Can you please help us verify the issue is fixed and the feature works as expected for you on aurora once the patch lands or nightly ? Thanks !
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #24)
> Is the plan to land this patch on beta as well?

I didn't request that just cause the beta rules are stricter, no other reasons.
(In reply to bhavana bajaj [:bajaj] from comment #26)
> Thanks for the report .Can you please help us verify the issue is fixed and
> the feature works as expected for you on aurora once the patch lands or
> nightly ?
It's OK on Aurora but on Nightly the download panel no longer shows up.
In 23.0a1/20130421, it also works as expected.
Status: RESOLVED → VERIFIED
(In reply to Marco Bonardo [:mak] from comment #27)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #24)
> > Is the plan to land this patch on beta as well?
> 
> I didn't request that just cause the beta rules are stricter, no other
> reasons.

Well, this bug is tracking-firefox21+ as well - are you suggesting we WONTFIX it there, or do you think the risk from taking this patch in a late beta is manageable?
I don't think the risk is adapt for a beta. it's low but it's still something that _potentially_ may break the ui and only affects a small amount of users that contemporary rely on mouse and keyboard interaction for the same UI piece.
Imho should be wontfix for beta.
(In reply to Marco Bonardo [:mak] from comment #32)
> I don't think the risk is adapt for a beta. it's low but it's still
> something that _potentially_ may break the ui and only affects a small
> amount of users that contemporary rely on mouse and keyboard interaction for
> the same UI piece.
> Imho should be wontfix for beta.

Thanks for the input :mak. If we think the fallout's may be worse then lets maintain the status-quo for one more cycle and let this get fixed on Fx 22 instead.
Depends on: 1115964
Blocks: 991965
You need to log in before you can comment on or make changes to this bug.