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)

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.

Working on gnomestripe and winstripe now. Starting work on pinstripe.

Ok, seems to be working across each OS now.

Fixing an erroneous comment.

Comment on attachment 683618 [details] [diff] [review] Patch v2 Ok, I think this is ready for a review.

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

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.

Updated the stylings for pinstripe and winstripe.

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.

Great! Thanks for the review. Going to make sure this looks OK on OSX and Windows, and then I'll land this puppy.

Comment on attachment 684477 [details] [diff] [review] Patch v5 - r+'d by mak Looks good and behaves properly on OSX and Windows - landing...

Landed on mozilla-inbound here: https://hg.mozilla.org/integration/mozilla-inbound/rev/70910dd1f72b

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