For some reason, the focus ring doesn't seem to be displaying automatically around the first download item if the panel is opened with some downloads in progress. This is inconsistent with the behaviour on OSX and Ubuntu.

notice it should appear only if the panel is reached through keyboard, is that the case?

(In reply to Marco Bonardo [:mak] from comment #1) > notice it should appear only if the panel is reached through keyboard, is > that the case? For OSX and Ubuntu, no, this is not the case if there are items in the list. If there are items in the list, when the panel is opened with the mouse, the first item is focused.

We're currently displaying the focus ring automatically when the Downloads Panel is first opened via the mouse. However, we should only display the focus ring when using the keyboard.

The strange focus behaviour we're experiencing here seems to be fallout from bug 815273, where we made the panel focusable. As Marco points out in that bug: "My fear is that we are going to change the default panel focus behavior, so I'm totally unsure which focus bugs we may uncover." So, I think we've uncovered some here. This is a backout patch for bug 815273 - going to see how this changes focus behaviour on Windows and OSX.

Here are my findings so far for OSX: On OSX, the focus-ring is a blue-ish glow around the focused element. You can see this when you have the Awesomebar focused, for example. In content (and only in content, it seems), the focus-ring is the dotted outline. We do not show the focus-ring on widgets unless the user used a keyboard to get there. There is ONE exception however - we *do* seem to show focus-rings around buttons *in panels when they're focused programmatically*. For example, in a Scratchpad executing in the Browser environment, execute this code: setTimeout(function() { document.getElementById("editBookmarkPanelRemoveButton").focus(); }, 3000); And then open up the Edit Bookmarks Panel (star in the URL bar). After 3 seconds, the "Remove Bookmark" button will get the focus-ring. I would imagine that this is on purpose, in order to have a default focused element when a panel opens, and to make that element obvious to the user. "Show All Downloads" in the Downloads Panel is a button, with all of its chrome removed. We've been focusing this element programmatically when the panel opens, which is why we automatically see the focus-ring around it.

One correction to my last post - we do also show the focus-ring on richlistbox's in panels when focused programmatically.

So, talking with shorlander and mak, here's what I've determined: 1) We do not want to show any kind of focus ring when the panel is first opened. 2) The *only* case where we want to display the focus ring, is if the user presses TAB once the panel is open. This should select the first item in the list if one exists, or the "Show All Downloads" button if none exist. 3) We should reset the state of the focus rings being visible once the panel is closed. 4) On OSX, we should use the glow-y blue focus ring. On Windows and Linux, we'll stick with the dotted outline for now. 5) The user should be able to TAB focus into the stop / restart / open in finder icons in each download item. The fact that -moz-focusring seems to think that *every item* deserves a focus ring, regardless of how focus was achieved, might be a problem. Still investigating that.

This *might* get us in the right state for Windows. Linux and OSX are still tricky.

soft-blocker, mostly it's worth having it on the radar for further decisions.

This patch does the following: 1) Suppresses mousedown events when clicking the cancel/retry/open-in-folder download buttons. This way, the underlying richlistbox doesn't get focused and the richlistitem doesn't get selected when clicking those. This is similar to how Safari functions. 2) Removes the focusPanel code. 3) Restyles selected download items when the list is focused on OSX - I'm currently using the style we'd originally used for hover on completed downloads. 4) Restyles panel elements on OSX to use -moz-mac-focusring instead of the dotted outline.

After talking to gavin and margaret, I'm using a different technique here. We still focus an element when the panel opens - we focus a vbox that I've wrapped the panel contents in. This makes us able to focus the richlistbox with the first tab. Also, paste events are still heard by the panel with this technique.

Getting pretty close to a state where I'm going to want feedback. Going to try this on Ubuntu and Windows first though.

Windows is looking good. Next for Ubuntu...

Updated gnomestripes theme. Going to do some final tests on OSX, and then produce some screenshots, and then I think this is ready for feedback.

Ready for someone to take a poke at this. Screenshots coming.

