305667 - Download manager inaccessible after download started with alt+click/alt+Enter or context menu save as

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Aaron Leventhal]

When the download manager automatically starts showing the new download it
doesn't get selected.

Sometimes I can see a selection being created and then removed. It's as if a
dummy download item is first inserted, and then removed and then the new one is
added.

The original fix I had for initial selection in the download manager fixes this.

Doron, sorry you didn't like that fix -- but I have complaints about this not
working and would like some fix for Firefox 1.5.

Comment 1

[Aaron Leventhal]

Attachment: Use original fix for initial focus in <richlistbox>

Comment 2

[Doron Rosenberg (IBM)]

(In reply to comment #1)
> Created an attachment (id=193608) [edit]
> Use original fix for initial focus in <richlistbox>
> 

Make sure it doesn't cause problems in the extension manager.

Comment 3

[Doron Rosenberg (IBM)]

I have to admit, I cannot reproduce this on the 1.8 branch in firefox.

Comment 4

[Doron Rosenberg (IBM)]

oops, I somehow reset the blocking 1.8b4 without getting a mid air page

Comment 5

[Aaron Leventhal]

You can't repro if you get a file naming dialog in the middle.

Comment 6

[Aaron Leventhal]

> Make sure it doesn't cause problems in the extension manager.

What kinds of problems? Selection always goes to the first item both in nightly
builds and with this patch.

Comment 7

[Doron Rosenberg (IBM)]

(In reply to comment #5)
> You can't repro if you get a file naming dialog in the middle.

How do I disable getting the file naming dialog?

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 193608 [details] [diff] [review]
Use original fix for initial focus in <richlistbox>

r=me, without the extraneous brackets I'm continually harping about.

Comment 9

[Doron Rosenberg (IBM)]

Attachment: different approach

listens for template rebuilds.	It assumes each element has an id and that the
id stays same for each refresh, but works fine for download manager.

Aaron, could you see if that fixes the issue for you?

Comment 10

[Aaron Leventhal]

Comment on attachment 193708 [details] [diff] [review]
different approach

This also works, and Doron would prefer we use this patch for <richlistbox>.

Comment 11

[Aaron Leventhal]

Comment on attachment 193608 [details] [diff] [review]
Use original fix for initial focus in <richlistbox>

Waiting until we see if Doron's alternative approach gets r+, before we decide
which patch to ask for a= on

Comment 12

[Doron Rosenberg (IBM)]

Comment on attachment 193708 [details] [diff] [review]
different approach

this was just a sample, real patch coming up in a sec

Comment 13

[Doron Rosenberg (IBM)]

Attachment: new patch

Had to do more code to make it more generic, might be too much for branch
though.

Also removed some EM code no longer needed because of this and added code to
download manager to fix pause/continue of downloads losing selecting (they
don't rebuild the template but do other changes that can't be listened to
easily).

Comment 14

[Aaron Leventhal]

Comment on attachment 193720 [details] [diff] [review]
new patch

Mike, is this new patch from Doron better? It also fixes the problem, and Doron
prefers it.

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 193720 [details] [diff] [review]
new patch

>+            // if we find no item, clear selection so that the code at the bottom
>+            // takes over
>+            if (item)
>+              this.selectedItem = item;
>+            else
>+              this._selectedItem = null;
>+          }

I think you mean this._selectedItem = item here, I hope, because I don't know
what else calls this.

>+          // if no id or the id no longer exists (above code fails)
>+          if (!this._selectedItem) {
>+            if (this._selectedIndex >= this.getRowCount()) {
>+              // select the last item
>+              this.selectedIndex = this.getRowCount() - 1;
>+            } else {
>+              this.selectedIndex = this._selectedIndex;
>+            }
>+          }

extra bogus brackets!

>Index: toolkit/mozapps/downloads/content/downloads.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v
>retrieving revision 1.47.2.1
>diff -u -r1.47.2.1 downloads.js
>--- toolkit/mozapps/downloads/content/downloads.js	16 Aug 2005 12:10:38 -0000	1.47.2.1
>+++ toolkit/mozapps/downloads/content/downloads.js	24 Aug 2005 17:52:08 -0000
>@@ -270,15 +270,37 @@
> 
> function onDownloadPause(aEvent)
> {
>+  var selectedIndex = gDownloadsView.selectedIndex;
>+
>   var uri = aEvent.target.id;
>   gDownloadManager.pauseDownload(uri);
>   setRDFProperty(uri, "DownloadStatus", aEvent.target.getAttribute("status-internal"));
>   setRDFProperty(uri, "ProgressPercent", aEvent.target.getAttribute("progress"));
>+
>+  // now reset the richlistbox
>+  gDownloadsView.clearSelection();
>+  if (selectedIndex >= gDownloadsView.getRowCount())
>+    gDownloadsView.selectedIndex = selectedIndex - 1;
>+  else
>+    gDownloadsView.selectedIndex = selectedIndex;
>+
>+  gDownloadsView.selectedItem.focus();
> }

if gDownloadsView.selectedIndex is greater than getRowCount(), then
selectedIndex - 1 is still invalid, we should use var rows =
gDownloadsView.getRowCount(); and set gDownloadsView.selectedIndex to rows - 1;
to get the last one if selectedIndex is too high.  You do this right in the
binding.

> function onDownloadResume(aEvent)
> {
>+  var selectedIndex = gDownloadsView.selectedIndex;
>+
>   gDownloadManager.resumeDownload(aEvent.target.id);
>+
>+  // now reset the richlistbox
>+  gDownloadsView.clearSelection();
>+  if (selectedIndex >= gDownloadsView.getRowCount())
>+    gDownloadsView.selectedIndex = selectedIndex - 1;
>+  else
>+    gDownloadsView.selectedIndex = selectedIndex

see previous comment.  And yes, I know this is cut and paste, but it seems
wrong and we should fix it now that I've noticed.

This does seem to be the better patch, and does some necessary cleanup, though
its bigger.  Let's go with this, with comments addressed

Comment 16

[Doron Rosenberg (IBM)]

Attachment: with mconnor's comments addressed

Comment 17

[Asa Dotzler [:asa]]

Can you get this landed on the trunk and verified there before checking into the
branch, please.

Comment 18

[Doron Rosenberg (IBM)]

checked into trunk

Comment 19

[Aaron Leventhal]

This patch made it so that if you Alt+click on a download and you have more than
a screenfull already, the new download starts scrolled off the top of the
richlistbox so you can't see it unless you scroll.

Comment 20

[Aaron Leventhal]

I backed this out of the branch. The original fix which still has review doesn't 
have this problem.

We can handle this in 2 ways:
1. Use the original fix after all
2. Fix Doron's fix

Doron, are you willing to look at #2 or should we just go with #1?

Comment 21

[Aaron Leventhal]

Backed out of trunk.

Comment 22

[Aaron Leventhal]

Comment on attachment 193720 [details] [diff] [review]
new patch

This patch seems to have at least 2 problems:
1) it scrolls off the first item when there is more than a page full of items,
so that you can't even see the item you're downloading when the dialog opens
automatically
2) If you tab to a cancel button for an ongoing download and hit Enter, the
arrow keys won't work after that.

