Closed Bug 809852 Opened 8 years ago Closed 8 years ago

Allow cycling through the "show download history" entry using the arrows

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: mak, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

using the arrows should be possible to move through the download items AND the Show All Downloads entry (or whatever we end up with in bug 808277)
Depends on: 808277
Attached patch WIP Patch 1 (obsolete) — Splinter Review
This patch allows the user to key down to the summary node when pressing the down button on the last item in the downloads list.

Pressing up when the summary node is focused will re-focus the last item in the download list.

Pressing enter or space when the summary node is focused will open the downloads manager.

Currently only styled for gnomestripe. I'll have pinstripe and winstripe done soon.
Assignee: nobody → mconley
Attached patch WIP Patch 2 (obsolete) — Splinter Review
Working on gnomestripe and winstripe now. Starting work on pinstripe.
Attachment #683287 - Attachment is obsolete: true
Attached patch Patch v1 (obsolete) — Splinter Review
Ok, seems to be working across each OS now.
Attachment #683315 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Fixing an erroneous comment.
Attachment #683616 - Attachment is obsolete: true
Comment on attachment 683618 [details] [diff] [review]
Patch v2

Ok, I think this is ready for a review.
Attachment #683618 - Flags: review?(mak77)
As suggested by mak in IRC, I investigated moving the summary in as a permanent-resident as the last item in the downloads list. That'd allow us to take advantage of richlistbox's keyboard event handling, and mean we didn't have to hand-roll it ourselves.

I just investigated it, and it looks like this introduces other hacks - we have to special-case the summary node in all of our key, click, and drag event bindings for the richlistbox.

So, in my opinion, I think our current approach is the less hack-y one (though they're both pretty hacky).
ok, thanks for investigating that possibility. I see we have many more handlers than I remembered and that may easily go out of control.
Comment on attachment 683618 [details] [diff] [review]
Patch v2

Review of attachment 683618 [details] [diff] [review]:
-----------------------------------------------------------------

doesn't seem to handle Show All History

::: browser/components/downloads/content/downloads.js
@@ +787,5 @@
> +        // Are we focused on the last element in the list?
> +        if (DownloadsSummary.visible &&
> +            this.richListBox.currentIndex == (this.richListBox.itemCount - 1)) {
> +          DownloadsSummary.focus();
> +        }

We should key down regardless DownloadsSummary, if it's not visible should be possible to key down to Show All History (that is the title of this bug).

So probably you want to check currentIndex externally, and inside distinguish the visible case.

This adds some complexity since you need a keypress handler on the button to handle key up. Alternatively we have to figure a way to unify the 2 bottom elements, or wrap them in a common box to handle that keypress (luckily the button already handles enter and space).

@@ +1425,5 @@
>   * of items in the list exceeds the panels limit.
>   */
>  const DownloadsSummary = {
>  
> +  _visible: false,

just to reduce blame changes you may leave this after the getter
Attachment #683618 - Flags: review?(mak77)
Attached patch Patch v3 (obsolete) — Splinter Review
Oops - you're right. I was focusing entirely on the summary, and forgot to deal with the hidden-summary case.

I've wrapped the summary and the "show all downloads" button in a footer vbox, and added a DownloadsFooter singleton in downloads.js to handle events and manage focus.
Attachment #683618 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
Updated the stylings for pinstripe and winstripe.
Attachment #684061 - Attachment is obsolete: true
Attachment #684064 - Flags: review?(mak77)
Blocks: 814388
Comment on attachment 684064 [details] [diff] [review]
Patch v4

Review of attachment 684064 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/content/downloads.js
@@ +1547,5 @@
> +    switch (aEvent.keyCode) {
> +      case KeyEvent.DOM_VK_ENTER:
> +      case KeyEvent.DOM_VK_RETURN:
> +        DownloadsPanel.showDownloadsHistory();
> +    }

considered the actual cases, I'd just go with a simple
if (aEvent.charCode == " ".charCodeAt(0) ||
    aEvent.keyCode == ||
    aEvent.keyCode ==)

we may add the switch in future if it will ever be needed

@@ +1654,5 @@
> +          DownloadsView.richListBox.selectedIndex =
> +            (DownloadsView.richListBox.itemCount - 1);
> +        }
> +        break;
> +    }

Switch is far too much for a single case, just add condition to the if.

if (aEvent.keyCode == KeyEvent.DOM_VK_UP &&
    DownloadsView.richListBox.itemCount > 0)

::: browser/components/downloads/content/downloadsOverlay.xul
@@ +104,5 @@
>                     onkeypress="DownloadsView.onDownloadKeyPress(event);"
>                     oncontextmenu="DownloadsView.onDownloadContextMenu(event);"
>                     ondragstart="DownloadsView.onDownloadDragStart(event);"/>
>  
> +      <vbox id="downloadsFooter" onkeypress="DownloadsFooter.onKeyPress(event);">

please put onkeypress on its own line, so it's more visible

::: browser/themes/gnomestripe/downloads/downloads.css
@@ +24,5 @@
>    cursor: pointer;
>  }
>  
>  #downloadsSummary,
> +#downloadsPanel[hasdownloads] > vbox > #downloadsHistory {

nit: this doesn't hurt, though if I got correctly how parsing works, it's likely there's no win from using just "#downloadsPanel[hasdownloads] #downloadsHistory", cause there is only one #downloadsHistory that has only one parentNode (vbox), that again has only 1 parentNode (#downloadsPanel). It can't move from there.
I'd probably go for the short version everywhere unless there's a good reason to not do so.

::: browser/themes/pinstripe/downloads/downloads.css
@@ +68,2 @@
>    padding: 8px 38px 8px 12px;
>    cursor: pointer;

nit: minor thing, please keep rules in the same order across different themes, the focus here is the first rule, while in gnomestripe and winstripe is the last one. This doesn't matter but when trying to keep themes in sync may help diffing.
Attachment #684064 - Flags: review?(mak77) → review+
Great! Thanks for the review. Going to make sure this looks OK on OSX and Windows, and then I'll land this puppy.
Attachment #684064 - Attachment is obsolete: true
Comment on attachment 684477 [details] [diff] [review]
Patch v5 - r+'d by mak

Looks good and behaves properly on OSX and Windows - landing...
Attachment #684477 - Attachment description: Patch v5 → Patch v5 - r+'d by mak
https://hg.mozilla.org/mozilla-central/rev/70910dd1f72b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
nit: you left > vbox > in pinstripe
(In reply to Marco Bonardo [:mak] from comment #16)
> nit: you left > vbox > in pinstripe

Well, this is embarrassing. :/

Sorry about that - I was certain I'd gotten them all. I'll push a follow-up patch to inbound.

Thanks for catching it!
Stashing here while I test on my OSX machine before pushing.
Ok, going for it - landing follow-up on inbound as:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f9890a9c3f
Depends on: 814961
You need to log in before you can comment on or make changes to this bug.