It's pretty clear that the outline isn't fitting properly in those rounded corners...not sure what we can do about that - setting the outline radius to 6px actually makes it worse, which is why I opted for 5px for now. Open to suggestions here.

Another small flaw - the border of the selected download item seems to visually clash with the bottom border of the previous item. We had this same problem in the Places downloads view.

I tested it on Windows, and it looks quite good. The only defect I found is that if you have the focus ring on Show All Downloads and press down, it disappears, while it should stay there (if you have the ring on the first item in the list and press up it stays there, so the two should likely be coherent)

Comment on attachment 697054 [details] [diff] [review] Patch v1 Review of attachment 697054 [details] [diff] [review]: ----------------------------------------------------------------- mostly questions! ::: browser/components/downloads/content/download.xml @@ +44,5 @@ > style="width: &downloadDetails.width;" > crop="end" > xbl:inherits="value=status,tooltiptext=statusTip"/> > </xul:vbox> > + <xul:stack onmousedown="DownloadsView.onDownloadButtonMouseDown(event);"> I think this is hiding too much the scope of this simple handler, please just do "event.preventDefault();" here, and add a <!-- --> comment explaining it ::: browser/themes/pinstripe/downloads/downloads.css @@ +1,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +%include ../shared.inc I can't find what this is used for, if it's really used maybe it's worth a comment... @@ +89,5 @@ > > richlistitem[type="download"] { > margin: 0; > + border-left: 1px solid transparent; > + border-right: 1px solid transparent; what is this for? worth a comment? @@ +152,5 @@ > +#downloadsSummary:-moz-focusring, > +#downloadsListBox:-moz-focusring > richlistitem[type="download"][selected] { > + border-radius: 3px; > + border: 1px solid hsla(0,0%,0%,.1); > + background-color: #e5e5e5; why does this differ from the other focus rings?