I am going to seek a= for the original fix, which deals with the problem of
items going away in templates in one place in the code, and it works. I don't
like this need to scatter fixes piecemeal all over for each special case. We're
never going to get every one, especially because anyone can use richlistbox.

Comment 23

[Aaron Leventhal]

Comment on attachment 193608 [details] [diff] [review]
Use original fix for initial focus in <richlistbox>

We could also remove the downloads.js changes from bug 301435 if we check this
in, since they're just one special case related to avoiding invalid selection,
and it's better to deal with it all in one place.

Comment 24

[Doron Rosenberg (IBM)]

(In reply to comment #22)
> (From update of attachment 193720 [details] [diff] [review] [edit])
> This patch seems to have at least 2 problems:
> 1) it scrolls off the first item when there is more than a page full of items,
> so that you can't even see the item you're downloading when the dialog opens
> automatically

Would probably have to special case that in downloads.js

> 2) If you tab to a cancel button for an ongoing download and hit Enter, the
> arrow keys won't work after that.

That probably means the focus is on the dialog and not the richlistbox.

Don't have time this weekend though, will take a look monday.

Comment 25

[Aaron Leventhal]

We should fix the problem as close to the source of the problem as possible,
otherwise there is always going to be another place to patch.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 193608 [details] [diff] [review]
Use original fix for initial focus in <richlistbox>

please re-request branch approval when you have a working fix landed and
verified on the trunk. thanks.

Comment 27

[Aaron Leventhal]

Used original fix this time.

Comment 28

[Doron Rosenberg (IBM)]

I filed bug 306522 for doing this the correct way.

Comment 29

[Asa Dotzler [:asa]]

If this is fixed on the 1.8 branch, please add the fixed1.8 keyword. Thanks.

Comment 30

[Asaf Romano (gone)]

fixed1.8, i assume.

Comment 31

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381