(In reply to Marco Bonardo [:mak] from comment #21) > Comment on attachment 697054 [details] [diff] [review] > Patch v1 > > Review of attachment 697054 [details] [diff] [review]: > ----------------------------------------------------------------- > > mostly questions! > > ::: browser/components/downloads/content/download.xml > @@ +44,5 @@ > > style="width: &downloadDetails.width;" > > crop="end" > > xbl:inherits="value=status,tooltiptext=statusTip"/> > > </xul:vbox> > > + <xul:stack onmousedown="DownloadsView.onDownloadButtonMouseDown(event);"> > > I think this is hiding too much the scope of this simple handler, please > just do "event.preventDefault();" here, and add a <!-- --> comment > explaining it > So, after bug 787285 landed, I've noticed that if I preventDefault here, then the :active pseudoclass isn't applied, and we don't show the "clicked" state of the buttons. I've removed this hack for now, meaning that the download item is selected upon clicking any of these buttons. > ::: browser/themes/pinstripe/downloads/downloads.css > @@ +1,5 @@ > > /* This Source Code Form is subject to the terms of the Mozilla Public > > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > +%include ../shared.inc > > I can't find what this is used for, if it's really used maybe it's worth a > comment... > Good eyes - leftover fragment from an earlier iteration. I've removed it. > @@ +89,5 @@ > > > > richlistitem[type="download"] { > > margin: 0; > > + border-left: 1px solid transparent; > > + border-right: 1px solid transparent; > > what is this for? worth a comment? > I was using a border before for the focusring outline, which caused the richlistitem to "puff out" on either side, since no borders were there before. Switching to outline fixed that problem. > @@ +152,5 @@ > > +#downloadsSummary:-moz-focusring, > > +#downloadsListBox:-moz-focusring > richlistitem[type="download"][selected] { > > + border-radius: 3px; > > + border: 1px solid hsla(0,0%,0%,.1); > > + background-color: #e5e5e5; > > why does this differ from the other focus rings? I'm going off of shorlanders mockup: https://bug787285.bugzilla.mozilla.org/attachment.cgi?id=694437 And (guessing here), re-using the same colours that the footer uses.

Gets rid of the hack where we suppress the mousedown event for download item buttons. This should only affect OSX - and the effect isn't altogether unpleasant. Also removed some unnecessary CSS. Need to test this on Windows and Ubuntu, and then I'll re-request review.

(In reply to Mike Conley (:mconley) from comment #22) > > @@ +152,5 @@ > > > +#downloadsSummary:-moz-focusring, > > > +#downloadsListBox:-moz-focusring > richlistitem[type="download"][selected] { > > > + border-radius: 3px; > > > + border: 1px solid hsla(0,0%,0%,.1); > > > + background-color: #e5e5e5; > > > > why does this differ from the other focus rings? > > I'm going off of shorlanders mockup: > > https://bug787285.bugzilla.mozilla.org/attachment.cgi?id=694437 > > And (guessing here), re-using the same colours that the footer uses. That looks like selection, not focus ring, but maybe I'm missing something... the focus ring I see on other items of the panel on OSX is a light-blue border, but on richlist items is this sort of selection. on other platforms the focus ring is consistent (the same) across the footer, the buttons and a richlistitem...

(In reply to Marco Bonardo [:mak] from comment #24) > (In reply to Mike Conley (:mconley) from comment #22) > > > @@ +152,5 @@ > > > > +#downloadsSummary:-moz-focusring, > > > > +#downloadsListBox:-moz-focusring > richlistitem[type="download"][selected] { > > > > + border-radius: 3px; > > > > + border: 1px solid hsla(0,0%,0%,.1); > > > > + background-color: #e5e5e5; > > > > > > why does this differ from the other focus rings? > > > > I'm going off of shorlanders mockup: > > > > https://bug787285.bugzilla.mozilla.org/attachment.cgi?id=694437 > > > > And (guessing here), re-using the same colours that the footer uses. > > That looks like selection, not focus ring, but maybe I'm missing > something... the focus ring I see on other items of the panel on OSX is a > light-blue border, but on richlist items is this sort of selection. on other > platforms the focus ring is consistent (the same) across the footer, the > buttons and a richlistitem... You're correct - it seems that richlistitems have a different "focusring" than other elements. Text inputs and buttons seem to get the glowing blue ring, but richlists show selection - which is a dark grey background. Stephen suggested I simply copy Safari's approach to focus on OSX, which is the dark grey background; but then I saw the screenshot that I linked to in Comment 23, and opted for the brighter colour. How should I proceed?

I don't understand, we are not focusing anything, we are putting a focus ring, and its style should be consistent across elements, as it is on other plaforms. I think we should use the light-blue focus ring... mouseover is another story.

After talking with paolo in IRC, we've come up with a better solution. Essentially, the experience we're after is that in the Downloads Panel, we should not show focus unless the user has explicitly pressed the "tab" key. This is slightly problematic on OSX and Linux, where focusrings are shown in panels regardless of whether or not we used the mouse, keyboard, or script to gain focus. Our workaround for this is to listen for the tab key, and then set a "keyfocus" attribute on the panel, and have -moz-focusring rules ensure that the keyfocus attribute exists on the panel before being qualified. This achieves the desired effect. I'm going to try this on Ubuntu and Windows before requesting review.

Cosmetic fixups.

how do you exit from the tab mode? like if I tab and go down to item 3, and then mouseover item 1, do I have both a focus ring and a selection? If I press Enter at this point, which of the 2 will I activate? in menupopups item 1 is opened on Enter.

(In reply to Marco Bonardo [:mak] from comment #29) > how do you exit from the tab mode? like if I tab and go down to item 3, and > then mouseover item 1, do I have both a focus ring and a selection? If I > press Enter at this point, which of the 2 will I activate? in menupopups > item 1 is opened on Enter. Two solutions to this come to mind: 1. Make it so that if we've tab-focused, then we don't show any hover effects for completed downloads. Pressing keys affects the item that is selected. 2. If keys are pressed, check to see if any download items are being hovered, and dispatch the command to the hovered items controller. Which do you prefer? Or is there another solution that I've missed?

I honestly dislike most solutions we discussed so far... Let's rewind. I think we should totally forget the focusring concept here, and stick to the simple concept of "selection". User can select a download hovering it with the mouse, or he can move the selection with the keyboard (up / down). Only richlistbox items (downloads) can be selected. The last used input device is what sets selection (so if you move with keyboard to item 3, and then hover item 1 the selected item is 1). Basically this is how a menupopup commonly works. As selected styling we should use the current hover styling. So this may end up being "set selection on mouse hover". I briefly discussed with Mano about this and he seems to agree, he said later he may discuss possible implementations with you to simplify it.

(In reply to Marco Bonardo [:mak] from comment #31) > As selected styling we should use the current hover styling. We currently only have hover styling for completed downloads. In-progress downloads don't get any special styling on hover, currently, because you cannot click them. A download that is in-progress can be selected with the keyboard, however. Pressing Enter on an in-progress download has no effect. I think any changes to the mouse-only hover interaction should be double-checked with Stephen, to avoid unintentionally regressing the most common use case.

(In reply to Paolo Amadini [:paolo] from comment #32) > I think any changes to the mouse-only hover interaction should be > double-checked > with Stephen, to avoid unintentionally regressing the most common use case. There's serious risk (partially already happened) we go into a loop here, and never reach a final compromise. My take on "non-active" items is that we may just use a different selection style on them, wherever it looks like nothing or like the old focusring, I don't care. Though if keeping some items "different" causes us to delay this fix forever, I'd rather discard that request. We just need a simple selection behavior.

We've come to the consensus that our best option at this point is to simply remove the ability to navigate through the downloads in the panel with the keyboard. We'll revisit keyboard navigation in the future for a later iteration.

This patch makes the Downloads Panel a mouse-only destination (except for the accel-V paste shortcut). Removes all traces of focusrings and keyboard selection for the panel on OSX. Testing next on winstripe and gnomestripe.

Comment on attachment 700538 [details] [diff] [review] Patch v4 Whoops - pasted in a bad name.

Refinements for winstripe. gnomestripe is next...

Refinements for gnomestripe.

Comment on attachment 700557 [details] [diff] [review] Patch v4.2 This seems to do the trick.

Comment on attachment 700557 [details] [diff] [review] Patch v4.2 This is bitrotted by bug 828302 which has already landed on inbound. I'll prepare a dependent patch.

Comment on attachment 700557 [details] [diff] [review] Patch v4.2 Hm - nevermind. This seems to apply just fine.

(In reply to Mike Conley (:mconley) from comment #34) > We've come to the consensus that our best option at this point is to simply > remove the ability to navigate through the downloads in the panel with the > keyboard. This is a foul compromise. Screen readers can click the button even though it's not focusable by default.

(In reply to Dão Gottwald [:dao] from comment #42) > (In reply to Mike Conley (:mconley) from comment #34) > > We've come to the consensus that our best option at this point is to simply > > remove the ability to navigate through the downloads in the panel with the > > keyboard. > > This is a foul compromise. Screen readers can click the button even though > it's not focusable by default. At the risk of re-opening our discussion on this, I'll ask: what do you suggest we do instead?

our target for screen readers is the Library view, that's why CTRL+J opens the library and not the panel.

(In reply to Marco Bonardo [:mak] from comment #44) > our target for screen readers is the Library view, that's why CTRL+J opens > the library and not the panel. A transient view for active / most recent downloads makes just as much sense for screen readers. (In reply to Mike Conley (:mconley) from comment #43) > At the risk of re-opening our discussion on this, I'll ask: what do you > suggest we do instead? I couple of reasonable options have been discussed previously here. They all sounded ok to me.

(In reply to Dão Gottwald [:dao] from comment #45) > I couple of reasonable options have been discussed previously here. They all > sounded ok to me. The problem is that each of those options have issues we don't have a solution for. Most of those issues come from having some items in the panel that are not mouse-selectable (inactive with no default action), per design. Discussed possibilities: 1. Totally remove keyboard interaction, leave for future better design. 2. Allow all downloads to be selected, included currently unselectable items. Those may have a different style though (or different cursor). This simplifies things since mouse and keyboard interactions will then be coherent. Cons, different selection styles may confuse users and nothing happens when clicking unselectable-but-selected items. Should maybe do a "default" action? 3. Skip unselectable items with the keyboard too. Again simplifies things since both input methods are coherent. Cons, there's no way to interact with keyboard on those items (no way to "retry", "pause" or "cancel" them with keyboard). 4. Allow to select unselectable items only with the keyboard. This complicates code quite a bit (the last used input must always win, how menupopups work) and makes the 2 inputs incoherent (keyboard can select but mouse can't?). With keyboard all items would look selected the same, but some of them won't do anything on Enter (like in the Library view, though at this point the panel may just work the same and allow to select any entry?).

if the problem is just screen readers, we may take previous Mike's patch that improves behavior, but hide the focusring.

> 2. Allow all downloads to be selected, included currently unselectable > items. Those may have a different style though (or different cursor). This > simplifies things since mouse and keyboard interactions will then be > coherent. Cons, different selection styles may confuse users and nothing > happens when clicking unselectable-but-selected items. Should maybe do a > "default" action? Did you mean unselectable-but-focused? Otherwise I'm not sure what you mean, or what an unselectable item is in the first place. > 3. Skip unselectable items with the keyboard too. Again simplifies things > since both input methods are coherent. Cons, there's no way to interact with > keyboard on those items (no way to "retry", "pause" or "cancel" them with > keyboard). Those buttons being unfocusable would be ok if those actions were available from the context menu. > if the problem is just screen readers Not quite. Sighted users can similarly depend on using a keyboard or speech commands (no mouse anyway).

(In reply to Dão Gottwald [:dao] from comment #48) > Did you mean unselectable-but-focused? Otherwise I'm not sure what you mean, > or what an unselectable item is in the first place. Completed downloads show selected style and on click open the file. Other kind of downloads don't react (no selected style) and can't be clicked.

(In reply to Dão Gottwald [:dao] from comment #48) > Not quite. Sighted users can similarly depend on using a keyboard or speech > commands (no mouse anyway). True - but the Downloads Panel is primarily a mouse target, and has been (as far as I can tell) from the very beginning. This is why there's no ability to open the panel via the keyboard. I think we can assume that if the panel is open, that the user has mouse-ing capabilities.

As I said, the button opening the panel can be "clicked" without a mouse.

Gavin, any thoughts?

(In reply to Dão Gottwald [:dao] from comment #42) > (In reply to Mike Conley (:mconley) from comment #34) > This is a foul compromise. Screen readers can click the button even though > it's not focusable by default. I'm not an expert on how screen readers work, but if they can click the not-keyboard-accessible toolbar buttons, what's stopping them from also being able to click the not-keyboard-accessible panel items? In any case, I think we've established that the current situation is not optimal (even apart from the focus ring issue, the keyboard navigation in the panel seems buggy on Mac - once I tab/arrow past the "show all downloads" button I can only shift+tab back to the last entry, and then the tab focus gets "stuck" looping through that entry, its "show" button, and the "show all downloads" button). So we should investigate that in a followup, and seek advice from accessibility experts rather than speculating. Simplifying the existing code and removing the keyboard navigation as temporary compromise to ship this feature seems reasonable to me. But let's definitely get the bug on file for an accessibility review of the panel in general.

filed bug 829495 to get an accessibility overview.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #53) > (In reply to Dão Gottwald [:dao] from comment #42) > > (In reply to Mike Conley (:mconley) from comment #34) > > This is a foul compromise. Screen readers can click the button even though > > it's not focusable by default. > > I'm not an expert on how screen readers work, but if they can click the > not-keyboard-accessible toolbar buttons, what's stopping them from also > being able to click the not-keyboard-accessible panel items? AT software specifically handles toolbar buttons. It can't magically identify and expose non-standard widgets, as they just look like random nodes.

Besides, it's also not clear to me why the issue this bug was filed for would be severe enough to rip out keyboard access...

I will re-evaluate Patch v3.1, if the behavior is "acceptable" we may just take that for now. It may be bogus in some case but provided those are limited use-cases may work as good as removing navigation for our scope... Let's also see what out a11y team thinks of the current status of the feature.

patch v3.1 doesn't appear to work properly: 1. clicking on the downloads button and pressing down should focus the first item as tab does (also keydown should set the attribute) 2. tab sets selection style instead of focus ring on download items, at least on Mac 3. tab from a download item doesn't focus the button nor proceeds to Show all downloads, basically once i tab into I can only move with up/down 4. when we set the attribute we should probably also set a mouseover listener that removes the attribute and itself (also remove the attribute and the listener on popuphiding if still there)

we are working on a v3.2 patch that improves the situation. We can't primise miracles but should cover the most common cases.

Going back to our panel attribute technique. This seems to address all 4 of Marco's points in the last comment. I'll be testing this on gnomestripe next.

Hrm, so, I just tested this on Windows... this seems busted. I think our focusring workarounds might be superfluous on Windows.

Ok, this seems reasonable for Windows. Trying Ubuntu next.

So, here's the really infuriating thing - the behaviour on Windows seems to be a bit random. *Sometimes* pressing tab works to focus the first element, and *sometimes* it only works after the second tab (which then focuses the download button). I think there's something strange going on with the -moz-focusring rule in platform - it's only *sometimes* being applied in our case here.

Ok, so I think this puts us in better shape for Windows. We sidestep a lot of the -moz-focusring stuff by using :focus instead. I also added an ifdef to prevent us from adding the keyFocused mouseover handler, since that causes some strange behaviour when we natively focus something like the Show All Downloads button and then mouseover the panel*. (* What happens in this case, is that the focusring stays, but the "up" cursor won't show the focus ring on the list since the attribute is removed. I figured not removing the attribute is an OK compromise; though, alternatively, we could special-case when we've got the "Show All Downloads" button focused, but that feels like it crosses the hack-y threshold).

Marco is awesome is showed me how to override the global theme's focusring, so we're adding the mouseover handler again.

Amalgamated some keydown handlers, and explained why we use keypress in other places. We no longer show the hover effect in gnomestripe if keyFocusing (need to do pinstripe and winstripe still). Added a big comment about how our focusing hacks work.

Ok, removing the hover effect with focuskey on winstripe, and then I think I'm ready for review.

Ok, I think I'm nearly done here. Just going to give my patch a visual once-over.

Updated after IRC review.

the "click button, down, goes to show all downloads" bug is back when there's just one download item... I'm investigating locally possibility to simplify the handling here. basically the richlistbox just handles keypress if the event is not prevented, that means we could just add a capture keypress listener to it instead of a bubbling one and preventDefault when needed.

this applies on top of your patch, simplifies a bit the handler limiting them to one keypress and one keydown. Tested only on Windows.

feel free to merge the patches if this works correctly on Mac... I won't have further time to refine anything until monday

(In reply to Marco Bonardo [:mak] from comment #71) > Created attachment 701450 [details] [diff] [review] > patch v3.9 addition > > this applies on top of your patch, simplifies a bit the handler limiting > them to one keypress and one keydown. Tested only on Windows. Hey, this works OK on Mac, but I've noticed that you've removed the footer keypress handler. Are we OK with focus switching to the richlistbox if the user presses the down cursor while the footer is focused?

I've folded mak's patch into this version, and switched pinstripe to use :focus instead of :-moz-focusring. As I mentioned in the previous comment, this patch makes it so that if a user is using keyboard navigation, and have the "Show All Downloads" button focused, pressing the down cursor will focus the richlistbox (at least on pinstripe). If this is OK, then this patch is ready for review.

(In reply to Mike Conley (:mconley) from comment #74) > As I mentioned in the previous comment, this patch makes it so that if a > user is using keyboard navigation, and have the "Show All Downloads" button > focused, pressing the down cursor will focus the richlistbox (at least on > pinstripe). should be easy to fix changing this condition if (richListBox.selectedItem === richListBox.lastChild) to also check || document.activeElement.parentNode.id === "downloadsFooter"

Comment on attachment 701613 [details] [diff] [review] Patch v4 Review of attachment 701613 [details] [diff] [review]: ----------------------------------------------------------------- testing... ::: browser/components/downloads/content/downloads.js @@ +235,5 @@ > this._state == this.kStateShown; > }, > > + /** > + * Returns whether the user has starting keyboard navigation. typo "has starting" ::: browser/themes/gnomestripe/downloads/downloads.css @@ +32,5 @@ > #downloadsHistory > .button-box { > margin: 1em; > } > > +#downloadsPanel[keyfocus] > #downloadsFooter > #downloadsHistory:-moz-focusring > .button-box { why -moz-focusring in gnomestripe?

(In reply to Marco Bonardo [:mak] from comment #76) > Comment on attachment 701613 [details] [diff] [review] > Patch v4 I was able to get the focus to stay on the footer when pressing DOM_VK_DOWN by changing the onKeyPress handler to return early if keyboard navigation was activated instead of checking defaultPrevented in the DOM_VK_DOWN condition. This is because XUL buttons seem to preventDefault on down keypresses on their own[1], so we were skipping the original condition. > > Review of attachment 701613 [details] [diff] [review]: > ----------------------------------------------------------------- > > testing... > > ::: browser/components/downloads/content/downloads.js > @@ +235,5 @@ > > this._state == this.kStateShown; > > }, > > > > + /** > > + * Returns whether the user has starting keyboard navigation. > > typo "has starting" Fixed. > > ::: browser/themes/gnomestripe/downloads/downloads.css > @@ +32,5 @@ > > #downloadsHistory > .button-box { > > margin: 1em; > > } > > > > +#downloadsPanel[keyfocus] > #downloadsFooter > #downloadsHistory:-moz-focusring > .button-box { > > why -moz-focusring in gnomestripe? Removed. [1]: https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/button.xml#181

In this patch, we remove the blue-ish tinge to pinstripe's hover effect for completed downloads. Instead, we re-use the selection style that we use with keyboard navigation, and set cursor: pointer.

Comment on attachment 701853 [details] [diff] [review] Patch v4.2 Review of attachment 701853 [details] [diff] [review]: ----------------------------------------------------------------- Tested and retested, and seems to work in a solid way, well done. The only thing I'm not extremely happy about is number of additional rules to keep proper buttons states, I'm starting thinking we have too many visual states there. Thank you very much for the patience and dedication demonstrated in bringing this to a resolution. ::: browser/components/downloads/content/downloads.js @@ +399,5 @@ > + return; > + } > + > + // If keyboard navigation just started we don't want to go to the footer, > + // otherwise if there is only one element we'd skip it. with your early return this comment is no more needed, since we return it's clear we won't reach this code. @@ +406,5 @@ > + // focused, focus the footer. > + if (richListBox.selectedItem === richListBox.lastChild || > + document.activeElement.parentNode.id === "downloadsFooter") { > + DownloadsFooter.focus(); > + aEvent.preventDefault(); we can probably return here as well, we just moved the focus to the footer, the next handler is not interesting.

Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/db7914a84af3 \o/

Comment on attachment 701860 [details] [diff] [review] Patch v4.3 (r+'d by mak) [Approval Request Comment] Bug caused by (feature/regressing bug #): Downloads panel feature User impact if declined: broken ui Testing completed (on m-c, etc.): m-i (merge pending) Risk to taking this patch (and alternatives if risky): Limited to the feature String or UUID changes made by this patch: none

Comment on attachment 701860 [details] [diff] [review] Patch v4.3 (r+'d by mak) \o/

I confirm the features improved in this fix, mentioned by Mike above works as expected on FF 20b6 on Windows 7 x64, Ubuntu 12.04 and Mac OS 10.8: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0 BuildID 20 Beta 6: 20130320062118 It's possible there could be features that I did not saw in comments above but all of those that I understood are working as expected.